Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Part of #1299710: [meta] Automate the coding-standards part of patch review.
Problem/Motivation
Once core is cleaned up, qa.drupal.org should take code style into account when determining pass or fail. It should make sure all javascript follow our coding standards.
As with all code quality checks, we should allow individual projects to determine whether a coding standards problem fails and halts, or merely reports. This should extend to allowing for different behavior on different test types, such as issue patch versus branch.
The default behavior of the testbot will be to check if a given tool has a configuration file (in this case, .eslintrc.json), and then only perform a review, without halting the test build if it fails.
We can optionally lint only changed files, always linting the whole project if .eslintrc.json has been changed in a patch.
Drupal core currently has a core/package.json file. This file is similar to composer.json, but for packaging JavaScript. This file specifies that core uses eslint for JS linting. ESLint change notice: https://www.drupal.org/node/2274223
Proposed resolution
Implement an npm_install plugin. This plugin will have a configurable start directory, and if there is a package.json file in that directory, it will perform npn install.
Implement an eslint plugin. This plugin will have a configurable start directory. It will check whether an eslint executable exists in {start_directory}/node_packages/eslint/binary/eslint.json, and if it does, use it to lint the project.
Remaining tasks
- Determine output artifact format.
- Determine D.O integration.
Related and/or Blocking Issues:
| Comment | File | Size | Author |
|---|---|---|---|
| #8 | i2575501-8b.patch | 4.2 KB | attiks |
Comments
Comment #2
attiks CreditAttribution: attiks at Attiks commentedManual testing
Comment #3
attiks CreditAttribution: attiks at Attiks commentedWarning: do not run this, it will fail hard
✖ 2548 problems (2325 errors, 223 warnings)
The reason of the failures was because it was checking modules and themes as well, so I'm proposing to move it to the core directory, see #2575541: Move ESLint config to core folder
Comment #4
attiks CreditAttribution: attiks at Attiks commentedFirst try
Comment #5
nod_It's supposed to check all modules and themes, they need to fix their stuff.
Comment #6
attiks CreditAttribution: attiks at Attiks commented#5 I guess we need to decide if we want our code standards to be global or not, it was just decided to move phpcs.xml to core so it only applies to core. But this is off-topic for now.
Attached an updated file
Comment #7
attiks CreditAttribution: attiks at Attiks commentedNew patch using config files from core folder
Comment #8
attiks CreditAttribution: attiks at Attiks commentedAttached patch allows you to run eslint on core, only errors are reported.
To run:
./drupalci run eslintOutput can be found in
/var/www/html/artifacts/eslint.xmlon the docker containerRunning eslint only takes like 10 seconds, but installing nodejs, updating npm and installing eslint can take a couple of minutes, so ideally they will be moved to the docker image.
Comment #9
nod_What needs to happen to get that in and have it run on all core patches? I really really want it, I'd do whatever is needed to facilitate.
Comment #10
jthorson CreditAttribution: jthorson commentedTODO:
1. Add nodejs / epm / jslint to DrupalCI container definitions
2. Strip install code from this class and commit jslint class to drupalci test runner
3. Add Drupal.org integration code to PIFT code, so that new jslint jobs are triggered on each patch
May require some thought on how to best display results from multiple job types on drupal.org.
Comment #11
jthorson CreditAttribution: jthorson commentedAlternatively, as a quick fix, we can add it to the DrupalCI containers and add an eslint command to the simpletest job.
Comment #12
elachlan CreditAttribution: elachlan commentedComment #13
elachlan CreditAttribution: elachlan commentedI have added the required change to the container definition in #2580007: Add phantomjs2 to the testrunners. As they are related.
If anyone could have a look and maybe give a review that would be great :)
Comment #14
elachlan CreditAttribution: elachlan commentedWe now have a separate issue to sort out adding eslint.
#2600626: [PP-1] Ensure availability of node/npm in the testrunners
Comment #15
elachlan CreditAttribution: elachlan commentedUpdated issue with more details.
Comment #16
elachlan CreditAttribution: elachlan commentedUpdated Parent issue, previous issue #1266442: [meta] Implement automated code style checks for core was closed and marked as duplicate.
Changing priority as Parent issue is Major and this is blocking.
Comment #17
elachlan CreditAttribution: elachlan commentedComment #18
YesCT CreditAttribution: YesCT commentedComment #19
Mile23Addressing blockers for this issue: #2654650: [meta] Jobs for linting and coding standards
Comment #20
MixologicComment #21
Wim LeersThis is blocked on #2600626: [PP-1] Ensure availability of node/npm in the testrunners, which is blocked on #2609560: Base DCI containers off official containers.
Comment #22
Mile23Updating IS with new specs based on discovery.
Comment #23
MixologicIm currently working on this.
Comment #24
MixologicOkay, so eslinting is now happening as part of drupalci, and results are showing up on dispatcher.drupalci.org
Next is getting checkstyle results on drupal.org itself - #2854700: DrupalCI: Display or link to checkstyle results on Drupal.org
Comment #25
MixologicThis is mostly working, but we need to shuffle around how its executed.
Comment #27
MixologicOkay, this should now work they way nod_ and I discussed in IRC. We're letting eslint be responsible for discovering its own config now.