Coding standard phpcs

Hi!

This needs a re-roll against 2.0.x. I'd be delighted to review if someone could please re-roll this.

Cheers.

Re-rerolled the patch against 2.0.x-dev. Please review

The last submitted patch, 14: 3322807-14.patch, failed testing. View results →

Updated patch.

I will review it.

Initial review:

1. +++ b/social_media.api.php
   @@ -1,54 +0,0 @@
   -<?php
   

   We probably shouldn't straight up delete the API example.

2. +++ b/src/Event/SocialMediaEvent.php
   @@ -2,17 +2,47 @@
   + * Conditional statements to support Drupal 8 and 9.
   ...
   +   * Array element.
   

   I think this description needs work. What exactly is `Array element`? A build array? Something else?

3. +++ b/src/Event/SocialMediaEvent.php
   @@ -20,7 +50,7 @@ class SocialMediaEvent extends Event {
   +   *   @todo Describe what element is.
   

   Let's fix this todo.

4. +++ b/src/Event/SocialMediaEvent.php
   @@ -40,7 +70,7 @@ class SocialMediaEvent extends Event {
   +   *   @todo Describe what element is.
   

   ditto on fixing todo.

5. +++ b/src/Form/SocialMediaAdminForm.php
   @@ -67,14 +67,14 @@ class SocialMediaAdminForm extends ConfigFormBase {
   +        '#title' => $this->t('@social_media settings', ['@social_media' => $label]),
   

   Typically we don't translate variables. The translation should occur within `$social_medias` (or whatever code generates that array).

6. +++ b/src/Plugin/Block/SocialSharingBlock.php
   @@ -51,15 +52,23 @@ class SocialSharingBlock extends BlockBase implements ContainerFactoryPluginInte
   +  public function __construct(array $configuration, $plugin_id, $plugin_definition, ConfigFactoryInterface $config_factory, Token $token, EventDispatcherInterface $event_dispatcher, CurrentPathStack $current_path, ModuleExtensionList $extension_list_module) {
        parent::__construct($configuration, $plugin_id, $plugin_definition);
        $this->configFactory = $config_factory;
        $this->token = $token;
        $this->eventDispatcher = $event_dispatcher;
        $this->currentPath = $current_path;
   +    $this->extensionListModule = $extension_list_module;
      }
    
      /**
   @@ -67,7 +76,7 @@ class SocialSharingBlock extends BlockBase implements ContainerFactoryPluginInte
   
   @@ -67,7 +76,7 @@ class SocialSharingBlock extends BlockBase implements ContainerFactoryPluginInte
       */
      public static function create(ContainerInterface $container, array $configuration, $plugin_id, $plugin_definition) {
        return new static(
   -      $configuration, $plugin_id, $plugin_definition, $container->get('config.factory'), $container->get('token'), $container->get('event_dispatcher'), $container->get('path.current')
   +      $configuration, $plugin_id, $plugin_definition, $container->get('config.factory'), $container->get('token'), $container->get('event_dispatcher'), $container->get('path.current'), $container->get('extension.list.module')
        );
      }
   

   Instead of modifying the constructor signature (b/c incompatible change), and instead use the `::create` method to inject services?

   See https://www.drupal.org/node/3076421 → for a shining example of how this is done.

7. +++ b/src/Plugin/Block/SocialSharingBlock.php
   @@ -151,7 +160,9 @@ class SocialSharingBlock extends BlockBase implements ContainerFactoryPluginInte
   +   * @todo describe what this does and what $element is.
   

   Ditto on the todo.

8. +++ b/src/Plugin/Block/SocialSharingBlock.php
   @@ -163,7 +174,9 @@ class SocialSharingBlock extends BlockBase implements ContainerFactoryPluginInte
   +   * @todo describe what this does and what $variables is.
   

   Ditto on the todo.

9. +++ b/src/Plugin/Block/SocialSharingBlock.php
   @@ -182,7 +195,9 @@ class SocialSharingBlock extends BlockBase implements ContainerFactoryPluginInte
   +   * @todo describe what this does and what $variables is.
   

   Ditto on the todo.

10. +++ b/src/Plugin/Field/FieldFormatter/SocialMediaFormatter.php
    @@ -30,12 +31,20 @@ class SocialMediaFormatter extends FormatterBase implements ContainerFactoryPlug
    -  public function __construct($plugin_id, $plugin_definition, FieldDefinitionInterface $field_definition, array $settings, $label, $view_mode, array $third_party_settings, BlockManagerInterface $block_manager) {
    +  public function __construct($plugin_id, $plugin_definition, FieldDefinitionInterface $field_definition, array $settings, $label, $view_mode, array $third_party_settings, BlockManagerInterface $block_manager, AccountProxyInterface $account) {
         parent::__construct($plugin_id, $plugin_definition, $field_definition, $settings, $label, $view_mode, $third_party_settings);
         $this->blockManager = $block_manager;
    +    $this->account = $account;
       }
     
       /**
    @@ -43,7 +52,7 @@ class SocialMediaFormatter extends FormatterBase implements ContainerFactoryPlug
    
    @@ -43,7 +52,7 @@ class SocialMediaFormatter extends FormatterBase implements ContainerFactoryPlug
        */
       public static function create(ContainerInterface $container, array $configuration, $plugin_id, $plugin_definition) {
         return new static(
    -      $plugin_id, $plugin_definition, $configuration['field_definition'], $configuration['settings'], $configuration['label'], $configuration['view_mode'], $configuration['third_party_settings'], $container->get('plugin.manager.block')
    +      $plugin_id, $plugin_definition, $configuration['field_definition'], $configuration['settings'], $configuration['label'], $configuration['view_mode'], $configuration['third_party_settings'], $container->get('plugin.manager.block'), $container->get('current_user')
         );
    

    Ditto on avoiding overriding constructors for B/C reasons.

My rationality for breaking away from overriding constructors is that the change is already backwards-incompatible. Better off positioning ourselves for future-proofing since this is the origin of the 2.x branch!

Thanks!

🚨 Addendum to the review from #20 - this also now needs an empty post-update hook, since the we need to force a container rebuild (due to changing constructor parameters).

Let's see what the all-knowing test bot thinks about this...

Oops! Forgot to include the new post_update file.

Hiding older patches.

Add missing typehint to ::sortSocialMedias method.

Removed PHPCS output and cleaned up issue summary -- any contributor can run this command locally. No need to repeat automation results within the issue.

Setting back to NR for #25. I cannot review this now, as I've had a direct hand in crafting it.

Proof that

1. The patch applies
2. There are no PHPCS violations

can be found at https://www.drupal.org/pift-ci-job/2662791 → (Drupal 10) and https://www.drupal.org/pift-ci-job/2662792 → (Drupal 9).

What we need here is real human review and manual testing / confirmation that nothing has broken.

Thanks!

Tested patch #25.
applied cleanly on dev branch - 2.0.x

Tested phpcs via phpcs --standard=DrupalPractice & phpcs --standard=Drupal -> No errors Found.
Tested functionality via adding fields, works as expected.

Big +1 on the RTBC.

I have a site that's running with social_media 1.9-rc2 plus the patch from 🐛 Dispatcher error with Block Closed: outdated .

Then I required 2.0.0-dev. Everything is still working (the block, the form).

(Oh, also the patch no longer applied because it is obsolete as noted on that issue.)

Then I apply the patch in #25. Everything still works. I didn't even have to run updb. I'm confused why nothing freaked out. My cache is turned on. Regardless, the empty post_update hook seems like is should be necessary. And obviously it shows up and runs when I call drush updb.

I even reverted the patch, added an event subscriber, triggered some events, applied the patch, triggered more events. Everything appears to be in great working order.

I also added a social_media field and that appeared to work fine.

I really appreciate the link to that excellent Webform CR: Code refactored to not override constructors for service objects, plugins, and controllers → . I've always handled things "the old way" and I look forward to handling them "the new way" moving forward.

Excellent work, all, with particular thanks to the new co-maintainer @Luke.Leber!

Committed and pushed #25 to 2.0.x.

Thanks all, stand by for 2.0.0 stable.

Automatically closed - issue fixed for 2 weeks with no activity.