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

Comments

daffie created an issue. See original summary.

daffie’s picture

daffie commented
Status: Active » Needs review
StatusFileSize
new 3224583-2.patch1.99 KB
new 3224583-2-with-only-changed-phpcs-xml-dist.patch700 bytes
new 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 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 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
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 commented
Status: Needs work » Needs review
StatusFileSize
new interdiff_2-6.txt632 bytes
new 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
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 commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-3224583-2-7.txt539 bytes
new 3224583-7-with-changed-phpcs-xml-dist.patch2.72 KB
new 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 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
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
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 🇪🇺🌍
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 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.