Problem/Motivation

The testbot does not run PHPCS on all files when core/phpcs.xml.dist is changed. Therefor the testbot passes, when there are files that break on the rule change and the testbot should fail.

Proposed resolution

Change the cor/scripts/dev/commit-code-check.sh so that PHPCS will check all files when core/phpcs.xml.dist is changed.

Remaining tasks

User interface changes

None

API changes

None

Data model changes

None

Release notes snippet

None

CommentFileSizeAuthor
#8 3224583-7.patch2.04 KBdaffie
#8 3224583-7-with-changed-phpcs-xml-dist.patch2.72 KBdaffie
#8 interdiff-3224583-2-7.txt539 bytesdaffie
#6 3224583-6.patch2 KBdhirendra.mishra
#6 interdiff_2-6.txt632 bytesdhirendra.mishra
#2 3224583-2-with-changed-phpcs-xml-dist.patch2.67 KBdaffie
#2 3224583-2-with-only-changed-phpcs-xml-dist.patch700 bytesdaffie
#2 3224583-2.patch1.99 KBdaffie
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

daffie created an issue. See original summary.

daffie’s picture

daffie CreditAttribution: daffie commented
Status: Active » Needs review
FileSize
3224583-2.patch1.99 KB
3224583-2-with-only-changed-phpcs-xml-dist.patch700 bytes
3224583-2-with-changed-phpcs-xml-dist.patch2.67 KB

Added 3 patches:

  1. The first is the patch to commit with only the change to core/scripts/dev/commit-code-check.sh
  2. The second file has only the change to the file core/phpcs.xml.dist. The testbot should fail on it but it does not.
  3. The third fiel has the changes to both files and run the PHPCS command on all files and the testbot should fail.
daffie’s picture

daffie CreditAttribution: daffie commented
Related issues: +#2937514: Fix Drupal.NamingConventions.ValidVariableName.LowerCamelName in tests

The changed core/phpcs.xml.dist comes from the issue: #2937514: Fix Drupal.NamingConventions.ValidVariableName.LowerCamelName in tests.

daffie’s picture

daffie CreditAttribution: daffie commented
Status: Needs review » Needs work

The patch 3224583-2-with-changed-phpcs-xml-dist.patch should fail the testbot, only it does not. On my local machine it does fail however. No idea why it does not fail the testbot.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented 
  1. +++ b/core/scripts/dev/commit-code-check.sh
@@ -163,6 +170,19 @@
+    STATUS=1

    I think you need to set FINAL_STATUS=1 here... 

  2. +++ b/core/scripts/dev/commit-code-check.sh
@@ -163,6 +170,19 @@
 for FILE in $FILES; do
   STATUS=0;

    ...because STATUS is overwritten on every trip around the file loop.

dhirendra.mishra’s picture

dhirendra.mishra CreditAttribution: dhirendra.mishra at Valuebound for Valuebound commented
Status: Needs work » Needs review
FileSize
interdiff_2-6.txt632 bytes
3224583-6.patch2 KB

Here uploading my patch and inter-diff for above comment....Please review it.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Needs review » Needs work

Please can you also upload the same change applied to 3224583-2-with-changed-phpcs-xml-dist.patch - which should fail.

daffie’s picture

daffie CreditAttribution: daffie commented
Status: Needs work » Needs review
FileSize
interdiff-3224583-2-7.txt539 bytes
3224583-7-with-changed-phpcs-xml-dist.patch2.72 KB
3224583-7.patch2.04 KB

@dhirendramishra: Sorry for xposting. Thank you for helping with this issue.

@longwave: Thank you for your help.

daffie’s picture

daffie CreditAttribution: daffie commented

The patch with a changed phpcs.xml.dist fails as expected. It even has failure that is not related to the change in the phpcs.xml.dist:

FILE: /var/www/html/core/modules/tour/src/TourViewBuilder.php
----------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
----------------------------------------------------------------------
 74 | ERROR | Comment indentation error, expected only 1 spaces
    |       | (Drupal.Commenting.InlineComment.SpacingBefore)
 78 | ERROR | Comment indentation error, expected only 1 spaces
    |       | (Drupal.Commenting.InlineComment.SpacingBefore)
----------------------------------------------------------------------
longwave’s picture

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

Thanks @daffie! I guess we should have another issue to deal with that problem in TourViewBuilder, I wonder how that slipped in.

Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje for Millor commented
Related issues: +#3224861: PHPCS failure in /core/modules/tour/src/TourViewBuilder.php

Issue for the indentation issue created here

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Version: 9.3.x-dev » 9.2.x-dev
Category: Bug report » Task
Priority: Critical » Normal
Status: Reviewed & tested by the community » Fixed
Issue tags: +Needs followup

This is not a bug. Coding standard issues are not bug fixes per se and neither is this a critical. That said this is nice functionality to have and it looks good. We should file follow-ups to do the same for JS and CSS files.

Committed and pushed 92a87eacc8 to 9.3.x and d64dc5c13c to 9.2.x. Thanks!

  • alexpott committed 92a87ea on 9.3.x 
    Issue #3224583 by daffie, dhirendra.mishra, longwave: The testbot does...

  • alexpott committed d64dc5c on 9.2.x 
    Issue #3224583 by daffie, dhirendra.mishra, longwave: The testbot does...

Status: Fixed » Closed (fixed)

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

quietone’s picture

quietone CreditAttribution: quietone commented
Issue tags: -Needs followup

This is tagged for followups, #12, so checking to see if that has happened.

It has, the work was done in

I am removing the tag.