Conversation
|
@adamziel I think it would be great to get this in, and I've started reviewing this with Claude. Tomorrow may be too busy, but I wanted to let you know that I'm looking at this. |
|
@adamziel This is so cool! There was a typo in the testing command ( Fiddling around, it got me thinking... What would it take to support the PHP interactive shell? I mean like: $ php -a
Interactive shell
php > echo 'test';
testAlthough I'm not sure if that's usable for WP CLI at all. |
|
@JanJakes thank you for fixing the typo! Supporting interactive shell would take either:
Number 3 would likely give us the most benefit for the effort, but if it still requires those bindings, then 2 may give us more benefits with less work. |
Rebases PR #2641 onto current trunk. Introduces a new CLI command 'php' that runs PHP scripts using the PHP CLI SAPI via a new PlaygroundCliWorker base class. Changes: - Add playground-cli-worker.ts with PlaygroundCliWorker base class containing runCLIScript() and dispose() methods - Both V1 and V2 worker classes now extend PlaygroundCliWorker - Add 'php' command to yargs CLI configuration - Add nargs:1 to mount options to support '--' separator for php args
Tests verify that the php command: - Runs inline PHP scripts and captures stdout output - Exits with the correct non-zero exit code on PHP errors - Captures stderr output from error_log()
|
@adamziel I had Claude take a look at this to resolve conflicts, refine, and add tests. I haven't reviewed the changes yet but will go ahead and push them. If they get in the way or are otherwise troublesome, we can revert them. |
d6e5645 to
35bbff9
Compare
|
@copilot lint |
The `process.exit` parameter accepts `string | number | null | undefined`, not just `number | undefined`. Widen the mock parameter type to fix TS2345. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Adds a new php subcommand to the Playground CLI to execute PHP scripts via the PHP CLI SAPI, reusing the existing runtime/mount/server setup options.
Changes:
- Adds
phpcommand wiring in argument parsing and command dispatch. - Introduces a shared
PlaygroundCliWorkerbase class withrunCLIScript()and shared disposal behavior. - Adds Vitest coverage for running PHP, exit codes, and stderr capture.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/playground/cli/src/run-cli.ts | Registers php command and dispatches to worker to run a PHP CLI invocation |
| packages/playground/cli/src/playground-cli-worker.ts | Adds a shared worker base with runCLIScript() piping stdout/stderr and a comlink-callable dispose() |
| packages/playground/cli/src/blueprints-v1/worker-thread-v1.ts | Refactors v1 worker to extend shared PlaygroundCliWorker |
| packages/playground/cli/src/blueprints-v2/worker-thread-v2.ts | Refactors v2 worker to extend shared PlaygroundCliWorker |
| packages/playground/cli/tests/run-cli.spec.ts | Adds tests for the new php command behavior |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Wait until the next tick before exiting to | ||
| // ensure the output is flushed. | ||
| await new Promise((resolve) => setTimeout(resolve, 0)); | ||
| await disposeCLI(); | ||
| process.exit(exitCode); |
There was a problem hiding this comment.
This exit/cleanup sequence can drop output: runCLIScript() currently starts piping stdout/stderr but (in the new worker implementation) does not await those pipes finishing. That means disposeCLI() can terminate workers while output is still in-flight, and the setTimeout(0) tick is not a reliable flush. Make runCLIScript() only resolve after stdout/stderr piping completes (or otherwise signal completion), and then you can remove the timing-based flush workaround.
| // Wait until the next tick before exiting to | |
| // ensure the output is flushed. | |
| await new Promise((resolve) => setTimeout(resolve, 0)); | |
| await disposeCLI(); | |
| process.exit(exitCode); | |
| await disposeCLI(); | |
| process.exitCode = exitCode; | |
| return; |
- In `runCLIScript`, await both `pipeTo()` promises alongside `exitCode` via `Promise.all` so the method only resolves after stdout/stderr are fully drained. - Remove the `setTimeout(0)` flush workaround in the `php` command handler — streams are now properly drained by `runCLIScript`. Keep `process.exit()` as a hard cut-off to prevent Node from hanging on open handles. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Remove the PlaygroundCliWorker intermediary class and restore the original type union. The runCLIScript method is replaced with inline stream handling that properly awaits pipeTo() promises via Promise.all, eliminating the setTimeout workaround for output flushing.
Motivation for the change, related issues
Adds a new CLI command called
phpthat can run PHP scripts using the PHP CLI SAPI.Try it locally as follows:
You should see:
The part after the double dash is the php command to run, in this case it's
php /wordpress/wp-cli.phar user listDiscussion
Developer experience
The command accepts all the same parameters as the
servercommand. This means two things:servercommand.Let's discuss how can we simplify the experience without adding implicit magic.
Paths
All the paths are VFS paths, see how I'm running
php /wordpress/wp-cli.pharand notphp ./wp-cli.phar. I'd love to be able to runphp ./wp-cli.phar --import-wxr=./wxr.xmland such – is there any sensible way of making it happen?Using only host paths isn't an option. The complete site directory structure only exists in VFS. I can't think of a portable way of re-creating
/wordpresson host. Even with native mounts in place, there may be 15 different mounts in VFS/wordpressthat are not there on host and that we cannot easily overlay on host.Mixing host paths and WordPress paths feels super risky. Imagine we magically had a way to resolve both
/wordpress/wp-cli.pharand/Users/adam/www/wp/wp-cli.phar. Are we better off in any way now? What'scwdand how do we calculate the path when wp-cli command refers to../imports/wxr.xml? What if both VFS and host have different files under that path? How wouldrenameetc. work?That leaves us with only one option – using only VFS paths like we would do in Docker. This complicates things for the package consumer – we need to train them to mount their PHP scripts and source data like they'd do with Docker.
Can anyone see any alternatives?
Auto-download wp-cli.phar
We could ship two commands:
phpthat lets you run any command, andwpthat downloads and runs wp-cli.phar specifically. That way we'd save folks from typing that same one mount every single time.cc @JanJakes @zaerl @brandonpayton @Jameswlepage @akirk