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

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.Commenting.DocComment.ShortFullStop"/>

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.

CommentFileSizeAuthor
#4 3183673-4.interdiff-3-4.txt3.33 KBjonathan1055
#4 3183673-4.fix-shortfullstop.patch103.75 KBjonathan1055
#3 3183673-3.fix-shortfullstop.patch69.85 KBjonathan1055
#2 3183673-2.fix-shortfullstop.patch2.62 KBjonathan1055

Comments

jonathan1055 created an issue. See original summary.

jonathan1055’s picture

jonathan1055 commented
Status: Active » Needs review
StatusFileSize
new 3183673-2.fix-shortfullstop.patch2.62 KB

Following on from comments #4, #9-11 in #2572635: Fix 'Drupal.Commenting.DocComment.LongFullStop' coding standard I have started work on improving the Coder auto-fixer for ShortFullStop, to avoid the incorrect changes and leave them to be manually corrected - see #3184314: Improve the auto-fixing for DocComment.ShortFullStop coding standard

This patch uses the modified Coder sniff to run against core 9.2 and show how many faults we can fix automatically and how many require manual changes. I have removed the phpunit call to speed up the test return and not waste resources in all these intermediate patch tests.

jonathan1055’s picture

jonathan1055 commented
StatusFileSize
new 3183673-3.fix-shortfullstop.patch69.85 KB

That's good. The output from drupal.org testing matches my local modifications to Coder, and we now get

----------------------------------------------------------------
A TOTAL OF 251 ERRORS AND 0 WARNINGS WERE FOUND IN 193 FILES
----------------------------------------------------------------
PHPCBF CAN FIX 211 OF THESE SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------

Here is the patch for those 211 automatic fixes, which also matches the codesniffer patch produced by drupal.org testbot in the #2 test run

I have only spotted one change which needs manually improving, because the original was only half a sentence:

--- a/core/modules/contextual/tests/src/Kernel/ContextualUnitTest.php
+++ b/core/modules/contextual/tests/src/Kernel/ContextualUnitTest.php
@@ -20,7 +20,7 @@ class ContextualUnitTest extends KernelTestBase {
   protected static $modules = ['contextual'];
 
   /**
-   * Provides testcases for testContextualLinksToId() and
+   * Provides testcases for testContextualLinksToId() and.
    */
   public function _contextual_links_id_testcases() {

This can be fixed manually, with the others.

The above patch will leave just 40 coding standards warnings which should be simple to fix when viewing them manually, but are not worth trying to make the Coder sniff work out how to, as it would lead to assumptions which would invariably create incorrect fixes.

jonathan1055’s picture

jonathan1055 commented
StatusFileSize
new 3183673-4.fix-shortfullstop.patch103.75 KB
new 3183673-4.interdiff-3-4.txt3.33 KB

As expected, the result of #3 shows that 211 warnings are fixed and just 40 remain. So here is the actual patch to commit to fix those 211 warnings.

This does not have the change to drupalci.yml so all phpunit tests will be run. It also does not have the phpcs.xml.dist change becuase there are still 40 warnings to fix. But those 40 will be easier to correct and review when the 211 are not continually included in the patch.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Reviewed & tested by the community

Reviewed with git diff --color-words. All cases just have a full stop appended with the exception of ContextualUnitTest where the comment didn't make any sense. We could do some further improvements to the comments in a number of cases, but this is out of scope in a coding standards issue. Therefore this is RTBC.

Opened #3185183: Refactor ContextualUnitTest to use a data provider to improve ContextualUnitTest further.

catch’s picture

catch
he/him
Primary language English
commented

It looks like we need a follow-up still to fix the remaining 40 failures and add the rule to phpcs.xml.dist - or am I missing something?

jonathan1055’s picture

jonathan1055 commented

@catch I was hoping that these 211 fixes could be committed here as an intermediate commit (without the rule being added obviosuly), then I would continue on this same issue to fix the 40 manual ones and add the rule. That would then be a smaller easier to review patch.

catch’s picture

catch
he/him
Primary language English
commented

@jonathan1055 apart from paper-bag commits (and sometimes even then) we usually try to keep a 1-1 commit to issue ratio - so it'd be better to complete the work here in a follow-up issue - for example it means no-one new to the issue has to read past patches and discussions about changes already committed.

For a fairly simple change it does not really matter, but if for example we all forget about this issue for the next four years after the interim commit (which happens all the time in the core queue), then no-one would get issue credit for the changes made so far, and it's more confusing for the person that rediscovers it to see what was done vs. what is left.

jonathan1055’s picture

jonathan1055 commented
Title: Fix 'Drupal.Commenting.DocComment.ShortFullStop' coding standard » Fix 'Drupal.Commenting.DocComment.ShortFullStop' coding standard - part 1 auto-fixes

Thanks @catch for taking the time to explain the reasons. Yes I fully see the point now. In that case this is RTBC from Longwave in #5 and I'm happy that this is committed.

I have raised #3185652: Fix 'Drupal.Commenting.DocComment.ShortFullStop' coding standard - part 2 as the follow-up for the manual fixes.

  • catch committed 7c5daac on 9.1.x 
    Issue #3183673 by jonathan1055, longwave: Fix 'Drupal.Commenting....
catch’s picture

catch
he/him
Primary language English
commented
Version: 9.2.x-dev » 9.1.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 9.2.x and backported to 9.1.x - had to remove one hunk for GoutteDriverTest which doesn't exist in 9.1.x. Thanks for opening the follow-up.

  • catch committed 86c7ed0 on 9.2.x 
    Issue #3183673 by jonathan1055, longwave: Fix 'Drupal.Commenting....
jonathan1055’s picture

jonathan1055 commented

#3185652: Fix 'Drupal.Commenting.DocComment.ShortFullStop' coding standard - part 2 is now ready for someone else to review. No rush, just letting you know that I have done the preliminary work, shown the warnings, then added my corrections. Ready for other eyes to see now.

Status: Fixed » Closed (fixed)

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