Add basepath parameter to PHP Code Sniffer

I just did a small suggestion/question. I think it might be correct as it is now but I'm not sure if the folder name will always match the CI variable.
If we can confidently say yes, then it's RTBC, but if we are not 100% sure then the suggestion I proposed might be enough.

Have a look and let me know if that would still be ok.

Related:

/builds/project/MODULENAME/web/modules/custom/MODULENAME/expand_composer_json.php

is also scanned, and it's part of the CI infra. That's wrong.

That is weird, that file has // phpcs:ignoreFile.

We can work on this one and open a follow-up if needed for the other issue.

I made a code suggestion because I think (not 100% sure) that list of files shown here https://www.drupal.org/files/issues/2023-08-11/gitlab-phpcs-with-basepat... → should actually be links.

jonathan1055 → changed the visibility of the branch main to hidden.

Updated title to reflect the changes. I will resume working on this when 📌 Add custom variables to set before_script commands in upstream project Active is done, as the downstream projects will be used to test these changes.

MR325 is ready for review and feedback. Here is a summary of the changes:

1. Switch to the drupal project folder to run the phpcs script, therefore using . for the files to check, not $_WEB_ROOT/modules/custom or $_WEB_ROOT/sites/all/modules/custom
2. check for the existence of either phpcs.xml or phpcs.xml.dist in the project folder, before getting the /assets/phpcs.xml file. Previously we only checked for phpcs.xml.dist and this worked fine because if the project had their own phpcs.xml it would take precedence over the curl'd phpcs.xml.dist. But doing the curl when not needed was not only wasting resources but also giving confusing info in the log
3. Improve the message about which phpcs.xml file is being used
4. Show info on the package versions using the simpler composer show | grep which gives better formatted output compared to composer show | awk {print $1 " " $2}
5. Add --basepath=$DRUPAL_PROJECT_FOLDER to the options. D10 did not have this option before. D7 did have it but was set incorrectly starting with $_WEB_ROOT so did not work properly.
6. Add --colors and --report-width=120 options to $_PHPCS_EXTRA if they are not already specified in that variable
7. Show the name of the standard being used and how many sniffs it contains
8. Echo "executing " to show the exact full command that is being run
9. Exit at the end of the job, not immediately on failure of phpcs
10. Check the return code properly and display "There are no PHPCS coding standards errors or warnings" if all is OK.
11. As a bonus - due to the new $DRUPAL_PROJECT_FOLDER variable holding the different path for Drupal 7 vs Drupal 10, the complete phpcs job definition can now be identical in the D10 and D7 files.

Before, to show the problems

d7-basic with forced failure BEFORE_SCRIPT_ACTIONS = phpcs-fail
log show long path FILE: ...ules/custom/gitlab_templates_downstream/gitlab_templates_downstream.module
https://git.drupalcode.org/project/gitlab_templates_downstream/-/jobs/43...
junit.xml has long paths
tests tab has long paths

d7-composer, clean
log shows long path
junit.xml has long path
test tab has long path

d9-basic with forced failure, same faults as above
log, junit and tests tab

d10-plus, clean
log, junit and tests tab

After, in downstream jobs

forced failure via BEFORE_SCRIPT_ACTIONS: 'phpcs-fail'
d7-basic
Log has short file path
junit.xml has short path
Tests tab has short path

d7-composer, clean
log has short paths, as do junit.xml and tests tab

d9-basic with forced failure
log has short path, same with junit.xmland test tab

d10-plus, clean
log, junit.xml and tests tab

The complete pipeline for MR325 is https://git.drupalcode.org/issue/gitlab_templates-3380694/-/pipelines/42...

I have left the original MR38 open as it has some comments in, so just need to check that.

The current code and results look great! The logs look nicer with the colors/width options.
Thanks for the summary of changes too and tests. Bonus point 11 is awesome!

Probably the comments in MR38 are not applicable here anymore due to the "cd" into the right folder and the rest of the params added.

I'd say that other than cleaning up debug code, this is ready. Not setting RTBC just waiting on the cleanup.

I thought you would like the bonus in 11.

Just re-read the comment in MR38, it was about not having a link to the file. There is nothing shown in the 'filename' column. I don't know how to add that, but it would be a separate follow-up anyway.

I've removed the debug code and the downstream override variables. Also moved the "There are no PHPCS coding standards errors or warnings" to the end of the script. Downstream pipelines all OK.

I triggered a couple more downstream pipelines here https://git.drupalcode.org/issue/gitlab_templates-3380694/-/pipelines/42...

Everything looks good, and I agree that getting that link could be a follow-up. RTBC.

One line seems to fail in https://git.drupalcode.org/project/api/-/jobs/4384772
Setting back to needs work to investigate/fix

OK. I will test ✨ Integrate Gitlab CI template for D7 Active

It might be because the result of the grep is empty.

That's what I thought, although that would be surprising. I updated API MR22 and added

phpcs:
  after_script:
    - $CI_PROJECT_DIR/vendor/bin/phpcs -e 
    - $CI_PROJECT_DIR/vendor/bin/phpcs -e | grep -E 'contains.*sniffs'
    - $CI_PROJECT_DIR/vendor/bin/phpcs -e | grep -E 'contains.*sniffs' || true

and this worked fine, exactly as intended. But oddly it did fail in the actual phpcs script, the same as when you ran it via downstream. So at least I can replicate the problem. I will add || true to the real MR, and also remove the 's' from 'sniffs' in the pattern, as a project could conceiveably have one sniff (although extremely unlikely). But that is not the cause of the problem here.

I have discovered the cause of the error. The API project has it's own phpcs.xml file, but the rule is defined as
<rule ref="vendor/drupal/coder/coder_sniffer/Drupal/ruleset.xml"/>
see https://git.drupalcode.org/project/api/-/blob/2.x/phpcs.xml.dist#L4
It is a hard-coded path relative to the project top-level directory. So when we run the job within the project directory it does not find this, and we get

$CI_PROJECT_DIR/vendor/bin/phpcs -e || true
ERROR: Referenced sniff "vendor/drupal/coder/coder_sniffer/Drupal/ruleset.xml" does not exist

But when the command is run at the original top-level folder (as happens in the added after_script, it works as before.
Here is the job that shows all this https://git.drupalcode.org/issue/api-3391946/-/jobs/4394940

A better way to specify the rules is by name, which we now do in this MR, see this change

If the relative hard-coded path is widely used maybe we can fix that in this job, or there are other solutions I'm sure. But at least we know now what the problem is.

Yeah, it's good to know why.

I can't remember where I took that file from, but I definitely took it from somewhere where many other people have as well, see the results here: https://git.drupalcode.org/search?group_id=2&repository_ref=2.x&scope=bl...

I agree that the new syntax is better, but I also think that since the above is also pretty standard, we need to find a way to make it work as well. We can throw a warning in the phpcs job to change the syntax, but if phpcs was working before this change, it should still work after this change.

fjgarlin → closed merge request !38

Yes we need to ensure that existing jobs run OK. The link you gave had 184 instances, but many of those are in scripts in composer.json or other build framework .yml files, or in vendor files. Those will not impact the pipeline job at all. I tried to see how many are in actual phpcs.xml(.dist) files and it is about 80 https://git.drupalcode.org/search?group_id=2&language%5B%5D=XML&page=4&r...

So maybe in the phpcs job after determining that we are going to use the project's own config, we can sed to replace the hard-coded path with the name of the rule. I will try that, and will use the API merge request to test it.

I have added two sed to change the full path into just the name

Tested in API 2.x here

Ready for feedback. I have also tested in Scheduler adding both a phpcs.xml and phpcs.xml.dist to make sure the job copes with that (even though the second one is redundant and would be ignored in any case)

I added some feedback to the MR, mostly about adding information about the change to the job log. The changes look good.

I've made a change and tested on API https://git.drupalcode.org/issue/api-3391946/-/jobs/4443621

The same needs to be done in main-d7 when we have settled on the output and wording.

I think the output looks great, but since we are changing it, we should include one more sentence recommending maintainers to change it themselves in the original fine. So, something like:

************************************************************************
In phpcs.xml the following
12:  <rule ref="vendor/drupal/coder/coder_sniffer/Drupal/ruleset.xml"/>
13:  <rule ref="vendor/drupal/coder/coder_sniffer/DrupalPractice/ruleset.xml"/>
 
has been edited to just the short name
12:  <rule ref="Drupal"/>
13:  <rule ref="DrupalPractice"/>
 
We recommend using the latter syntax in the configuration file.
************************************************************************

I have pushed a change for that. I actually added the file name as we have that in the variable, so the text is:

   
We recommend using the latter syntax in the project's phpcs.xml file.
 

Should we also give a link to this issue? Or maybe we should update https://project.pages.drupalcode.org/gitlab_templates/jobs/phpcs/ and link there instead?

Test with phpcs.xml with long paths
https://git.drupalcode.org/issue/scheduler-3445052/-/jobs/4460417

Test with phpcs.xml where paths are already the sort version
https://git.drupalcode.org/issue/scheduler-3445052/-/jobs/4460419

Ready for review again, but there is still lots of debug to remove, and I will do that when all testing is complete.

I have just re-read the start of this issue, and was wondering if re really need to change directory to the project's own top folder. Using --basepath seems to fix the relative paths even when running from the top build folder. I ran a custom phpcs here with long paths, no sed, from the build top folder and it seems to be correct https://git.drupalcode.org/project/scheduler/-/jobs/4460787
So maybe most of the extra work above is unnecessary.

Re #28, if we add and link something, let's make sure that it's in the documentation page.

Re #29, that's a great question/finding, we were so busy fixing things that we didn't consider that 😅.
In this case, if --basepath fully fixes the issue, then we don't need to do the sed replacement to run the job. However, I see some value in detecting the vendor/... syntax and recommending calling the rule by its name as the syntax is path agnostic. So maybe we can detect and report the suggestion, but do nothing else other than that. And, also, worth adding this recommendation to https://project.pages.drupalcode.org/gitlab_templates/jobs/phpcs/.

However, I see some value in detecting the vendor/... syntax and recommending calling the rule by its name

I pushed a change to run from top-level before I saw this comment, which removed most of that. But I can add back the detection and message.

Ready for review again. Here's a test on Scheduler to show the message
https://git.drupalcode.org/issue/scheduler-3445052/-/jobs/4469141

I have added 'phpcs-fail' to two of the downstream pipelines (d7-composer and d10-plus) so that we can check the output. I also added long paths into the d7-composer phpcs.xml so we can see the message there too. That's a permanent change in GTD.

Here is the updated doc page. This one has not been modified yet since the very first set of pages.
https://git.drupalcode.org/issue/gitlab_templates-3380694/-/blob/3380694...

Everything looks great. I only added a comment about the configuration file detection, where I think we can be more specific about the files to look for to avoid possible false positives.

From discussion on Slack I added the collapsed section to list all sniffs, see screenshots
https://git.drupalcode.org/project/gitlab_templates_downstream/-/jobs/44...

Yes the regex can be tighted up.

Changes since last review

1. all sniffs written to collapsed section in the log
2. ls restricted to just the four valid variations of the config file name
3. use 2>/dev/null so that the ls does not produce a whole set of confusing messages if there are no files
   

   ls: cannot access '.phpcs.xml.dist': No such file or directory
   ls: cannot access '.phpcs.xml': No such file or directory
   ls: cannot access 'phpcs.xml.dist': No such file or directory
   ls: cannot access 'phpcs.xml': No such file or directory
   

4. Implemented before_script action 'phpcs-duplicate-files' in the downstream project branches to allow testing with multiple valid (and non-valid) config file names

Downstream pipeline phpcs jobs

d7-basic
https://git.drupalcode.org/project/gitlab_templates_downstream/-/jobs/44...
nothing done in before_script_actions

d7-composer
https://git.drupalcode.org/project/gitlab_templates_downstream/-/jobs/44...
has phpcs-fail

d9-basic
https://git.drupalcode.org/project/gitlab_templates_downstream/-/jobs/44...
has phpcs-dupliacte-files. Shows the three files, not phpcs.xml as expected
Lists all the files including phpcs.xml.old
first file in the list is .phpcs.xml, as shown in the log message warning of long paths

d10-composer
https://git.drupalcode.org/project/gitlab_templates_downstream/-/jobs/44...
has both phpcs-fail and phpcs-duplicate-files, all as expected

This is ready for review. Still need to remove the debug, when all OK.

It really looks good to me. Is it possible to setup a phpcs.xml file which has no sniffs? just to test that edge case. It's not the common thing at all but just want to make sure that the greps and checks introduced here can manage that case.

Interesting question. I've just tested that on Scheduler - the before_script renames to use this empty file. There is no problem with the new greps, etc, but the job does fail when listing all the sniffs, we get "ERROR: No sniffs were registered"
https://git.drupalcode.org/issue/scheduler-3445052/-/jobs/4484234

I think the real phpcs call would fail in the same way. A simple change would be to make the collapsed list of sniffs executing the real analysis, then we'd see the "ERROR: No sniffs were registered" plain in the log, as that would be done first.

Or we can just add the $CI_PROJECT_DIR/vendor/bin/phpcs -e || true to the line that lists the sniffs. We don't really care about the exit code of that command, just the output.

I had already pushed the other idea to the d7 branch, but I think your way is better, as it is safer overall. So I have done that on both branches and tested on Scheduler https://git.drupalcode.org/issue/scheduler-3445052/-/jobs/4484665

Can you run the other downstream pipelines? I will remove the debug when all OK.

Rest of the dowstream pipelines: https://git.drupalcode.org/issue/gitlab_templates-3380694/-/pipelines/43...

I think this is looking good. Are you doing more tests or shall we remove the debug?

I've done all the testing I need, and your review and downstream pipelines all show OK, so I have removed the debug. Best to re-run all the downstream pipelines again, just to check.

Downstream pipelines: https://git.drupalcode.org/issue/gitlab_templates-3380694/-/pipelines/43...

I've checked the code one more time with the debug and it is looking good. All dowsntream jobs run as expected. We can see the jobs still working but showing the recommendations: https://git.drupalcode.org/project/keycdn/-/jobs/4539066

Merged! Thanks so much for the work here.

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