DrupalCI should return failed for poor code style Part II - php

That's part of the point here: Figure out the use-case for contrib.

So far the idea is that if the contrib project doesn't have a phpcs.xml.dist file, then there's no sniffing.

We're also just making a report at this point, and not doing the failing until there's a way to configure it through d.o.

Added better behavior for phpcs plugin.

Don't sniff if:

• There's no phpcs executable
• There's no phpcs.xml(.dist) file in start_directory.

There's no longer a config for config_directory because start_directory should be where your phpcs.xml.dist file resides.

Added a test which uses Examples as a fixture. Added this issue to the Examples project to provide a place to park patches: #2842832: Dummy issue for DrupalCI

Er rather I see now they run inside the container. I'll assume that means it cant be on the host, but phpcs *could* be installed globally if we wanted.

Coder/phpcs should be a dependency of the project, and we only check if it's there. This is the same problem solved by composering under the container: If the project specifies a version of coder/phpcs that can't be resolved under the platform constraints, then it's a bad idea to run it, and the maintainer should find out about it.

So once the compser-within-the-container thing happens, this part will solve itself.

The checkstyle report produced inside the container has the container paths in it. which makes jenkins full of java exception hate:

I'll look at other output formats because it would be much better to not have to modify anything in the report file. Otherwise we end up with the same container vs. host path problem, which is a bunch of string functions traversing an XML file.

One more thing, we also might want to consider how to handle only changing phpcs.xml.dist. If a patch changes the rules, it ought to sniff the whole codebase? Not sure.

This is a good point. If either phpcs.xml or the dist is changed, then sniff everything.

It turns out core has a package.json file, which does all the specifying for eslint. #2809477: Add ES6 to ES5 build process So we'd do an npm plugin and then a run-jslint plugin, both in the container. I think our implementation issues for that are #2600626: [PP-1] Ensure availability of node/npm in the testrunners and #2575501: [PP-2] DrupalCI should return failed for poor code style - JS

ada601ee adds a behavior where if phpcs.xml or phpcs.xml.dist is changed, then the sniff is run against the whole project, with a test. Also we strongarm the checkstyle file paths to be host paths, as per #15.

The value '/phpcs/phpcs_checkstyle.xml' shows up a few times. maybe we set that on a property on this plugin and use that instead

We really should be doing this generically anyway, so that it's always /{pluginname}/some_report.xml. But I'll make that a follow-up and allow configuring the report filepath.

getPhpcsExecutable() -> Im kinda not sure we should be so rigid with our check. I think we ought to maybe leave room for a phpcs that just happens to live inside the executable containers, much like composer does

D8 core is doing coding standards by adding individual rules in manageable patches, against a specific version of coder. That means we will always be requiring coder at a specific version, until maintainers decide to move on to the next one. Coder will always require the phpcs executable.

This means we can let core do all the work of determining whether phcps should be there, and what version it is.

This also means we can let Composer do the work of telling us when there are version and platform constraint problems wrt phpcs.

Also: In order for phpcs to work with coder, it has to be configured with installed_paths to use the composer-specified version of coder. That has to be the container path to /vendor/drupal/coder/coder_sniffer/.

Basically it's all the same reasons we should be re-running composer install inside the container.

I also think checking-for-the-tool is going to be a common pattern, too, because for JS linting we'll be doing an npm install and then checking whether eslint is present where we expect it.

finally, lets ensure that whatever needs d7 overriding can easilly be done so in a phpcsD7 plugin.. I didnt fully look it over, maybe its already that way.

We can't really test the behavior for D7 core because no config file. Find a D7 contrib with a phpcs.xml.dist and we can work on it. I think D7 core/contrib should be a follow-up when someone asks for it. It's really clear what D8's needs are, but not so much D7.

Basically it's all the same reasons we should be re-running composer install inside the container.

...IN FACT....

We have to run composer install inside the container for this to work with #2744463: Add phpcs, coder 8.2.8 as --dev requirements for drupal/core.

Because we run a composer post-install event which we use to configure phpcs to use coder.

We could work-around since we already know the installed_paths directory is a requirement, and the plugin does that, but it'd be better to have the composer step in the container.

This adds the report_file_path config per #24 and updates the tests for the new dev environment.

We're basically postponed here for core 8's use-case while we figure out #2845802: Run composer install inside the container to allow for platform constraints, path-based config

After a certain point, core will perform the config of phpcs to use coder, so we won't need the installed_path config for D8.

Core D7 doesn't have a phpcs.xml (or a composer.json to depend on phcps), so it's unclear how to proceed.

Contrib for D8 could have a phpcs.xml file, but no composer.json, in which case it would inherit whatever versions core is using. That could lead to wrong reporting. If the contrib has a composer.json, it might need a helping hand with installed_path config. Or not. How should we proceed with a merged dev dependency that might be satisfied by core and installed by core?

We were unable to find a D7 contrib project with a phpcs.xml file, so it's not a use case at the moment. Obviously that can change.

In IRC we talked about having a global code quality metric where we could just run coder against all the projects. That's out of scope here, but it's an interesting idea. It kind of suffers from the same ambiguity as the above situations, though.

The principles that we decided on in IRC are: Everything gets sniffed, and if you want to control how your project is sniffed, add a phpcs.xml.dist file. If the project doesn't require coder or phpcs, we require the latest stable of drupal/coder. If the project has a phpcs.xml(.dist) file, we use that, otherwise we use the current Drupal standard.

OK, so there's a matrix of test cases you can see here: http://cgit.drupalcode.org/drupalci_testbot/tree/tests/DrupalCI/Tests/Pl...

They represent a bunch of use-cases.

The use cases are a matrix of the following components per project:

1. Has composer requirement for phpcs/coder. This would be, eg, core after #2744463: Add phpcs, coder 8.2.8 as --dev requirements for drupal/core If the project doesn't have a requirement for coder, we require drupal/coder @stable and use it. If core requires one version and contrib requires another, they fight it out in Composer-land and our plugin has no opinion. If this becomes a problem, file a follow-up and we'll make it work.
2. Has a phpcs.xml(.dist) file. If the project has a phcps config file (.dist or not), then we use it. If it doesn't, we use the Drupal standard with CLI options as per https://www.drupal.org/node/1587138
3. Has modified files. We want to only sniff modified files if possible. If there are no modified files, we'll sniff the whole project. This would be for, eg, branch testing.
4. Has a modified phpcs.xml(.dist) file. If the config file is modified, we always sniff the whole project. Note that 'modifying' the config file could be adding or removing. If it's removing the config file, then we honor the sniff_only_changed configuration, and try to only sniff the changed files.

While there's a way to cause this thing to stop the build based on CS errors (thus failing the test), that's off by default. You only end up with a phpcs report in your artifact folder.

What still needs to happen:

D7 tests, more contrib tests, and most of all, the following:

This plugin uses Composer inside the PHP container to resolve dependencies. This means it does not have access to the cache of metadata goodness which lives in the host. We need to map /opt/drupalci/composer-cache from the host to a similar place in the PHP container. Then Composer takes only a few seconds to figure out which drupal/coder to use, instead of 30.

Added more tests.

Needs a follow-up to determine what's installed using a better technique than looking for a phpcs executable.

Other than that I think we should move forward and learn more things before trying any more.

Follow-up #2847519: Phpcs plugin should use installed.json to figure out what's installed.