PHPCS is run even when it cannot check any files

The phpcs job has:

  rules:
    - *skip-phpcs-rule
    - if: $_PHPCS_ALLOW_FAILURE == "1" || ($_ALL_VALIDATE_ALLOW_FAILURE == "1" && $_PHPCS_ALLOW_FAILURE != "0")
      allow_failure: true
    - if: $_PHPCS_ALLOW_FAILURE == "0" || $_ALL_VALIDATE_ALLOW_FAILURE == "0"
      allow_failure: false
    - when: on_success

There is no checking at all for what file types exist, so that is why the GTD main branch runs the jobs but checks zero files.

The default assets/phpcs.xml checks file extensions of php,inc,module,install,info,test,profile,theme

We currently have a rule .php-files-exist which checks for:

  exists:
    - '*.{$DRUPAL_PHP_FILE_TYPES}'
    - '**/*.{$DRUPAL_PHP_FILE_TYPES}'

where $DRUPAL_PHP_FILE_TYPES is defined as php,module,install,inc,profile,theme,engine

These are nearly the same set. phpcs.xml has .info and .test which are not in $DRUPAL_PHP_FILE_TYPES. I think these are exclusively Drupal 7 filetype extensions, not used in Drupal 8+ ?

However $DRUPAL_PHP_FILE_TYPES has .engine which is not in the phpcs.xml list.

We could use .php-files-exist in the phpcs job definition for the main pipeline, and if we wanted to backport to main-d7 we could also add .info and .test but that's not a problem.

Yeah, that sounds like a good way forward. We can/should unify the extentions being checked for D8+ and add the extra ones for D7.

I used _PHPCS_EXTRA: -v and verified that the Coding Standards phpcs job is checking 0 files, just like our GTD branch

We can/should unify the extentions being checked

So is there any harm in adding 'info' and 'test' into $DRUPAL_PHP_FILE_TYPES and also adding 'engine' into the assets/phpcs.xml file? Then $DRUPAL_PHP_FILE_TYPES will work in D8+ and also in the D7 job. Or should the Drupal7 specifc extensions be added via hardcoding an extra line in the d7 phpcs job rule? If D8+ does not have any files ending .info or .test then I can't see any problem adding them into the list.

Maybe I will just start the work and see how it looks, the you can make a judgement when you can see the chnages :-)

Yeah, agree. We can have them all under the same variable.

First commit just adds _PHPCS_EXTRA: '-v' so we can see which filetypes are currently being checked with no rules:

d7-composer has .info, .module and .test
d10-plus checks .module and .php
d10-theme checks .theme
d10-profile checks .install

We do not have any project with a .engine file so I will add one to the d10-theme branch. We have no .profile so I can add one in the d10-profile branch. We have no .inc so I can add that too.

But to add another angle to this, Core 11.1 phpcs.xml.dist has
arg name="extensions" value="engine,inc,install,module,php,profile,test,theme,yml"
This adds .yml into the list, and we should do the same. Coder does do sniffs on .yml files, for example #2841649: Add sniff for _access: 'TRUE' in Drupal 8 routing.yml →

I have updated the issue summary to reflect the full list of extensions we are dealing with.

I have added the other file types to the GTD branches and re-run the MR pipeline. We now see that:
d10-plus has .module, .php and .inc
d10-theme still only checks .theme, because our default assets/phpcs.xml does not (yet) contain .engine
d10-profile checks .install and .profile

Now adding the two missing extensions engine and yml into the assets/phpcs.xml we get
d7-composer has .info, .module and .test (no change)
d10-plus has .module, .php, .inc and .yml
d10-theme has .theme, and now .engine and .yml
d10-profile has .install, .profile and .yml

(the pipeline failure was unrelated - Nightwatch on Previous major. Re-run and it passed)

With the updated GTD documentation files (see 📌 Add documentation .md files to test the pages job Active ) I have added 'main' to the list of downstream branches that we can test. Here's the first run
https://git.drupalcode.org/issue/gitlab_templates_downstream-3515276/-/p...
We see that the phpcs job has been added and runs. Ironically it does actually have a file to test against, because we have added .yml to the default list in assets/phpcs.xml. mkdocs.yml is the only file checked.

Doing that addition in phpcs.xml is correct, but I think we should not add yml to the list of extensions in $DRUPAL_PHP_FILE_TYPES because this should not be used to determine if the phpcs job gets added to the pipeline. It is correct that if other php files exist then the .yml files will also get checked in the job.

Now adding the php-files-exist condition into the job rules

d7-composer still checks .info, .module and .test
d10-plus still checks .module, .php, .inc and .yml
d10-theme still checls .them, .engine and .yml
d10-profile still checks install, .profile and .yml
But main does not run the phpcs job at all, which is what this issue is all about.

This is ready for review.

It looks good to me. Thanks for the testing in all those cases.

RTBC pending the revert mentioned in the MR.

I've removed the temporary reference to MR18 in the "GTD Main" pipeline. Ready for merging.
Also updated the issue summary to explain why 'yml' is in the extensions list but not the rule for determining if the job is run.

Just noticed that we need to update https://project.pages.drupalcode.org/gitlab_templates/jobs/phpcs

Docs done. RTBC (unless you would like the wording altered from this commit)

Merged. Thanks for noticing the documentation page!

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