Part of #2571965: [meta] Fix PHP coding standards in core.

Approach

We are testing coding standards with PHP CodeSniffer, using the Drupal coding standards from the Coder module. Both of these packages are not installed in Drupal core. We need to do a couple of steps in order to download and configure them so we can run a coding standards check.

How to run tests?

Clone changes from branch 3123069-11.x-useless-overriding-method.
Run the following command from Drupal root to launch the test:

$ phpcs --standard=core/phpcs.xml.dist -- '--report-full' '--report-summary' -p core

This takes a couple of minutes. The -p flag shows the progress, so you have a bunch of nice dots to look at while it is running.

Fix the failures

When the test is complete it will present you a list of all the files that contain violations of your sniff, and the line numbers where the violations occur. You could fix all of these manually.
You can then make a diff of the changes using git. You can also re-run the test with phpcs and determine if that fixed all of them.

Current failures on 11.x

PHP CODE SNIFFER REPORT SUMMARY
---------------------------------------------------------------------------------------------------------------------------------
FILE                                                                                                             ERRORS  WARNINGS
---------------------------------------------------------------------------------------------------------------------------------
core/lib/Drupal/Core/Action/ActionPluginCollection.php                                             0       1
core/lib/Drupal/Core/Condition/ConditionPluginCollection.php                                       0       1
core/lib/Drupal/Core/Config/InstallStorage.php                                                     0       1
core/lib/Drupal/Core/Config/Entity/Query/Query.php                                                 0       1
core/lib/Drupal/Core/Installer/InstallerKernel.php                                                 0       1
core/lib/Drupal/Core/KeyValueStore/DatabaseStorageExpirable.php                                    0       2
core/lib/Drupal/Core/Layout/LayoutDefault.php                                                      0       1
core/lib/Drupal/Core/Queue/QueueWorkerManager.php                                                  0       1
core/lib/Drupal/Core/Render/ElementInfoManager.php                                                 0       1
core/lib/Drupal/Core/Test/TestRunnerKernel.php                                                     0       1
core/lib/Drupal/Core/Validation/Plugin/Validation/Constraint/EmailValidator.php                    0       1
core/modules/block/src/BlockPluginCollection.php                                                   0       1
core/modules/ckeditor5/tests/src/FunctionalJavascript/CKEditorStylesheetsWarningTest.php           0       1
core/modules/comment/tests/src/Functional/Views/CommentApproveLinkTest.php                         0       1
core/modules/config/tests/src/Functional/ConfigLanguageOverrideWebTest.php                         0       1
core/modules/config_translation/src/Form/ConfigTranslationAddForm.php                              0       1
core/modules/filter/src/FilterFormatAddForm.php                                                    0       1
core/modules/filter/src/FilterPluginCollection.php                                                 0       1
core/modules/forum/tests/src/Functional/Views/ForumIntegrationTest.php                             0       1
core/modules/image/src/ImageEffectPluginCollection.php                                             0       1
core/modules/jsonapi/tests/src/Functional/MediaTest.php                                            0       1
core/modules/locale/tests/src/Functional/LocaleConfigTranslationImportTest.php                     0       1
core/modules/migrate/tests/src/Kernel/Plugin/LogTest.php                                           0       1
core/modules/migrate/tests/src/Unit/SqlBaseTest.php                                                0       1
core/modules/migrate/tests/src/Unit/TestMigrateExecutable.php                                      0       1
core/modules/migrate/tests/src/Unit/TestSqlIdMap.php                                               0       3
core/modules/migrate/tests/src/Unit/destination/EntityRevisionTest.php                             0       2
core/modules/migrate/tests/src/Unit/process/SubstrTest.php                                         0       1
core/modules/migrate_drupal/tests/modules/field_discovery_test/src/FieldDiscoveryTestClass.php     0       6
core/modules/node/src/Entity/Node.php                                                              0       1
core/modules/node/tests/src/Functional/Views/NodeTestBase.php                                      0       1
core/modules/options/tests/src/Kernel/OptionsFormattersTest.php                                    0       1
core/modules/phpass/tests/src/Unit/PasswordVerifyTest.php                                          0       1
core/modules/responsive_image/src/Entity/ResponsiveImageStyle.php                                  0       1
core/modules/search/src/Plugin/SearchPluginCollection.php                                          0       1
core/modules/serialization/tests/src/Unit/Normalizer/DateTimeIso8601NormalizerTest.php             0       1
core/modules/serialization/tests/src/Unit/Normalizer/DateTimeNormalizerTest.php                    0       1
core/modules/serialization/tests/src/Unit/Normalizer/TimestampNormalizerTest.php                   0       1
core/modules/system/src/Plugin/ImageToolkit/Operation/gd/GDImageToolkitOperationBase.php           0       1
core/modules/system/tests/modules/entity_test/src/Entity/EntityTest.php                            0       1
core/modules/system/tests/modules/entity_test/src/Entity/EntityTestMul.php                         0       1
core/modules/system/tests/src/Functional/Datetime/DrupalDateTimeTest.php                           0       1
core/modules/system/tests/src/Functional/Render/UrlBubbleableMetadataBubblingTest.php              0       1
core/modules/system/tests/src/Functional/System/IndexPhpTest.php                                   0       1
core/modules/system/tests/src/Kernel/Scripts/DbCommandBaseTest.php                                 0       1
core/modules/taxonomy/tests/src/Functional/Views/TaxonomyParentUITest.php                          0       1
core/modules/tour/src/TipsPluginCollection.php                                                     0       1
core/modules/user/tests/src/Functional/AccessRoleUITest.php                                        0       1
core/modules/user/tests/src/Functional/Views/AccessRoleTest.php                                    0       1
core/modules/views/src/DisplayPluginCollection.php                                                 0       1
core/modules/views/tests/src/Kernel/Plugin/PluginBaseTest.php                                      0       1
core/tests/Drupal/KernelTests/Core/Datetime/DatelistElementFormTest.php                            0       1
core/tests/Drupal/KernelTests/Core/Datetime/DatetimeElementFormTest.php                            0       1
core/tests/Drupal/KernelTests/Core/Routing/RouteProviderTest.php                                   0       1
core/tests/Drupal/Tests/Component/Utility/ArgumentsResolverTest.php                                0       1
core/tests/Drupal/Tests/Core/Entity/Routing/DefaultHtmlRouteProviderTest.php                       0       5
core/tests/Drupal/Tests/Core/Entity/Sql/DefaultTableMappingTest.php                                0       2
core/tests/Drupal/Tests/Core/Entity/Sql/SqlContentEntityStorageSchemaTest.php                      0       2
core/tests/Drupal/Tests/Core/StackMiddleware/NegotiationMiddlewareTest.php                         0       1
---------------------------------------------------------------------------------------------------------------------------------
A TOTAL OF 0 ERRORS AND 74 WARNINGS WERE FOUND IN 59 FILES
---------------------------------------------------------------------------------------------------------------------------------

Issue fork drupal-3123069

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jungle created an issue. See original summary.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Title: Fix 'Generic.CodeAnalysis.UselessOverridingMethodp' coding standard » Fix 'Generic.CodeAnalysis.UselessOverridingMethod' coding standard

This one triggers on a whole bunch of code like

  /**
   * {@inheritdoc}
   *
   * @return \Drupal\Core\Queue\QueueWorkerInterface
   */
  public function createInstance($plugin_id, array $configuration = []) {
    return parent::createInstance($plugin_id, $configuration);
  }

which is technically a useless override, except that we are providing a better typehint for the return value.

Should we duplicate this sniff in Coder and make it ignore the case where the return value is of a different type to the parent?

jungle’s picture

jungle
He/Him
Location Chongqing, China
CreditAttribution: jungle as a volunteer commented

Should we duplicate this sniff in Coder and make it ignore the case where the return value is of a different type to the parent?

Re #2, From my understanding, If the method in parent class/interface has a return value typehint, the corresponding method with the same signature in child class should not declare a different return value typehint. So personally, I vote for No.

Version: 9.0.x-dev » 9.1.x-dev

Drupal 9.0.10 was released on December 3, 2020 and is the final full bugfix release for the Drupal 9.0.x series. Drupal 9.0.x will not receive any further development aside from security fixes. Sites should update to Drupal 9.1.0 to continue receiving regular bugfixes.

Drupal-9-only bug reports should be targeted for the 9.1.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.2.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Issue tags: +Coding standards

Version: 9.1.x-dev » 9.3.x-dev

Drupal 9.1.10 (June 4, 2021) and Drupal 9.2.10 (November 24, 2021) were the last bugfix releases of those minor version series. Drupal 9 bug reports should be targeted for the 9.3.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.15 was released on June 1st, 2022 and is the final full bugfix release for the Drupal 9.3.x series. Drupal 9.3.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.4.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

dineshkumarbollu made their first commit to this issue’s fork.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.9 was released on December 7, 2022 and is the final full bugfix release for the Drupal 9.4.x series. Drupal 9.4.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.5.x-dev branch from now on, and new development or disruptive changes should be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

zniki.ru made their first commit to this issue’s fork.

zniki.ru opened merge request !5446

Nikolay Shapovalov’s picture

Nikolay Shapovalov
Primary language Russian
CreditAttribution: Nikolay Shapovalov as a volunteer commented
Version: 9.5.x-dev » 11.x-dev
Issue summary: View changes
Status: Active » Needs review

I wasn't able to find any commits in the branch 3123069-fix-generic.codeanalysis.uselessoverridingmethod-coding.
Compare to 9.4.x
And I wasn't able to merge changes to 11.x, so I decide to create a new branch.

I create draft MR with some changes, and I want to get some feedback about approach.
I update issue description and change target branch for this issue.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented

I still think we need to decide what to do about #2, where the updated return type does add value for people using IDEs and perhaps static analysis too. Just removing them all outright seems like we are losing information.

Ideally we would just add return types everywhere anyway, and it is valid to return a more specific type in a subclass because of covariance, but we can't do that without breaking backward compatibility where contrib or custom code might have already extended without adding a type.

smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented
Status: Needs review » Needs work

For #13