PHPCS is failing on contrib modules

The project that is failing is https://www.drupal.org/project/pagerer

Interesting - so DrupalCI falls back to using the entire Drupal standard.

01:54:27 cd /var/www/html/modules/contrib/pagerer && /var/www/html/vendor/squizlabs/php_codesniffer/scripts/phpcs --warning-severity=2 --report-full=/var/lib/drupalci/workdir/phpcs/codesniffer_results.txt --report-checkstyle=/var/lib/drupalci/workdir/phpcs/checkstyle.xml --report-diff=/var/lib/drupalci/workdir/phpcs/codesniffer_fixes.patch --standard=Drupal --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md /var/www/html/modules/contrib/pagerer

In this instance DrupalCI needs to install the standard. In fact I would argue that it should always do an install of the standard after running composer install. This was being done by the Drupal Composer class - but this affects global state so we shouldn't be doing that. DrupalCI does not have to care about global state because each run is isolated.

DrupalCI will need to run something like

$ /var/www/html/vendor/bin/phpcs --config-set installed_paths /var/www/html/vendor/drupal/coder/coder_sniffer

@mondrake yeah I just included a link to the project to make it easier to see which project the test in the issue summary was referring too. This will affect nearly all contrib apart from those that completely manage their own dependency on phpcs.

Something like this should fix it....

Yah, this is because the testbot relies on core's config of phpcs.

This illustrates how not-isolated the testbot is from the system under test...

It's a little funny because the testbot has the same Composer::configurePhpcs() pattern for its own CS checks.

Added a branch, but it hasn't shown up yet: 2851661-phpcs-config-coder

Yah there. :-) Got the number wrong.

This is affecting at least two projects that I have in Drupal 8 that run automated tests. It looks like the changes have been committed -- have they been deployed? If so, they aren't working.

Interestingly, a Drupal 7 project that I have is working fine... although the last time it ran the test was April 19 (tested on commit), so maybe this isn't working in D7 any more either? Not sure.

drupalci works on feature branches, not patches, so the commits were to the feature branch. Still needs review.

Just checking - the commit in #10 is nothing like the patch in #7, so could you confirm what exactly we should be reviewing (not that I have the skill to give a proper review but I am following and learning from this thread). Thanks

Both commits are on the same branch, so #12 is in addition to #10.

Yes, I got that. What I was questioning was that the code fix commit in #10 was not the same as the patch in #7. So is the patch in #7 discarded/irrelevent now?

Oh, sorry. My bad.

The patch in #7 just adds another command, but the change in #10 moves existing code out of installGenericCoder() where it's no longer needed and into run(), since we'll always need to call it.

Still passing tests locally... :-)

And tests are still failing to run coding standards tests on contrib D8 projects, with:

https://dispatcher.drupalci.org/job/drupal_contrib/54265/artifact/jenkin...

Here's an example:
https://dispatcher.drupalci.org/job/drupal_contrib/54265/artifact/jenkin...
from job
https://dispatcher.drupalci.org/job/drupal_contrib/54265/

@mile23 what does this need for review? Just a standard code review/run test on my system or does it need to be reviewed by any specific people?

I wish #2851661: Ensure that we're using the right ruleset for coding standards checking hadn't happened...

Some might raise an issue that this makes it more difficult to do coding standards checks against a non standard install where vendor has moved.

That's actually not a small thing because its clearly causing issues for contrib. Additionally it is now basically impossible for a project like a custom site or a contrib module running CI outside D.O to track core's coding standards because the codified version has hardcoded paths against a _specific_ Drupal core installation setup. If you copy the file out or try to diff against it you get a useless "every rule is different" blob.

Maybe that other issue needs to be reverted?

So there are a few issues here:

1) Core changed their setup to not configure phpcs and instead use file paths in phpcs.xml.dist, so we have to pick up the slack here for contrib to work properly. It's not that big a deal in terms of the testrunner, because we can just adapt. If it's a big deal in other areas, maybe file a core issue to reverse it. The use-cases of non-d.o CI for contrib is outside of scope for drupalci_testbot.

2) The dev process here is that we have a fix, it just needs some review and for a certain someone to push to dev and then production. :-)

3) There's the complication that if core is modified back to the old way of configuring phpcs, then we might need to update the testbot again. Though, if it does go back to automatically configuring phpcs on composer install, our subsequent re-configuration might end up working anyway.

So: If someone could please set up a local dev testbot, switch to the 2870645-phpcs-config-coder branch, and run the testbot tests then please RTBC when they pass, that'd be great. Learn how to do that from TESTING.md: http://cgit.drupalcode.org/drupalci_testbot/tree/TESTING.md

There's the complication that if core is modified back to the old way of configuring phpcs, then we might need to update the testbot again. Though, if it does go back to automatically configuring phpcs on composer install, our subsequent re-configuration might end up working anyway.

I can't possibly fathom why composer would in any way be involved....

How do I provide feedback on said branch? can you upload a patch?

Previously, core would use a composer script to configure phpcs to use drupal/coder. Now it doesn't. That's how composer would be involved.

As far as how to provide feedback: It's a feature branch of drupalci_testbot called 2870645-phpcs-config-coder.

To make it work, you can follow the instructions in http://cgit.drupalcode.org/drupalci_testbot/tree/TESTING.md and just substitute 2870645-phpcs-config-coder for dev in the git checkout.

Or read the changes in #10 and #12 above.

Field report ...

4 days ago this bug was seen in the wild in a contrib project

ERROR: the "Drupal" coding standard is not installed. The installed coding standards are Zend, Squiz, PSR2, PSR1, PHPCS, PEAR and MySource

https://dispatcher.drupalci.org/job/drupal8_contrib_patches/10445/consol...

and yesterday

Here in the same issue the problem is resolved and all the coding standard errors are laid out in their full glory

https://www.drupal.org/pift-ci-job/733743

So +1 from me on closing this issue.

I see this is fixed.... I would actually close this issue but I think other readers .. have other things to say

Sounds like this only affects Drupal 8 modules.

The August 4 test run on Drupal 8 contrib module config_update seems to actually have run the code sniffs. This is the same module that I reported in comment #20 that was not running the code sniffs on June 25.

So on this August 4 weekly run, the same artifact file looks like this:
https://dispatcher.drupalci.org/job/drupal_contrib/73867/artifact/jenkin...
It looks like we have a typo in the documentation. :)

Here's the full test job:
https://dispatcher.drupalci.org/job/drupal_contrib/73867/

So I agree with martin107 that the bug appears to be fixed for Drupal 8 projects (the report on comment #27 was also for a D8 project).

Tentatively marking this Fixed, unless we have reports of additional Drupal 8 projects that are still having this problem on new test runs...

Well, there's the commit in production branch: http://cgit.drupalcode.org/drupalci_testbot/commit/src/DrupalCI/Plugin/B...

Strange that even though it has the issue # in the commit message, it doesn't show up in the comment thread.

Anyway, yah, fixed. :-)

Interesting... I'm pretty sure my concerns haven't been addressed since I don't know how to provide them :-/

@neclimdul: Agreed, but using a nonstandard vendor directory is out of scope for the testbot.

Reverting the recent title change. The problem affected both D7 and D8 modules, and was fixed simultaneously by the same commit. It's better to avoid leaving a mis-leading title.

Thanks alexpott and Mile23 for fixing this. I did try to help by attempting to install my own local testbot as per instructions in #24 but I did not get it running. It is very good that we have coding standards checking back on - thank you all.

After I got drupalci back into a testable state, I reviewed and verified the patches here and got it in. Then I was updating this issue, got distracted in another tab, and never made it back here to let everybody know it was fixed.

Huge thanks to Mile23 for getting this working again.

That's not actually what I was intersted in. I'll open a new issue to discuss why we need to revert.

#2902381: Revert phpcs.xml.dist changes from "Ensure that we're using the right ruleset for coding standards checking"