Problem/Motivation
We added phpcs.dist.xml in #2573377: Add our own phpcs.xml file we chose to exclude rules. This was because we thought it was better to get improvements from drupal/coder and phpcs automatically. But this is problematic because HEAD could just start failing completely out of our control - for example #2666112: phpcs has a new sniff: Generic.PHP.LowerCaseKeyword
Proposed resolution
I discussed this with @xjm and we agreed that (obviously) core can't and doesn't want to hold back development on drupal/coder or phpcs, therefore, we have swap the file around to only detail what sniffs are currently passing.
Remaining tasks
User interface changes
None
API changes
None
Data model changes
None
Comment | File | Size | Author |
---|---|---|---|
#6 | 2666172-6.patch | 8.18 KB | alexpott |
#2 | 2666172-2.patch | 5.28 KB | alexpott |
Comments
Comment #2
alexpottHere's a patch that swaps this around. We need #2620576: fnmatch() is not available on all environments (i.e QNAP QTS) to land so that
Drupal.Functions.DiscouragedFunctions
passes on the 8.2.6 release of coder.Output from running test:
Comment #3
pfrenssenAlex, I guess you have symlinked the Drupal coding standards inside the PHPCS standards folder. I have a local installation of PHPCS and the Coder module managed by Composer. I had to change the file to the following:
Then I'm getting the same results:
We can't add Coder to the core Composer dependencies since it is packaged as a Drupal module. As our Composer integration improves this is bound to end up in the modules/ folder instead of the vendor/ folder at some point in the future. The best solution would be to abstract out the code sniffer ruleset into a separate PHP project, so we can add it along with PHPCS as dev-dependencies in our composer file. We would have full control over the exact version of the coding standards that is used to test a specific version of core too. That's way out of scope for this issue though. We should discuss this with Klausi.
So I guess on the testbot PHPCS is also installed with a symlink to the Coder ruleset in its standards folder. This is fine for now. What is most important is that we can run the coding standards checks automatically without breakage due to upstream changes in the coding standards.
Going to mark this as postponed on #2620576: fnmatch() is not available on all environments (i.e QNAP QTS). Once that is in we can run a retest of this before merging.
Comment #4
pfrenssenI just checked and Coder is no longer packaged as a Drupal module, it already declares itself as a standard PHP library. That would make it easy to add it as a dev-dependency to core.
Comment #5
alexpott@pfrenssen but then you'd have to make changes to use the current phpcs.xml.dist. phpcs needs access to the Drupal ruleset regardless whether we include or exclude. Therefore, I think we can proceed with this issue because with your current set up we wouldn't have committed the phpcs.xml.dist in the first place.
Comment #6
alexpottRerolled and fixing the things we've inadvertently broken. No interdiff possible cause reroll.
Comment #7
pfrenssenCool #2620576: fnmatch() is not available on all environments (i.e QNAP QTS) has been fixed. This looks good. Thanks!
Comment #8
pfrenssenBy the way I have discussed the problem I mentioned in comment #3 about the location of the coding standard in our
phpcs.xml.dist
with the PHP CodeSniffer maintainers. The best solution for the moment is to run the tests with the--runtime-set installed_paths
option pointing to the directory that contains the coding standard.For example if you have installed Coder with composer in the root of the Drupal site:
In the next release of PHP CodeSniffer it will even be easier. This configuration setting can now be used in custom rulesets (ref. PR #899) so anyone that has the coding standards committed in their project can provide their own
phpcs.xml
that includes core'sphpcs.xml.dist
and set the path of the coding standards:Running the coding standards is then as simple as:
This will be 100% reliable since you can control the exact versions of Coder and PHP CodeSniffer with composer. No unexpected failures when either of the projects releases an update.
Comment #10
catchCommitted/pushed to 8.1.x, thanks!