Repository navigation
Run composer compat as a part of grunt (precommit|prerelease) - #11983
KarunyaChavan wants to merge 7 commits into
Conversation
- Register a new compat:php Grunt task that invokes composer compat (PHPCompatibilityWP ruleset via phpcs) against changed PHP files. - Add it to precommit:php so it runs automatically in both grunt precommit and grunt prerelease.
|
Hi @KarunyaChavan! 👋 Thank you for your contribution to WordPress! 💖 It looks like this is your first pull request to No one monitors this repository for new pull requests. Pull requests must be attached to a Trac ticket to be considered for inclusion in WordPress Core. To attach a pull request to a Trac ticket, please include the ticket's full URL in your pull request description. Pull requests are never merged on GitHub. The WordPress codebase continues to be managed through the SVN repository that this GitHub repository mirrors. Please feel free to open pull requests to work on any contribution you are making. More information about how GitHub pull requests can be used to contribute to WordPress can be found in the Core Handbook. Please include automated tests. Including tests in your pull request is one way to help your patch be considered faster. To learn about WordPress' test suites, visit the Automated Testing page in the handbook. If you have not had a chance, please review the Contribute with Code page in the WordPress Core Handbook. The Developer Hub also documents the various coding standards that are followed:
Thank you, |
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. Props Bot commandsYou can trigger Props Bot actions by commenting on this PR:
|
There was a problem hiding this comment.
🟢 Approval recommended
No active review issues are reported for the PHP compatibility workflow changes.
No open issues 🎉
What changed in this PR
Adds PHP compatibility checks to the existing Grunt precommit and prerelease workflows to catch compatibility regressions automatically.
Changes:
- Registers
compat:phpto runcomposer compatagainst changed PHP files. - Runs compatibility checks between PHPStan and PHPUnit in
precommit:php, also covering prerelease. - Copies the changed-file array in
format:phpandlint:phpto prevent task arguments from mutating shared state.
| File | Description |
|---|---|
Gruntfile.js |
Adds PHP compatibility checks to the pipeline and prevents shared changed-file array mutation. |
Review effort: Balanced
lancewillett
left a comment
lancewillett
left a comment
There was a problem hiding this comment.
The compatibility task uses the existing Composer ruleset, propagates failures, and preserves the changed-file list across tasks. Static syntax and JSHint checks pass.
AI review · gpt-6
|
@desrosj Could you give a second opinion on whether adding AI review · |
lucatume
left a comment
lucatume
left a comment
There was a problem hiding this comment.
A file staged and then edited again shows as MM/AM in git status --short. The parsing at L1963/L1967 strips only the first status letter, so changedFiles.php gets M src/…. format:php ignored the resulting phpcs error; compat:php fails on it and aborts grunt precommit.
To reproduce:
echo '// staged' >> src/wp-includes/class-wp-hook.php
git add src/wp-includes/class-wp-hook.php
echo '// unstaged' >> src/wp-includes/class-wp-hook.php
git status --short # MM src/wp-includes/class-wp-hook.php
npm run grunt precommit
# Running "compat:php" task
# ERROR: The file "M src/wp-includes/class-wp-hook.php" does not exist.
# Aborted due to warnings.
git restore --staged --worktree src/wp-includes/class-wp-hook.phpGruntfile.js L1963/L1967 are outside this PR's diff, so here's the fix as a patch:
--- a/Gruntfile.js
+++ b/Gruntfile.js
@@ -1960,11 +1960,11 @@ module.exports = function(grunt) {
// Find .php files that have been modified or added.
PHPfiles = PHPfiles.filter( function( file ) {
- return /^\s*[MA]\s*.*\.php$/.test( file );
+ return /^\s*[MA]{1,2}\s+.*\.php$/.test( file );
} );
PHPfiles = PHPfiles.map( function( file ) {
- return file.replace( /^\s*[MA]\s*/, '' );
+ return file.replace( /^\s*[MA]{1,2}\s+/, '' );
} );
changedFiles = {| var args = changedFiles.php.slice(); | ||
|
|
||
| args.unshift( 'compat' ); |
There was a problem hiding this comment.
Changed files outside src/ get scanned too: 20 files under tests/ have PHPCompatibility errors/warnings CI never checks, so editing e.g. tests/phpunit/tests/option/option.php now aborts grunt precommit at compat:php.
| var args = changedFiles.php.slice(); | |
| args.unshift( 'compat' ); | |
| // phpcompat.xml.dist only scans src/; paths passed on the command line bypass its <file> selection. | |
| var files = changedFiles.php.filter( function( file ) { | |
| return 0 === file.indexOf( 'src/' ); | |
| } ); | |
| if ( changedFiles.php.length && ! files.length ) { | |
| grunt.log.writeln( 'No changed PHP files in src/. Skipping PHP compatibility checks.' ); | |
| done(); | |
| return; | |
| } | |
| var args = [ 'compat' ].concat( files ); |
There was a problem hiding this comment.
@desrosj Could you give a second opinion on whether adding
composer compatto precommit and prerelease is worthwhile? The code review is approved; the remaining question is the benefit beyond existing checks versus the extra runtime and maintenance. A timing comparison would help settle that before landing.
The PHPCS scans (both for coding standards and compatibility checks) have caching configured, so I am not too worried about the runtime aspect. In my testing the first run takes ~12-18 seconds, but every subsequent run takes ~1 second or less.
Currently, this compatibilty check is only run as a part of GitHub Actions, so I think it's a good check to surface somewhere else (especially in the final precommit verification).
From a maintainability perspective, there are currently 5 different tasks that use grunt.util.spawn({ cmd: 'composer' ... }) with each honoring the :error flag differently. It would be great to consolidate the repeated logic into a utility function. Claude came up with this as an initial suggestion:
/**
* Returns a task function that runs a Composer script.
*
* @param {string} script Composer script name from composer.json.
* @param {Object} [options]
* @param {boolean} [options.changed] Pass changed PHP files as arguments.
* @param {Function} [options.filter] Narrows the changed files (e.g. to match a ruleset's <file>).
* @param {boolean} [options.lenient] Only fail when the `:error` flag is passed.
*/
function composerTask( script, options ) {
options = options || {};
return function() {
var done = this.async();
var flags = this.flags;
var files = [];
if ( options.changed && changedFiles.php.length ) {
files = changedFiles.php.filter( options.filter || Boolean );
if ( ! files.length ) {
grunt.log.writeln( 'No applicable changed PHP files. Skipping `composer ' + script + '`.' );
return done();
}
}
grunt.util.spawn( {
cmd: 'composer',
args: [ script ].concat( files ),
opts: { stdio: 'inherit' }
}, function( error ) {
done( ! error || ( options.lenient && ! flags.error ) );
} );
};
}
function inSrc( file ) {
return 0 === file.indexOf( 'src/' );
}
grunt.registerTask( 'phpstan', 'Runs PHPStan on the entire codebase.', composerTask( 'phpstan' ) );
grunt.registerTask( 'format:php', 'Runs the code formatter on changed files.', composerTask( 'format', { changed: true, lenient: true } ) );
grunt.registerTask( 'lint:php', 'Runs the code linter on changed files.', composerTask( 'lint', { changed: true, lenient: true } ) );
grunt.registerTask( 'compat:php', 'Runs the PHPCompatibility ruleset on changed files.', composerTask( 'compat', { changed: true, filter: inSrc } ) );I don't think this needs to be done in this PR or block adding the compat:php task, but in my opinion is a worthwhile follow up.
One final thing worth noting is that this will really only be run for a Core Committer prior to committing. And as far as I know, the grunt precommit command is mostly unused at the moment largely because the setup required for the phpunit task to run correctly is not well documented and does not make use of the local Docker environment (when present and available). This falls on making the task more useful, so I that shouldn't be a blocker. But we should look at fixing and improving that separately.
| var done = this.async(); | ||
| var flags = this.flags; | ||
| var args = changedFiles.php; | ||
| var args = changedFiles.php.slice(); |
There was a problem hiding this comment.
Confirmed that the .slice() additions are needed: without them, format:php:error in prerelease turns changedFiles.php into [ 'format' ], and compat:php then skips itself.
Could all three tasks build their args the same way, with [ 'format' ].concat( changedFiles.php ) like compat:php already does, instead of copying and then using unshift? That way there's no need to remember to copy the array.
It's a shame this has to also support SVN because alternatively we could rely on --filter=GitModified and --filter=GitStaged and let PHPCS handle the detection of changed files.
There was a problem hiding this comment.
Updated both format:php and lint:php in 207c1fa to build their arguments with concat(), matching compat:php. Syntax, JSHint, and controlled sequential-task checks pass for empty, source-only, mixed, and tests-only lists; the shared array stays unchanged.
@desrosj, could you take another look and clear the changes-requested review if this addresses your remaining concern? The broader helper consolidation remains a separate follow-up.
AI review · gpt-6
This PR wires
composer compatinto the Grunt build pipeline so that potential PHP compatibility regressions are caught automatically before a commit or release.Trac ticket: https://core.trac.wordpress.org/ticket/50393
What this changes
Gruntfile.jscompat:phpGrunt task that invokescomposer compat(i.e.phpcs --standard=phpcompat.xml.dist) against the set of changed PHP files, consistent with how the existinglint:phpandformat:phptasks are structured.compat:phpto theprecommit:phptask sequence (betweenphpstanandphpunit), so it runs automatically during bothgrunt precommit(when.phpfiles are staged) andgrunt prerelease(which already includesprecommit:php).format:phpandlint:phpwhereargs.unshift()was modifying the sharedchangedFiles.phparray reference in place. Both tasks now call.slice()to operate on a copy, preventing the composer subcommand name from being prepended to the shared array between task runs.Use of AI Tools
AI assistance: Yes
Model(s): GPT-5-mini
Used for: Surfacing the
changedFiles.phpmutation bug and verifying optimality of patch.