In #2861902: Fix coding standards violations in 8.x codebase all coding standards violations were fixed, and many changes were made in line with Drupal Best Practice. There are some warnings which still need to be fixed, which were out of scope of that original issue, and should be dealt with here.

CommentFileSizeAuthor
#20 2868553_20.scheduler_best_practice.patch16.53 KBjonathan1055
#19 scheduler-coding_standards-2868553-19.patch23.16 KBbzhazreal
#19 interdiff_15-19.txt1.81 KBbzhazreal
#15 interdiff_12_15.txt908 byteskeenegan
#15 scheduler-coding_standards-2868553-15.patch21.28 KBkeenegan
#12 scheduler-coding_standards-2868553-12.patch20.76 KBidebr
#6 2868553-6.best_practice.admin_and_cron_forms.patch3.17 KBjonathan1055
#4 2868553-4.code_style.patch1.57 KBjonathan1055

Comments

jonathan1055 created an issue. See original summary.

jonathan1055’s picture

jonathan1055 commented

The output from phpcs --standard=DrupalPractice on the current codebase is:

FILE: ...scheduler_rules_integration/scheduler_rules_integration.info.yml
----------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
----------------------------------------------------------------------
 6 | WARNING | All dependencies must be prefixed with the project
   |         | name, for example "drupal:"
 7 | WARNING | All dependencies must be prefixed with the project
   |         | name, for example "drupal:"
----------------------------------------------------------------------


FILE: ...8/modules/scheduler/src/Controller/LightweightCronController.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
----------------------------------------------------------------------
 25 | WARNING | \Drupal calls should be avoided in classes, use
    |         | dependency injection instead
 42 | WARNING | \Drupal calls should be avoided in classes, use
    |         | dependency injection instead
----------------------------------------------------------------------


FILE: ...uments/drupal8/modules/scheduler/src/Form/SchedulerAdminForm.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
 92 | WARNING | \Drupal calls should be avoided in classes, use
    |         | dependency injection instead
----------------------------------------------------------------------


FILE: ...cuments/drupal8/modules/scheduler/src/Form/SchedulerCronForm.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
 141 | WARNING | \Drupal calls should be avoided in classes, use
     |         | dependency injection instead
----------------------------------------------------------------------


FILE: ...ver/Documents/drupal8/modules/scheduler/src/SchedulerManager.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 16 WARNINGS AFFECTING 13 LINES
----------------------------------------------------------------------
  80 | WARNING | \Drupal calls should be avoided in classes, use
     |         | dependency injection instead
  92 | WARNING | \Drupal calls should be avoided in classes, use
     |         | dependency injection instead
 114 | WARNING | Node::loadMultiple calls should be avoided in
     |         | classes, use dependency injection instead
 173 | WARNING | t() calls should be avoided in classes, use
     |         | dependency injection and $this->t() instead
 183 | WARNING | t() calls should be avoided in classes, use
     |         | dependency injection and $this->t() instead
 187 | WARNING | \Drupal calls should be avoided in classes, use
     |         | dependency injection instead
 187 | WARNING | t() calls should be avoided in classes, use
     |         | dependency injection and $this->t() instead
 229 | WARNING | \Drupal calls should be avoided in classes, use
     |         | dependency injection instead
 241 | WARNING | \Drupal calls should be avoided in classes, use
     |         | dependency injection instead
 261 | WARNING | Node::loadMultiple calls should be avoided in
     |         | classes, use dependency injection instead
 322 | WARNING | t() calls should be avoided in classes, use
     |         | dependency injection and $this->t() instead
 332 | WARNING | t() calls should be avoided in classes, use
     |         | dependency injection and $this->t() instead
 336 | WARNING | \Drupal calls should be avoided in classes, use
     |         | dependency injection instead
 336 | WARNING | t() calls should be avoided in classes, use
     |         | dependency injection and $this->t() instead
 445 | WARNING | \Drupal calls should be avoided in classes, use
     |         | dependency injection instead
 445 | WARNING | t() calls should be avoided in classes, use
     |         | dependency injection and $this->t() instead
----------------------------------------------------------------------


FILE: ...sts/modules/scheduler_access_test/scheduler_access_test.info.yml
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
 7 | WARNING | All dependencies must be prefixed with the project
   |         | name, for example "drupal:"
----------------------------------------------------------------------


FILE: ...ler/tests/modules/scheduler_api_test/scheduler_api_test.info.yml
----------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
----------------------------------------------------------------------
 7 | WARNING | All dependencies must be prefixed with the project
   |         | name, for example "drupal:"
 8 | WARNING | All dependencies must be prefixed with the project
   |         | name, for example "drupal:"
----------------------------------------------------------------------

and here is the summary

PHP CODE SNIFFER REPORT SUMMARY
----------------------------------------------------------------------
FILE                                                  ERRORS  WARNINGS
----------------------------------------------------------------------
...s/scheduler/config/install/scheduler.settings.yml  0       0
...nstall/views.view.scheduler_scheduled_content.yml  0       0
...ules/scheduler/config/schema/scheduler.schema.yml  0       0
...r/Documents/drupal8/modules/scheduler/LICENSE.txt  0       0
...ler/plugins/content_types/scheduler_form_pane.inc  0       0
...er/Documents/drupal8/modules/scheduler/README.txt  0       0
...nts/drupal8/modules/scheduler/scheduler.admin.inc  0       0
...ments/drupal8/modules/scheduler/scheduler.api.php  0       0
...nts/drupal8/modules/scheduler/scheduler.drush.inc  0       0
...ents/drupal8/modules/scheduler/scheduler.info.yml  0       0
...ments/drupal8/modules/scheduler/scheduler.install  0       0
...drupal8/modules/scheduler/scheduler.libraries.yml  0       0
...rupal8/modules/scheduler/scheduler.links.menu.yml  0       0
...rupal8/modules/scheduler/scheduler.links.task.yml  0       0
...uments/drupal8/modules/scheduler/scheduler.module  0       0
...upal8/modules/scheduler/scheduler.permissions.yml  0       0
...s/drupal8/modules/scheduler/scheduler.routing.yml  0       0
.../drupal8/modules/scheduler/scheduler.services.yml  0       0
...ts/drupal8/modules/scheduler/scheduler.tokens.inc  0       0
...nts/drupal8/modules/scheduler/scheduler.views.inc  0       0
...r/scheduler_handler_field_scheduler_countdown.inc  0       0
..._integration/scheduler_rules_integration.info.yml  0       2
...es_integration/scheduler_rules_integration.module  0       0
...tion/scheduler_rules_integration.rules.events.yml  0       0
...on/scheduler_rules_integration.rules_defaults.inc  0       0
...ent/ExistingNodeIsScheduledForPublishingEvent.php  0       0
...t/ExistingNodeIsScheduledForUnpublishingEvent.php  0       0
...rc/Event/NewNodeIsScheduledForPublishingEvent.php  0       0
.../Event/NewNodeIsScheduledForUnpublishingEvent.php  0       0
.../src/Event/SchedulerHasPublishedThisNodeEvent.php  0       0
...rc/Event/SchedulerHasUnpublishedThisNodeEvent.php  0       0
...Plugin/Condition/NodeIsScheduledForPublishing.php  0       0
...ugin/Condition/NodeIsScheduledForUnpublishing.php  0       0
...tion/src/Plugin/Condition/PublishingIsEnabled.php  0       0
...on/src/Plugin/Condition/UnpublishingIsEnabled.php  0       0
...integration/src/Plugin/RulesAction/PublishNow.php  0       0
...n/src/Plugin/RulesAction/RemovePublishingDate.php  0       0
...src/Plugin/RulesAction/RemoveUnpublishingDate.php  0       0
...tion/src/Plugin/RulesAction/SetPublishingDate.php  0       0
...on/src/Plugin/RulesAction/SetUnpublishingDate.php  0       0
...tegration/src/Plugin/RulesAction/UnpublishNow.php  0       0
...ules/scheduler/src/Access/ScheduledListAccess.php  0       0
...uler/src/Controller/LightweightCronController.php  0       2
...r/src/Exception/SchedulerMissingDateException.php  0       0
...xception/SchedulerNodeTypeNotEnabledException.php  0       0
...modules/scheduler/src/Form/SchedulerAdminForm.php  0       1
.../modules/scheduler/src/Form/SchedulerCronForm.php  0       1
.../FieldWidget/TimestampDatetimeNoDefaultWidget.php  0       0
...ation/Constraint/SchedulerPublishOnConstraint.php  0       0
...straint/SchedulerPublishOnConstraintValidator.php  0       0
...ion/Constraint/SchedulerUnpublishOnConstraint.php  0       0
...raint/SchedulerUnpublishOnConstraintValidator.php  0       0
...s/scheduler/src/Plugin/views/access/Scheduler.php  0       0
.../drupal8/modules/scheduler/src/SchedulerEvent.php  0       0
...drupal8/modules/scheduler/src/SchedulerEvents.php  0       0
...rupal8/modules/scheduler/src/SchedulerManager.php  0       16
...eduler_access_test/scheduler_access_test.info.yml  0       1
...heduler_access_test/scheduler_access_test.install  0       0
...cheduler_access_test/scheduler_access_test.module  0       0
..._form_display.node.scheduler_api_test.default.yml  0       0
..._view_display.node.scheduler_api_test.default.yml  0       0
....scheduler_api_test.field_approved_publishing.yml  0       0
...cheduler_api_test.field_approved_unpublishing.yml  0       0
.../field.storage.node.field_approved_publishing.yml  0       0
...ield.storage.node.field_approved_unpublishing.yml  0       0
...t/config/install/node.type.scheduler_api_test.yml  0       0
...es/scheduler_api_test/scheduler_api_test.info.yml  0       2
...les/scheduler_api_test/scheduler_api_test.install  0       0
...ules/scheduler_api_test/scheduler_api_test.module  0       0
...cheduler_api_test/scheduler_api_test.services.yml  0       0
...odules/scheduler_api_test/src/EventSubscriber.php  0       0
...sts/src/Functional/SchedulerAdminSettingsTest.php  0       0
...heduler/tests/src/Functional/SchedulerApiTest.php  0       0
...duler/tests/src/Functional/SchedulerBasicTest.php  0       0
...tests/src/Functional/SchedulerBrowserTestBase.php  0       0
...tests/src/Functional/SchedulerDefaultTimeTest.php  0       0
.../tests/src/Functional/SchedulerDeleteNodeTest.php  0       0
...sts/src/Functional/SchedulerFieldsDisplayTest.php  0       0
...s/src/Functional/SchedulerLightweightCronTest.php  0       0
...s/src/Functional/SchedulerMetaInformationTest.php  0       0
...ests/src/Functional/SchedulerMultilingualTest.php  0       0
.../tests/src/Functional/SchedulerNodeAccessTest.php  0       0
...ts/src/Functional/SchedulerNonEnabledTypeTest.php  0       0
...r/tests/src/Functional/SchedulerPastDatesTest.php  0       0
...tests/src/Functional/SchedulerPermissionsTest.php  0       0
...er/tests/src/Functional/SchedulerRequiredTest.php  0       0
...tests/src/Functional/SchedulerRevisioningTest.php  0       0
...ests/src/Functional/SchedulerRulesActionsTest.txt  0       0
...s/src/Functional/SchedulerRulesConditionsTest.txt  0       0
...tests/src/Functional/SchedulerRulesEventsTest.txt  0       0
...ional/SchedulerScheduledContentListAccessTest.php  0       0
...ests/src/Functional/SchedulerStatusReportTest.php  0       0
...ests/src/Functional/SchedulerTokenReplaceTest.php  0       0
.../tests/src/Functional/SchedulerValidationTest.php  0       0
----------------------------------------------------------------------
A TOTAL OF 0 ERRORS AND 25 WARNINGS WERE FOUND IN 94 FILES
----------------------------------------------------------------------

95 files checked with 25 warnings spread across seven files. However the 5 warnings in three .info.yml files All dependencies must be prefixed with the project name, for example "drupal:" cannot be fixed. This is a known problem with the codestyle checking.

Hence the best we can currently aim for is to fix the other 20 message. The four files in question have @TODO comments at the places where fixes are required.

morsok’s picture

morsok
Primary language French
Location Paris
commented

However the 5 warnings in three .info.yml files All dependencies must be prefixed with the project name, for example "drupal:" cannot be fixed.

Well actually yes it's fixable ! For exemple in scheduler_rules-integration.info.yml, the dependencies should list as scheduler:scheduler and rules:rules

It is so to prevent namespace collisions.

dependencies - A list of other modules your module depends on. Dependencies should be namespaced in the format: {project}:{module} where {project} is the project name as it appears in the Drupal.org url (i.e: drupal.org/project/views) and {module} is the module's machine name. Dependencies can also include version restrictions. For example, webform:webform (>=8.x-5.x).

https://www.drupal.org/docs/8/creating-custom-modules/let-drupal-8-know-...

jonathan1055’s picture

jonathan1055 commented
Status: Active » Needs review
StatusFileSize
new 2868553-4.code_style.patch1.57 KB

Thanks morsok for the tip, that's really helpful. Yes you are right, those three files can be cleaned. I cannot recall where I was reading about the dependency message not being fixable. Maybe it was an old issue and the test for namespace addition has been improved since.

Anyway, it is good that we can clean up the messages. Patch for three .info.yml files attached, just to check the testing on d.o. works the same as on my local machine before I commit the change.

  • jonathan1055 committed 69fe91b on 8.x-1.x 
    Issue #2868553 by jonathan1055, morsok: Fix .info.yml dependencies to...
jonathan1055’s picture

jonathan1055 commented
StatusFileSize
new 2868553-6.best_practice.admin_and_cron_forms.patch3.17 KB

Here are the current remaining codestyle warnings:

PHP CODE SNIFFER REPORT SUMMARY
-------------------------------------------------------------------
FILE                                               ERRORS  WARNINGS
-------------------------------------------------------------------
src/Controller/LightweightCronController.php       0       2
src/Form/SchedulerAdminForm.php                    0       1
src/Form/SchedulerCronForm.php                     0       1
src/SchedulerManager.php                           0       16
tests/src/Functional/SchedulerRulesEventsTest.php  0       4
-------------------------------------------------------------------
A TOTAL OF 0 ERRORS AND 24 WARNINGS WERE FOUND IN 95 FILES
-------------------------------------------------------------------

Patch fixes dependency injection in AdminForm and CronForm

  • jonathan1055 committed 9d48a98 on 8.x-1.x 
    Issue #2868553 by jonathan1055: Change \Drupal::l to $this->l in...

  • jonathan1055 committed 1148120 on 8.x-1.x 
    Issue #2868553 by jonathan1055: Inject SchedulerManager and change \...
mgifford’s picture

mgifford
he/him
Primary language English
commented

Thanks for working towards this! Always helps in the long run.

idebr’s picture

idebr commented
Status: Needs review » Needs work

The patch in #6 is no longer relevant. However, some remaining coding standard issues can be found in the daily test results:

The status at 25 May 2018: https://www.drupal.org/node/3292/qa

src/Controller/LightweightCronController.php
line 25	\Drupal calls should be avoided in classes, use dependency injection instead
42	\Drupal calls should be avoided in classes, use dependency injection instead
src/SchedulerManager.php
80	\Drupal calls should be avoided in classes, use dependency injection instead
92	\Drupal calls should be avoided in classes, use dependency injection instead
114	Node::loadMultiple calls should be avoided in classes, use dependency injection instead
173	t() calls should be avoided in classes, use dependency injection and $this->t() instead
183	t() calls should be avoided in classes, use dependency injection and $this->t() instead
187	\Drupal calls should be avoided in classes, use dependency injection instead
187	t() calls should be avoided in classes, use dependency injection and $this->t() instead
229	\Drupal calls should be avoided in classes, use dependency injection instead
241	\Drupal calls should be avoided in classes, use dependency injection instead
261	Node::loadMultiple calls should be avoided in classes, use dependency injection instead
322	t() calls should be avoided in classes, use dependency injection and $this->t() instead
332	t() calls should be avoided in classes, use dependency injection and $this->t() instead
336	\Drupal calls should be avoided in classes, use dependency injection instead
336	t() calls should be avoided in classes, use dependency injection and $this->t() instead
445	\Drupal calls should be avoided in classes, use dependency injection instead
445	t() calls should be avoided in classes, use dependency injection and $this->t() instead
tests/src/Functional/SchedulerRulesEventsTest.php
55	Variable $event_name is undefined.
55	Variable $description is undefined.
57	Variable $description is undefined.
64	Variable $event_name is undefined.
jonathan1055’s picture

jonathan1055 commented
Related issues: +#2876245: Incorrect "variable is undefined" when using array destructuring, for example list( ) syntax

Hi idebr,
Thanks for your comment, and your interest in coding standards and best practice.

The patch in #6 is no longer relevant.

Do you mean it no longer applies? That is because it has already been committed, see #7 and #8

The warnings you list are actually the ones I showed in the summary in #6 

2 in src/Controller/LightweightCronController.php
16 in src/SchedulerManager.php
4 in tests/src/Functional/SchedulerRulesEventsTest.php

The four "variable is undefined" messages is actually a false alert. The variables are defined using the list() function, but the code style checker is not programmed to recognise this, so thinks they are undefined. I have already raised #2876245: Incorrect "variable is undefined" when using array destructuring, for example list( ) syntax with no response yet.

Jonathan

idebr’s picture

idebr commented
Status: Needs work » Needs review
StatusFileSize
new scheduler-coding_standards-2868553-12.patch20.76 KB

#11 Yes, I meant the patch is no longer relevant since it has already been committed. The workflow in this issue is somewhat different than the typical workflow in Drupal core, where a patch file fixes the issue completely and the issue is then marked as 'Fixed' when it is committed.

To fix the remaining failures, I have updated the LightweightCronController and SchedulerManager to follow the Drupal coding standards. Since the SchedulerRulesEventsTest generates false positives, I have added @codingStandardsIgnoreStart and @codingStandardsIgnoreEnd to bypass this validation.

jonathan1055’s picture

jonathan1055 commented

You have done a great deal of work here, thank you very much.
The test result does not show any style or practice messages, so it looks like you have corrected them all in these changes. However, I thought we were getting "N more than branch" or "N fewer than branch" notices. and it would have been good to get "22 fewer" or at least something to prove that the codebase is clean.

It will take me a while to review the changes, before committing, but thanks again for your work on this. Much appreciated.

Jonathan

thalles’s picture

thalles
Primary language Portuguese, Brazil
Location Teófilo Otoni - MG
commented

Looks Good!

keenegan’s picture

keenegan
Primary language French
commented
StatusFileSize
new scheduler-coding_standards-2868553-15.patch21.28 KB
new interdiff_12_15.txt908 bytes

Looks good to me too.
Even if this doesn't appear in the phpcs, there is still unused use statements in some files, so maybe we should fix this aswell ?

jonathan1055’s picture

jonathan1055 commented

Thanks Kng. Did you have an automatic way to detect these unrequired use statements? I am sure I have seen something like that before, but cannot recall where it was. If you can give a screen shot, please share.

keenegan’s picture

keenegan
Primary language French
commented

I use phpstorm, in the menu "Code / Inspect code" and it gives you a bunch of informations about your folder (code smells, erros, unused statements or variables, etc..)

idebr’s picture

idebr commented
Status: Needs review » Needs work

There are a few minor code style issues that are not picked up by phpcs:

  1. \Drupal\scheduler\Form\SchedulerCronForm references @var Drupal\scheduler\SchedulerManager without a \ prefix, so PHPStorm cannot find the reference.
  2. \Drupal\scheduler\Form\SchedulerCronForm::__construct is missing a call to parent::__construct().

When these issues are fixed, this issue should be ready to be committed.

bzhazreal’s picture

bzhazreal commented
Status: Needs work » Needs review
StatusFileSize
new interdiff_15-19.txt1.81 KB
new scheduler-coding_standards-2868553-19.patch23.16 KB

Fix two last errors.

jonathan1055’s picture

jonathan1055 commented
StatusFileSize
new 2868553_20.scheduler_best_practice.patch16.53 KB

Thanks idebr for your helpful reviews, much appreciated. Also thanks Kng and bzhazreal for your patches.

This issue has had quite a bit of scope-creep, and the patch in #19 alters 10 files. Whilst all of these changes are good, and won't be ignored, it is only actually 4 files that need to be changed to remove the phpcs best practice warnings. I think it would be better to do the minimum number of changes in the first place, so that we know exactly what has been done for this issue. Then we can do further tidy-up, either here or on #3033108: Miscellaneous changes before Scheduler 8.x-1.1 but I would like to keep the commits separate.

Here's a patch with just the minimum four files fixed.

  • jonathan1055 committed bbeb611 on 8.x-1.x 
    Issue #2868553 by jonathan1055, Kng, bzhazreal, idebr, morsok: Fix the...
idebr’s picture

idebr commented
Status: Needs review » Fixed

Alright, let's consider this fixed then.

jonathan1055’s picture

jonathan1055 commented
Related issues: +#3033783: Remove unused statements, use EntityTypeManager, fix annotations and other tidy-up

@idebr, @Kng, @thalles - I would value your opinions on the extra changes that I did not commit above - please see the follow-up issue #3033783: Remove unused statements, use EntityTypeManager, fix annotations and other tidy-up

Thanks all for helping to clean-up the Scheduler codebase. It is great not to have any PHPCS warnings. We still have work to do as tests are failing on Travis due to deprecation warnings not being suppressed like they are on d.o. but those are for separate issues. For now lets be pleased that PHPCS gives a clean slate.

Status: Fixed » Closed (fixed)

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