Honor WP_CLI_PHP and WP_CLI_PHP_ARGS in the Debian and RPM packages - #1079
Merged
Conversation
The Debian and RPM packages shipped the Phar directly as /usr/bin/wp, so it launched through its `#!/usr/bin/env php` shebang and never consulted WP_CLI_PHP or WP_CLI_PHP_ARGS. Those variables are only read by the bin/wp shell wrapper, which is not part of a Phar install, so a packaged wp-cli has never been able to select its PHP interpreter. Reported in wp-cli/wp-cli#5886. Install the Phar to /usr/share/wp-cli/wp-cli.phar and ship a small POSIX-sh launcher at /usr/bin/wp that selects the interpreter from WP_CLI_PHP (falling back to the php on PATH), exports WP_CLI_PHP_USED, and passes WP_CLI_PHP_ARGS through, mirroring the semantics of wp-cli's bin/wp. The launcher is generated inline because the deployment workflow downloads the build scripts standalone. `wp cli update` keeps working: it resolves the Phar via argv[0], so it rewrites /usr/share/wp-cli/wp-cli.phar and leaves the launcher untouched. Also replace the ls-only package verification steps with smoke tests that assert the package layout and that the launcher actually honors WP_CLI_PHP. Fixes #1078
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughDebian and RPM packages now install a shell launcher at ChangesPackage launcher and packaging
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant Launcher as /usr/bin/wp
participant PHP
participant Phar as /usr/share/wp-cli/wp-cli.phar
User->>Launcher: Invoke wp with environment variables and CLI arguments
Launcher->>PHP: Select WP_CLI_PHP and forward WP_CLI_PHP_ARGS
PHP->>Phar: Execute bundled Phar
Phar-->>User: Return command output
Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Checkov (3.3.8).github/workflows/deployment.ymlTraceback (most recent call last): Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/deployment.yml (1)
378-385: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winProve PHP arguments reach the selected interpreter.
The DEB command succeeds even if
WP_CLI_PHP_ARGSis dropped, while the RPM check only greps text. Use a temporary PHP probe that assertsWP_CLI_PHP_USED,-d, andmemory_limit=256Mbefore delegating to real PHP.
.github/workflows/deployment.yml#L378-L385: run the installed DEB launcher through the probe instead of the normal PHP binary..github/workflows/deployment.yml#L302-L311: install the RPM in the isolated runner, then run the same probe against/usr/bin/wp.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/deployment.yml around lines 378 - 385, Update the DEB check at .github/workflows/deployment.yml lines 378-385 and the RPM check at lines 302-311 to create and use the same temporary PHP probe. Configure the probe to assert WP_CLI_PHP_USED, -d, and memory_limit=256M, then delegate to the real PHP interpreter; run the installed DEB launcher through it and install the RPM in the isolated runner before invoking /usr/bin/wp through it.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In @.github/workflows/deployment.yml:
- Around line 378-385: Update the DEB check at .github/workflows/deployment.yml
lines 378-385 and the RPM check at lines 302-311 to create and use the same
temporary PHP probe. Configure the probe to assert WP_CLI_PHP_USED, -d, and
memory_limit=256M, then delegate to the real PHP interpreter; run the installed
DEB launcher through it and install the RPM in the isolated runner before
invoking /usr/bin/wp through it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 481273f8-da96-41a8-b91d-602a7406b85d
📒 Files selected for processing (4)
.github/workflows/deployment.ymlutils/wp-cli-rpm.specutils/wp-cli-updatedeb.shutils/wp-cli-updaterpm.sh
The DEB check passed even when the launcher silently dropped WP_CLI_PHP_ARGS, because `wp cli version` prints the same output either way, and the RPM check only grepped the launcher text. Replace both with a probe that stands in for PHP and asserts it received `-d memory_limit=256M` and inherited WP_CLI_PHP_USED before delegating, run through the installed DEB launcher and the extracted RPM launcher.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The Debian and RPM packages install the Phar directly as
/usr/bin/wp, so it launches through its own#!/usr/bin/env phpshebang and always runs under whateverphpis first onPATH.WP_CLI_PHPandWP_CLI_PHP_ARGSare only ever read by thebin/wpshell wrapper, which is not part of a Phar install, so a packaged wp-cli has never been able to select its PHP interpreter. This was reported downstream in wp-cli/wp-cli#5886, where it looked like a regression from wp-cli/wp-cli#5672; it is not. #5672 only madewp cli inforeport the interpreter that is genuinely running. The variable was ignored on packaged installs before and after that change.What this does
Installs the Phar to
/usr/share/wp-cli/wp-cli.phar.Ships a small POSIX-
shlauncher at/usr/bin/wpthat:WP_CLI_PHP, falling back to thephponPATH,WP_CLI_PHP_USED,WP_CLI_PHP_ARGSthrough before the Phar.This is the same logic as wp-cli's
bin/wp. The launcher is generated inline (heredoc) in both build scripts because the deployment workflow downloads each script standalone, without a repo checkout.Fixes the
php.ini usedmismatch from the report for free: the chosen interpreter loads its own configuration.Compatibility
wp cli updatekeeps working. It resolves the Phar throughargv[0], which via the launcher is/usr/share/wp-cli/wp-cli.phar, so it rewrites the Phar and leaves the launcher intact. On a root-owned install it errors "not writable" for non-root, same as today./usr/bin/wpgoes from a package-owned regular file (the Phar) to a package-owned regular file (the launcher), no file-type conflict./usr/share/wp-cli(%dir) so it is removed on uninstall.Testing
PATHphp;WP_CLI_PHPselects the interpreter;WP_CLI_PHP_ARGSword-splits into multiple args placed before the Phar;WP_CLI_PHP_USEDis exported; trailing user args are preserved.bash -n; the launcher passesdash -nand ShellCheck (the single unquoted$WP_CLI_PHP_ARGSis intentional, matchingbin/wp, and is annotated).Verifysteps now assert the package layout and, for the.deb, install it and prove the launcher actually honorsWP_CLI_PHP(a deliberately brokenWP_CLI_PHP=/bin/falsemust fail the run).Notes for reviewers
.debbehavioral smoke test doessudo dpkg -iand relies onphpbeing present on the runner; it falls back toapt-get install -f -yfor the declared dependencies. Worth confirming that stays reliable onubuntu-latest.0755(it retains a shebang, so a non-executable mode tripsscript-not-executable/non-executable-scriptlinters); it remains directly runnable, but the supported entry point is the launcher.Fixes #1078
Refs wp-cli/wp-cli#5886
Summary by CodeRabbit
New Features
wplauncher and bundled WP-CLI Phar in their expected system locations.WP_CLI_PHP.WP_CLI_PHP_ARGSis now forwarded, and the selected interpreter is exposed throughWP_CLI_PHP_USED.Bug Fixes