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

Step 1: Preparation

Open the file core/phpcs.xml.dist and add a line for the sniff of this ticket. The sniff name is in the issue title. Make sure your patch will include the addition of this line.

Step 2: Install & configure PHPCS

Install PHP CodeSniffer and the ruleset from the Coder module:

$ composer install
$ ./vendor/bin/phpcs --config-set installed_paths ../../drupal/coder/coder_sniffer

Once you have installed the phpcs package, you can list all the sniffs available to you like this:

$ ./vendor/bin/phpcs --standard=Drupal -e

This will give you a big list of sniffs, and the Drupal-based ones should be present.

Step 3: Prepare the phpcs.xml file

To speed up the testing you should make a copy of the file phpcs.xml.dist (in the core/ folder) and save it as phpcs.xml. This is the configuration file for PHP CodeSniffer.

We only want this phpcs.xml file to specify the sniff we're interested in. So we need to remove all the rule items, and add only our own sniff's rule. Rule items look like this:

<rule ref="Drupal.Classes.UnusedUseStatement"/>

Remove all of them, and add only the sniff from this issue title. This will make sure that our tests run quickly, and are not going to contain any output from unrelated sniffs.

Step 4: Run the test

Now you are ready to run the test! From within the core/ folder, run the following command to launch the test:

$ cd core/
$ ../vendor/bin/phpcs -p

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.

Step 5: 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, but thankfully phpcbf can fix many of them. You can call phpcbf like this:

$ ../vendor/bin/phpcbf

This will fix the errors in place. 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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mfernea created an issue. See original summary.

iainp999’s picture

I had a quick look at this, and a small number of issues (14) can be fixed with phpcbf. However, there are others that possibly require the input of someone closer to that code. For example :-

FILE: ...drupal/core/modules/views/src/ViewExecutable.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 5 WARNINGS AFFECTING 5 LINES
----------------------------------------------------------------------
1005 | WARNING | Method name "_preQuery" should not be prefixed with an
| | underscore to indicate visibility
| | (Drupal.Methods.MethodDeclaration.Underscore)
1020 | WARNING | Method name "_postExecute" should not be prefixed with
| | an underscore to indicate visibility
| | (Drupal.Methods.MethodDeclaration.Underscore)
1038 | WARNING | Method name "_initHandler" should not be prefixed with
| | an underscore to indicate visibility
| | (Drupal.Methods.MethodDeclaration.Underscore)
1059 | WARNING | Method name "_buildArguments" should not be prefixed
| | with an underscore to indicate visibility
| | (Drupal.Methods.MethodDeclaration.Underscore)
1344 | WARNING | Method name "_build" should not be prefixed with an
| | underscore to indicate visibility
| | (Drupal.Methods.MethodDeclaration.Underscore)
----------------------------------------------------------------------

iainp999’s picture

For the moment, I've attached a patch that exclude the sniff for methods beginning with underscores as a starting point.

mfernea’s picture

Title: Fix 'Drupal.Methods.MethodDeclaration' coding standard » Fix 'Drupal.Methods.MethodDeclaration' coding standard (AbstractAfterVisibility, FinalAfterVisibility, StaticBeforeVisibility)
Status: Active » Needs work

The patch looks good. I'm not sure we'll be able to fix Drupal.Methods.MethodDeclaration.Underscore as it is breaking backwards compatibility. I opened a new issue for this #2906262: Fix 'Drupal.Methods.MethodDeclaration.Underscore' coding standard and let's focus on this issue on the rest of the sniffs from Drupal.Methods.MethodDeclaration. I changed the title accordingly. Please add in phpcs.xml.dist which sniffs are we targeting:

    <!-- Sniff for: AbstractAfterVisibility, FinalAfterVisibility,
      StaticBeforeVisibility -->

If that is done I would mark this as RTBC.

mfernea’s picture

Title: Fix 'Drupal.Methods.MethodDeclaration' coding standard (AbstractAfterVisibility, FinalAfterVisibility, StaticBeforeVisibility) » Fix 'Drupal.Methods.MethodDeclaration' coding standard

Actually, as per my comment in #2906262: Fix 'Drupal.Methods.MethodDeclaration.Underscore' coding standard, Drupal.Methods.MethodDeclaration.Underscore is not part of Drupal CS because of these lines in ruleset.xml:

 <!-- Silence method name underscore warning which is covered already in
   Drupal.NamingConventions.ValidFunctionName.ScopeNotCamelCaps. -->
 <rule ref="Drupal.Methods.MethodDeclaration.Underscore">
  <severity>0</severity>
 </rule>

So, for modifications to phpcs.xml.dist I would go for something like:

<rule ref="../vendor/drupal/coder/coder_sniffer/Drupal/Sniffs/Methods/MethodDeclarationSniff.php">
  <!-- Silence method name underscore warning which is covered already in
    Drupal.NamingConventions.ValidFunctionName.ScopeNotCamelCaps. -->
  <exclude name="Drupal.Methods.MethodDeclaration.Underscore"/>
</rule>
iainp999’s picture

thanks @mfernea

updated patch is attached

mfernea’s picture

When a patch is uploaded, the status should be changed to 'Needs review'. This also triggers the tests to run.
When adjusting an existing patch, it's best to upload an interdiff too (https://www.drupal.org/documentation/git/interdiff). Not really needed in this case, being a small patch, but it's easier to review.

Regarding to phpcs.xml.dist modifications, I would only add the code from #5. The "Sniff for" comment is not needed anymore. Also, please note the ref value which is different than the one in the patch.

Thanks for the help!

iainp999’s picture

np @mfernea, thanks for the feedback.

patch and interdiff attached!

iainp999’s picture

Status: Needs work » Needs review
mfernea’s picture

Status: Needs review » Reviewed & tested by the community

Thanks for the work done! ;)

RTBC as:

  • patch applies cleanly
  • modifications look good
  • we are excluding Drupal.Methods.MethodDeclaration.Underscore as Drupal CS does
  • no failed tests
  • no cs issues
iainp999’s picture

thanks @mfernea :)

  • catch committed 4207b30 on 8.5.x
    Issue #2901727 by iainp999, mfernea: Fix 'Drupal.Methods....

catch credited catch.

catch’s picture

Version: 8.5.x-dev » 8.4.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.5.x and cherry-picked to 8.4.x. Thanks!

  • catch committed 5c6342e on 8.4.x
    Issue #2901727 by iainp999, mfernea: Fix 'Drupal.Methods....

Status: Fixed » Closed (fixed)

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