The patches in #2868553: Fix the code to adhere to Drupal Best Practice had several changes which were not strictly required to get the codebase clean of PHPCS warning. That issue is now fixed, and the codebase has no warnings. This issue contains all the other changes which were not committed, and can be discussed here.

Patch to follow.

CommentFileSizeAuthor
#10 unused use statement sniff on travis.png160.34 KBjonathan1055
#10 unused use statement sniff on d.o..png129.88 KBjonathan1055
#4 Remove_unused_statements_use_EntityTypeManager_fix_annotations_and_other_tidy_up-3033783-4-D8.patch16.43 KBthalles
#2 3033783-1.other_bits_of_tidy_up.patch12.76 KBjonathan1055

Comments

jonathan1055 created an issue. See original summary.

jonathan1055’s picture

jonathan1055 commented
Status: Active » Needs review
StatusFileSize
new 3033783-1.other_bits_of_tidy_up.patch12.76 KB

Here's the patch with all the left-over changes. Some things, such as correcting @var Drupal\scheduler to @var \Drupal\scheduler are easy. But other things need a bit more explanation, e.g. what is the reason for using EntityTypeManager instead of EntityManager

idebr’s picture

idebr commented

Looks good! I have two minor remarks:

  1. +++ b/src/Form/SchedulerCronForm.php
@@ -24,19 +25,22 @@ class SchedulerCronForm extends ConfigFormBase {
-   * @var Drupal\scheduler\SchedulerManager
+   * @var \Drupal\scheduler\SchedulerManager

    There are similar @var parameters that need a prefix in the \Drupal\scheduler_rules_integration\Event namespace, for example ExistingNodeIsScheduledForPublishingEvent.php:

      /**
   * The node which is being scheduled and saved.
   *
   * @var Drupal\node\NodeInterface
   */
  public $node;

    should be
    @var \Drupal\node\NodeInterface 

  2. +++ b/src/Form/SchedulerCronForm.php
@@ -45,7 +49,7 @@ class SchedulerCronForm extends ConfigFormBase {
   public static function create(ContainerInterface $container) {
-    return new static($container->get('module_handler'), $container->get('scheduler.manager'));
+    return new static($container->get('config.factory'), $container->get('module_handler'), $container->get('scheduler.manager'));
   }

    Each service is typically placed on a new line throughout Drupal core.

thalles’s picture

thalles
Primary language Portuguese, Brazil
Location Teófilo Otoni - MG
commented
StatusFileSize
new Remove_unused_statements_use_EntityTypeManager_fix_annotations_and_other_tidy_up-3033783-4-D8.patch16.43 KB

Thanks for the invite @jonathan1055!

Guys, follow the patch!

idebr’s picture

idebr commented
Status: Needs review » Reviewed & tested by the community

The patch in #4 applies correctly and fixes the remarks in #3.

jonathan1055’s picture

jonathan1055 commented

Good work, thanks.

From #2 can you tell me

what is the reason for using EntityTypeManager instead of EntityManager?

This was introduced by idebr in #12 of the other issue. I am not saying I disagree, I just want to know if there is a benefit in making this change. Scheduler appears to work the same with either version. But I expect there is a good reason to switch, so I would like to learn.

idebr’s picture

idebr commented

The entity manager has been split up into different classes, see the change record at https://www.drupal.org/node/2549139

The usage of entity manager is becoming deprecated and will be removed before Drupal 9.0.0, see #2886622: Deprecate all EntityManager methods with E_USER_DEPRECATED

  • jonathan1055 committed b747455 on 8.x-1.x 
    Issue #3033783 by jonathan1055, thalles, idebr: Remove unused statements...
jonathan1055’s picture

jonathan1055 commented
Status: Reviewed & tested by the community » Fixed

Ah, that makes sense now, why it works the same with both. Thanks @idebr for the explanation and links.

Committed and fixed. Thank you all for this combined effort.

jonathan1055’s picture

jonathan1055 commented
StatusFileSize
new unused use statement sniff on d.o..png129.88 KB
new unused use statement sniff on travis.png160.34 KB

The unused use statements were not detected by PHPCS, neither running on d.o. or the Travis test builds. The sniffs were used (see attached), so does anyone know how/why they were not detected?

unused use statement

unused use statement

Status: Fixed » Closed (fixed)

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