Problem/Motivation

Part of #1299710: [meta] Automate the coding-standards part of patch review.

We want the test runner to be able to run phpcs to check coding standards for projects. This could allow for halting the test run based on CS errors.

Proposed resolution

It's not really the job of the test runner to figure out what version of coder to use, or what rules apply, so core should allow us to run phpcs and capture the results.

This fits with the general CS strategy of core, which changes the set of sniffs which should be fixed as CS errors are fixed.

So that means that for the runner to work we need to modify it with the following:

  • Add a phpcs task plugin. (It'll be a near-copy of DrupalCI\Plugin\BuildTask\BuildStep\CodebaseValidate\PhpLint.)
  • The phpcs task will run phpcs within core/, or within the root directory of a contrib project.
  • Add some sanity-checking on core version to make sure we're not trying to run phpcs against an earlier version with no phpcs. (D7 or D8 from before phpcs is a --dev requirement.)
  • Modify the test run definitions to include this new plugin.
  • Look at a configuration variable to determine whether to fail the test run based on failed CS.
  • Store the result file in the artifacts directory.

Remaining tasks

Update d.o infrastructure to use this information usefully.

Update d.o UI to allow for failing the test run on CS errors.

User interface changes

API changes

Data model changes

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

cweagans’s picture

This is postponed because we can't turn this on until core is 100% compliant or all the tests will start failing.

Then again, perhaps that'd be a good way to get a LOT of people working on code style cleanup! =P

basic’s picture

Issue summary: View changes

We are working on cleaning up Drupal.org-related issue queues. Many issues have been open for years with no resolution because there were no volunteers interested in taking on the task or staff time available to complete the work.

We are going through old issues and marking them as Closed (won't fix). If you feel your issue has been closed in error, please do comment on the issue and let us know. If we know something is critical, we may reconsider the closure of a feature request or bug report.

Thank you for your understanding while we work to clean up the Drupal.org queues.

YesCT’s picture

Project: Drupal.org infrastructure » Drupal.org Testbots
Component: qa.drupal.org » Miscellaneous
YesCT’s picture

The current plan is to only put one coding standard in place for automatic testing, one that core already passes for. And start the system like that.

Then, we can introduce one new standard at a time, along with an issue to get core passing for just that one additional standard.

isntall’s picture

Title: Make qa.drupal.org returned failed tests for poor code style » DrupalCI returns failed tests for poor code style
Project: Drupal.org Testbots » DrupalCI: Test Runner
isntall’s picture

Title: DrupalCI returns failed tests for poor code style » DrupalCI should return failed for poor code style
jthorson’s picture

This would be implemented through a new 'CodeSniffer' job type.

Instructions on what this job type would need to perform are located at https://www.drupal.org/node/1419988.

I anticipate this would leverage the PHP containers instead of the web containers, as I do not believe CodeSniffer have any apache dependencies.

mradcliffe’s picture

Added a related issue from PIFR that will need to be taken into account in the CodeSniffer job type.

pfrenssen’s picture

Status: Postponed » Active
Parent issue: » #2571965: [meta] Fix PHP coding standards in core

Setting back to active as per #2571965: [meta] Fix PHP coding standards in core. This has been cleared to work on, but any changes can only be put in action after the release of 8.0.x-RC1.

To work around the chicken-and-egg problem that the tests would fail unless all coding standards are fixed, we are going to maintain a blacklist of coding standards checks. Initially we will blacklist all coding standards, so that effectively no tests will run. We will then fix coding standards one by one, each time removing the fixed standard from the blacklist. These standards can then be tested automatically from that point onwards. The blacklist is being added in #2573377: Add our own phpcs.xml file.

pfrenssen’s picture

Status: Active » Needs review
FileSize
4.93 KB

Here's an initial implementation of a 'php_codesniffer' command. It downloads the Coder module using git, installs PHP CodeSniffer by running composer install in the Coder module folder, and runs the coding standards checks based on a phpcs.xml.dist which is expected to be present in the core/ folder (this will be added in #2573377: Add our own phpcs.xml file).

It is currently using the latest 8.x-2.x dev release of Coder, but we should probably "pin" a recent commit to avoid sudden failures if new sniffs are added to the Coder module.

attiks’s picture

Patch looks good and works, but what else is needed to move this forward?

pfrenssen’s picture

It needs to be RTBC'd :)

attiks’s picture

Status: Needs review » Reviewed & tested by the community

Marking as RTBC, if this gets deployed to the real test bot, I think we should add coder to the docker image to save time and pin it to a certain version.

pfrenssen’s picture

Priority: Normal » Major

Raising priority, now that the release candidate is out we can start working on coding standards fixes, but this needs to get in first. This is now blocking the 30+ issues from #2571965: [meta] Fix PHP coding standards in core.

  • jthorson committed 00316ef on 1266444-add-coder-support authored by pfrenssen
    Issue #1266444 by pfrenssen: DrupalCI codesniffer support
    
jthorson’s picture

Added to branch 1266444-add-coder-support

Ready for merge, and no risk (completely different job type).

elachlan’s picture

elachlan’s picture

Mixologic’s picture

Title: DrupalCI should return failed for poor code style » DrupalCI should return failed for poor code style - php
pfrenssen’s picture

Is there anything holding this up? The whole coding standards initiative has lost its momentum after waiting 6 weeks for this issue to be committed :(

pfrenssen’s picture

This probably needs another look in the meanwhile.

basic’s picture

Unfortunately there is a lot of work that still needs to happen. We are not just waiting on this to be committed in order to enable coding standards.

DrupalCI needs a larger refactor in order to incorporate a change like this, and while we can commit this as is, DrupalCI would not actually be able to use it correctly because it is a different job type.

Each Job in drupalci currently assumes an entire process from start to finish - environment creation (containers), Codebase setup (git clones and patches), running the Job, and finally post processing the Job results in to a standardized format for jenkins to consume, so that Drupal.org can access to the results from the Jenkins API.

Additionally, the AWS management portion of the DrupalCI Job process spins up EC2 instances in response to individual Jobs, and we cannot afford to spin up double the number of bots in order to make this happen.

What we need is to refactor DrupalCI to be able to execute multiple "tasks" as part of a "job". This way we can have simpletest testing, plus all of the other coding standards checks, plus any additional tasks that we come up with all run as part of the *same* Job, without spinning up a new bot, rebuilding/reconstructing the container environment or recreating the codebase.

Before we do the refactor needed for splitting Jobs into smaller "tasks", the priority is to add functional/unit testing to DrupalCI. We need a better way to verify that we're not introducing regressions. We need to do some pretty serious restructuring of the codebase to make that happen, primarily we need to add a dependency injection container to have proper mocks and real unit tests alongside some functional tests.

Once testing is in place, we can begin the refactor DrupalCI to allow for tasks inside of a jobs knowing we haven't introduced big regressions. This patch can be refactored as a task in the DrupalCI Job which outputs checkstyle xml format, Jenkins can be configured to parse that xml, and Drupal.org integration can be built to parse that information from the Jenkins API and display the results on Drupal.org.

YesCT’s picture

Issue tags: +Coding standards
xjm’s picture

@basic, so how can we move this issue forward?

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/src/DrupalCI/Plugin/JobTypes/php_codesniffer/PHPCodeSnifferJob.php
    @@ -0,0 +1,72 @@
    +    'DCI_PHPVersion' => '5.5',
    

    Might as well use PHP7 it'll be quicker and therefore cost less

  2. +++ b/src/DrupalCI/Plugin/JobTypes/php_codesniffer/drupalci.yml
    @@ -0,0 +1,26 @@
    +  command:
    +    - cp /var/www/html/core/phpcs.xml.dist /var/www/html/core/phpcs.xml
    

    Not needed with 2.5.0 version of PHPCS

Is spinning up a new bot expensive - I thought it was mostly about the time a bot is running - these bots will be short lived.

Mile23’s picture

Codesniffer is properly a separate job, since that gives us a way to specify that it run last or whatever.

As per the discussion over here: #1299710-50: [meta] Automate the coding-standards part of patch review we should have a way to specify individual files, based on what files have changed in the repo.

Mile23’s picture

Something more like a review:

We want to add drupal/coder and squizlab/php_codesniffer to our composer.json file, so we have an executable in the testbot rather than relying on downloading the module somewhere.

That leads to:

+++ b/src/DrupalCI/Plugin/JobTypes/php_codesniffer/PHPCodeSnifferJob.php
@@ -0,0 +1,72 @@
+    'DCI_RunScript' => "/var/www/html/modules/coder/vendor/bin/phpcs",

This path should point to the phpcs binary we'd have installed by composer.

Mile23’s picture

FileSize
14.15 KB
9.22 KB

Here's an appropriate change to composer.json, based on #11. This doesn't make any other code changes. Run composer update after you apply this.

I'd do more code-y work on this but the vagrant box is a pile.

alexpott’s picture

Can't we do something simpler and more cost affective? At the moment we php lint all the changed files as part of every test. Can't we just add a step to run phpcs with the drupal coding standards and core's current config against the changed files. This is what I'm doing on every commit I make with my precommit hook... https://github.com/alexpott/d8githooks/blob/master/pre-commit

At least if we do this we don't introduce regressions for things we've already fixed.

Mile23’s picture

Addressing blockers for this issue: #2654650: [meta] Jobs for linting and coding standards

hass’s picture

We already had coder_love sh** enabled in past. This implemented non-core rules. Additionally coder module *always* has fails detections. I never seen it working reliable.

I'm wondering how you'd like to solve this.

pfrenssen’s picture

@hass how long has it been since you used Coder? It's very reliable. I run it multiple times per day on a very large distribution that has around 10000 PHP and JS files across hundreds of custom modules and features, written by 70+ developers of varying skill levels. We have not a single false positive.

If you encounter a false positive then please file an issue so it can be fixed.

alexpott’s picture

@hass another point is that we're only enabling rules in phpcs.xml.dist when we have core passing. Therefore all you have to do is remove on of the disabled rules and get core to pass in a patch... and hey presto we'll then get no regressions.

hass’s picture

@pfrenssen: I'm not aware that only one of my fails positive issues I have filed against coder has been fixed in the last 5 years. All my cases should still be open. I think my last test was two months ago when i checked all my D8 projects with coder "minor" mode.

hass’s picture

jthorson’s picture

I think there's a bit of confusion here ... PAReview != Coder.

PAReview was created to automate reviews of Project Applications long before Coder was refactored to use CodeSniffer; and has it's own set of rules and custom scripts.

jthorson’s picture

@hass: In any case, your problem appears to be with Coder, and not with DrupalCI ... this issue is about enabling Coder type reviews in DrupalCI, but we shouldn't derail the effort by bringing baggage which is specific to Coder module into scope of this particular discussion.

xjm’s picture

In a meeting with the core committers and DA infrastructure staff today, we learned that it will take a lot of resources or a long time to actually implement this on DrupalCI. A workaround was proposed for the short-term: #2663086: [meta] Determine whether run-tests.sh can trigger and report phantomjs and phpcs results

alexpott’s picture

Status: Needs work » Needs review
FileSize
1.18 KB

So we could make phpcs run as part of the simpletest job and only on the files that have changed after applying a patch. The patch here fails as expected when applying https://www.drupal.org/files/issues/phpcs.test.patch and running some tests. We can probably optimise the installation of phpcs and Drupal's coding standards but pulling them down each time at least allows improvements and perhaps we can get rid of the sleep 5 to regain the lost seconds :)

The next will be to work on how we communicate this back to Jenkins etc...

alexpott’s picture

Issue summary: View changes
alexpott’s picture

FileSize
2.02 KB
1.67 KB

Patch attached generates a checkstyle report and saves it in /var/www/html/artifacts/phpcs-checkstyle.xml. Afaics that should mean that this will get shipped back to Jenkins and Jenkins has a plugin to read checkstyle files...

Here's an example of a checkstyle file when it fails:

<?xml version="1.0" encoding="UTF-8"?>
<checkstyle version="2.5.1">
<file name="/var/www/html/core/modules/update/src/Form/UpdateManagerUpdate.php">
 <error line="120" column="12" severity="error" message="Use &quot;elseif&quot; in place of &quot;else if&quot;" source="Drupal.ControlStructures.ElseIf"/>
</file>
</checkstyle>

If there are no files changed (a branch test) then there is no code style test and therefore no file.

If the patch has no errors detactable by phpcs then you get an empty report:

<?xml version="1.0" encoding="UTF-8"?>
<checkstyle version="2.5.1">
</checkstyle>
Mixologic’s picture

Component: Miscellaneous » Jobs and Job Handling
Mixologic’s picture

Component: Jobs and Job Handling » Coding Standards
Mile23’s picture

Mile23’s picture

Issue summary: View changes
Mile23’s picture

Issue summary: View changes

  • Mile23 committed f3f71be on 1266444-phpcs-coder
    Issue #1266444: Basics working. Use simpletest_sniff test definition for...
Mile23’s picture

This issue already has branch 1266444-add-coder-support but it's from before the big refactor.

Adding branch 1266444-phpcs-coder which adds some non-spec behavior just to get it working.

What it does:

  • Adds a simpletest_sniff test definition which omits the run-tests.sh part of testing.
  • Adds Codebase::getModifiedFilesForNewPath($path) which lets you substitute the environment path for the build path.
  • Adds a composer_phpcs plugin which always uses composer to require coder and phpcs. This replaces the composer plugin in the test definition flow during build. This is a temporary measure and eventually we’ll just check if the phpcs executable is present in the validate plugin after #2744463: Add phpcs, coder 8.2.8 as --dev requirements for drupal/core
  • Adds a core_phpcs plugin which occurs just after PHP linting. This needs to be split into a generic runner and core’s specific needs.

Run it and see. Set up a local testbot and do this:

DCI_UseLocalCodebase=/var/lib/drupalci/drupal-checkout DCI_LocalBranch=8.3.x DCI_Fetch=https://www.drupal.org/files/issues/2839170-coder-phpcs-sniff-error.patch,. DCI_Patch=2839170-coder-phpcs-sniff-error.patch,. DCI_CS_SniffFailsTest=TRUE ./drupalci run simpletest_sniff

echo $? then shows you that the test failed because of poor CS by outputting 1. Remove the DCI_CS_SniffFailsTest env variable it will pass despite poor CS. There’s also a DCI_CS_WarningFailsSniff which promotes CS warnings to failures.

Things to think about:

Both linting and sniffing occur in a step called validate_codebase. validate_codebase happens after the containerized environments have been set up, which means that they currently have to wait for things like the database to start up. This is necessary because the PHP environment is required for both linting, which is tied to a specific PHP version, and also for composer to work out dependencies based on platform. But it's not the greatest because we have to spin up those resources which aren't needed to fail the test run at that point. So splitting the db environment out from the web environment might be a follow-up.

This branch doesn't handle any special reporting. D.O will need to use the report somehow.

  • Mile23 committed 3aab5ee on 1266444-phpcs-coder
    Issue #1266444: Separate phpcs from core sniff requirements, check for...
Mile23’s picture

Teensy bit of refactoring so we have a generic phpcs plugin. Renamed plugins to be more generic and understandable.

Checks whether phpcs exists before proceeding in the validation plugin. This still gets a false positive because we're using the composer_force_coder plugin during codebase build.

  • Mile23 committed 25158b3 on 1266444-phpcs-coder
    Issue #1266444: phpcs plugin is generalized, use config options for core...
Mile23’s picture

So what I did in 25158b3 is:

Removed the special-case core coder sniff plugin. Everything it did is now configurable to do in the phpcs plugin.

Added a couple tests. Needs more, especially once #2744463: Add phpcs, coder 8.2.8 as --dev requirements for drupal/core happens.

phpcs can configure:

  • Whether to sniff only changed files or all files.
  • An installed_path config for adding coder's sniffs.
  • A config path which is where you expect the phpcs.xml(.dist) file.
  • A start path, which is where the tool will start sniffing if there aren't any changed files.

  • Mixologic committed 77343b9 on 1266444-phpcs-coder
    Issue #1266444: Modifies modified files to find new files too
    
  • Mixologic committed 820cf3d on 1266444-phpcs-coder
    Issue #1266444: Updates usages of get modified files to expect relative...
  • Mixologic committed 99840aa on 1266444-phpcs-coder
    Issue #1266444: Adds a method to codebase to get modified php files
    

  • Mixologic committed 573a691 on 1266444-phpcs-coder
    Issue #1266444: adds a public function to the Build Interface
    
  • Mixologic committed c6d6707 on 1266444-phpcs-coder
    Issue #1266444: stores checkstyle data in its own artifact folder so we...
Mixologic’s picture

Okay, made a bunch of tweaks to support phplinting, which ended up on this branch because I was lazy, but also had some shared concerns.

So, some changes I still think need to happen that we've talked about in IRC:

1. I think I've changed my mind on the "modified files" being the determiner for whether or not to sniff everything, as the modified files are only sniffing php files, so we wouldnt want to sniff all php if the patch only introduced javascript changes, for example. In other words, lets keep the config option and the way it works here in the branch.

2. I'd still like to remove the 'composer force coder' plugin and just have that be an additional composer.coder step with the 'options' configured. There may be an issue with installing coder for contrib modules however, since we add packages.drupal.org/8 to require the contrib module, and there exists, currently, a bug with coder at the composer facade. I think though that requiring the 8.2.8 version alleviates that due to it falling back on packagist, but its something we should make sure works , like with a test or something.

3. If we add phpcs to the default use cases (simpletest/simpletest7), we can have it run on jenkins for now, and produce output there until such time as we come up with a UI to show it on d.o.

4. We should also either find an issue, or open one with core that asks "how shall we proceed when we find codesniff failures?" Should a failed phpcs check fail the build and return "whoops" immediately? or should we go ahead and run tests.. I suppose that doesnt even really need to be worked out in advance.

I may go ahead and merge this into dev, since it doesnt really affect anything to have the extra plugins yet, even if they arent totally 'done'

Mile23’s picture

I'd still like to remove the 'composer force coder' plugin and just have that be an additional composer.coder step with the 'options' configured.

The only reason that plugin exists is because core doesn't currently require coder/phpcs. Once it does, we should only check whether it's present during the phpcs plugin step. #2744463: Add phpcs, coder 8.2.8 as --dev requirements for drupal/core

That is: We shouldn't even add an options step to composer, and only add the phpcs plugin. It will check the codebase and only run if the tool is present in core's vendor/bin/.

There may be an issue with installing coder for contrib modules however

Not all contrib has a phpcs.xml file or would even want a CS report. Examples does... :-)

If we add phpcs to the default use cases (simpletest/simpletest7), we can have it run on jenkins for now, and produce output there until such time as we come up with a UI to show it on d.o.

+1.

Full-codebase sniffs of core can take a long time, however, and might not be wanted, so let's not sniff if there are no changes worth sniffing.

I may go ahead and merge this into dev

Hang on... Let's get rid of the force-coder plugin and ensure that the policies are right.

Mile23’s picture

Assigned: Unassigned » Mile23

  • Mile23 committed 0590f86 on 1266444-phpcs-coder
    Issue #1266444: Removed composer_force_coder, improved docs for fixture/...
Mile23’s picture

Removed composer_force_coder, improved docs for fixture/demo build definitions, added policy test for phpcs to not kill a build job if it fails and will only sniff changed files, minor tweaks

Mixologic’s picture

sweet. am approving of all of the things.

Im even more inclined to merge this in, unless you've got more planned (still assigned to you)

Mile23’s picture

Status: Needs review » Needs work

Still needs contrib stuff, such as changing the config to point to module/projectname and also tests.

Mile23’s picture

Assigned: Mile23 » Unassigned
Status: Needs work » Fixed
Related issues: +#2841472: DrupalCI should return failed for poor code style Part II - php

So we merged the work here in order to fix the git added files issue here: #2597778: Install composer dependencies for contrib projects before running tests

This issue accomplished the goal of allowing a way to fail the test run based on phpcs status, for Drupal core.

Still waiting on core to catch up... :-) #2744463: Add phpcs, coder 8.2.8 as --dev requirements for drupal/core

Follow-up to add in contrib support and follow-through on other bugs in #2841472: DrupalCI should return failed for poor code style Part II - php

Status: Fixed » Closed (fixed)

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