Reworked run Command Parsing (#50559)

Removes the manual command parsing in favor of letting Docker handle it the way that it normally would. It adds double dash support to pass options to the container and removes the second shell that was further handling any quotes and shell tokens.
WordPress · May 23, 2023 · 9c9fe12 · 9c9fe12
1 parent 447126e
commit 9c9fe12
Show file tree

Hide file tree

Showing 7 changed files with 121 additions and 100 deletions.
diff --git a/.github/workflows/unit-test.yml b/.github/workflows/unit-test.yml
@@ -171,12 +171,12 @@ jobs:
 
             - name: Docker container debug information
               run: |
-                  npm run wp-env run tests-mysql "mysql --version"
-                  npm run wp-env run tests-wordpress "php --version"
-                  npm run wp-env run tests-wordpress "php -m"
-                  npm run wp-env run tests-wordpress "php -i"
-                  npm run wp-env run tests-wordpress "/var/www/html/wp-content/plugins/gutenberg/vendor/bin/phpunit --version"
-                  npm run wp-env run tests-wordpress "locale -a"
+                  npm run wp-env run tests-mysql mysql -- --version
+                  npm run wp-env run tests-wordpress php -- --version
+                  npm run wp-env run tests-wordpress php -m
+                  npm run wp-env run tests-wordpress php -i
+                  npm run wp-env run tests-wordpress /var/www/html/wp-content/plugins/gutenberg/vendor/bin/phpunit -- --version
+                  npm run wp-env run tests-wordpress locale -a
 
             - name: Running single site unit tests
               if: ${{ ! matrix.multisite }}

diff --git a/package.json b/package.json
@@ -272,7 +272,7 @@
 		"fixtures:generate": "cross-env GENERATE_MISSING_FIXTURES=y npm run test:unit test/integration/full-content/ && npm run format test/integration/fixtures/blocks/*.json",
 		"fixtures:regenerate": "npm-run-all fixtures:clean fixtures:generate",
 		"format": "wp-scripts format",
-		"format:php": "wp-env run --env-cwd=\"wp-content/plugins/gutenberg\" cli composer run-script format",
+		"format:php": "wp-env run --env-cwd='wp-content/plugins/gutenberg' cli composer run-script format",
 		"lint": "concurrently \"npm run lint:lockfile\" \"npm run lint:tsconfig\" \"npm run lint:js\" \"npm run lint:pkg-json\" \"npm run lint:css\"",
 		"lint:css": "wp-scripts lint-style \"**/*.scss\"",
 		"lint:css:fix": "npm run lint:css -- --fix",
@@ -282,7 +282,7 @@
 		"lint:tsconfig": "node ./bin/validate-tsconfig.mjs",
 		"lint:md:docs": "wp-scripts lint-md-docs",
 		"prelint:php": "wp-env run --env-cwd='wp-content/plugins/gutenberg' cli composer update --no-interaction",
-		"lint:php": "wp-env run --env-cwd=\"wp-content/plugins/gutenberg\" cli composer run-script lint",
+		"lint:php": "wp-env run --env-cwd='wp-content/plugins/gutenberg' cli composer run-script lint",
 		"lint:pkg-json": "wp-scripts lint-pkg-json . 'packages/*/package.json'",
 		"native": "npm run --prefix packages/react-native-editor",
 		"other:changelog": "node ./bin/plugin/cli.js changelog",
@@ -312,15 +312,15 @@
 		"test:performance": "wp-scripts test-e2e --config packages/e2e-tests/jest.performance.config.js",
 		"test:performance:debug": "wp-scripts --inspect-brk test-e2e --runInBand --no-cache --verbose --config packages/e2e-tests/jest.performance.config.js --puppeteer-devtools",
 		"test:php": "npm-run-all lint:php test:unit:php",
-		"test:php:watch": "wp-env run composer run-script test:watch",
+		"test:php:watch": "wp-env run --env-cwd='wp-content/plugins/gutenberg' tests-cli composer run-script test:watch",
 		"test:unit": "wp-scripts test-unit-js --config test/unit/jest.config.js",
 		"test:unit:date": "bash ./bin/unit-test-date.sh",
 		"test:unit:debug": "wp-scripts --inspect-brk test-unit-js --runInBand --no-cache --verbose --config test/unit/jest.config.js ",
 		"test:unit:profile": "wp-scripts --cpu-prof test-unit-js --runInBand --no-cache --verbose --config test/unit/jest.config.js ",
 		"pretest:unit:php": "wp-env start",
-		"test:unit:php": "wp-env run --env-cwd=\"wp-content/plugins/gutenberg\" tests-wordpress vendor/bin/phpunit -c phpunit.xml.dist --verbose",
+		"test:unit:php": "wp-env run --env-cwd='wp-content/plugins/gutenberg' tests-wordpress vendor/bin/phpunit -c phpunit.xml.dist --verbose",
 		"pretest:unit:php:multisite": "wp-env start",
-		"test:unit:php:multisite": "wp-env run --env-cwd=\"wp-content/plugins/gutenberg\" tests-wordpress vendor/bin/phpunit -c phpunit/multisite.xml --verbose",
+		"test:unit:php:multisite": "wp-env run --env-cwd='wp-content/plugins/gutenberg' tests-wordpress vendor/bin/phpunit -c phpunit/multisite.xml --verbose",
 		"test:unit:update": "npm run test:unit -- --updateSnapshot",
 		"test:unit:watch": "npm run test:unit -- --watch",
 		"wp-env": "wp-env"

diff --git a/packages/env/CHANGELOG.md b/packages/env/CHANGELOG.md
@@ -2,8 +2,13 @@
 
 ## Unreleased
 
+### Breaking Change
+
+-   Rework`run` command to resolve bugs with non-quoted commands. As a consequence it is no longer possible to pass your entire command to `wp-env` wrapped in double-quotes. While `npx wp-env run cli wp help` will still work, `npx wp-env run cli "wp help"` will not. If you are currently escaping any quotes you will need to review those commands and ensure they are compatible with this update.
+
 ### Enhancement
 
+-   Support using double dashes in `wp-env run ...` to pass arguments that would otherwise be consumed by `wp-env`. For example, while normally `--help` would provide the `wp-env` help text, if you use `npx wp-env run cli php -- --help` you will see the PHP help text.
 -   Validate whether or not config options exist to prevent accidentally including ones that don't.
 
 ## 7.0.0 (2023-05-10)

diff --git a/packages/env/README.md b/packages/env/README.md
@@ -319,40 +319,38 @@ Options:
   --scripts  Execute any configured lifecycle scripts. [boolean] [default: true]
 ```
 
-### `wp-env run [container] [command]`
+### `wp-env run <container> [command...]`
 
-The run command can be used to open shell sessions or invoke WP-CLI commands.
+The run command can be used to open shell sessions, invoke WP-CLI commands, or run any arbitrary commands inside of a container.
 
-<div class="callout callout-alert">In some cases, <code>wp-env</code> may consume options that you are attempting to pass to
-the container. This happens with options that <code>wp-env</code> has already declared,
-such as <code>--env-cwd</code>, <code>--debug</code>, <code>--help</code>, and <code>--version</code>. When this happens, you should fall
-back to using quotation marks; <code>wp-env</code> considers everything inside the
-quotation marks to be command argument.
-
-For example, to ask <code>WP-CLI</code> for its help text:
-
-<pre>sh
-<code class="language-sh">wp-env run cli "wp --help"</code></pre>
-
-Without the quotation marks, <code>wp-env</code> will print its own help text instead of
-passing it to the container. If you experience any problems where the command
-is not being passed correctly, fall back to using quotation marks.
+<div class="callout callout-alert">
+<p>
+In some cases <code class="language-sh">wp-env run</code> may conflict with options that you are passing to the container.
+When this happens, <code class="language-sh">wp-env</code> will treat the option as its own and take action accordingly.
+For example, if you try <code class="language-sh">wp-env run cli php --help</code>, you will receive the <code class="language-sh">wp-env</code> help text.
+</p>
 
+<p>
+You can get around this by passing any conflicting options after a double dash. <code class="language-sh">wp-env</code> will not process anything after
+the double dash and will simply pass it on to the container. To get the PHP help text you would use <code class="language-sh">wp-env run cli php -- --help</code>.
+</p>
 </div>
 
 ```sh
-wp-env run <container> [command..]
+wp-env run <container> [command...]
 
-Runs an arbitrary command in one of the underlying Docker containers. The
-"container" param should reference one of the underlying Docker services like
-"development", "tests", or "cli". To run a wp-cli command, use the "cli" or
-"tests-cli" service. You can also use this command to open shell sessions like
-bash and the WordPress shell in the WordPress instance. For example, `wp-env run
-cli bash` will open bash in the development WordPress instance.
+Runs an arbitrary command in one of the underlying Docker containers. A double
+dash can be used to pass arguments to the container without parsing them. This
+is necessary if you are using an option that is defined below. You can use
+`bash` to open a shell session and both `composer` and `phpunit` are available
+in all WordPress and CLI containers. WP-CLI is also available in the CLI
+containers.
 
 Positionals:
-  container  The container to run the command on.            [string] [required]
-  command    The command to run.                           [array] [default: []]
+  container  The Docker service to run the command on.
+              [string] [required] [choices: "mysql", "tests-mysql", "wordpress",
+                   "tests-wordpress", "cli", "tests-cli", "composer", "phpunit"]
+  command    The command to run.                                      [required]
 
 Options:
   --debug    Enable debug output.                     [boolean] [default: false]

diff --git a/packages/env/lib/cli.js b/packages/env/lib/cli.js
@@ -13,6 +13,10 @@ const { execSync } = require( 'child_process' );
  */
 const env = require( './env' );
 const parseXdebugMode = require( './parse-xdebug-mode' );
+const {
+	RUN_CONTAINERS,
+	validateRunContainer,
+} = require( './validate-run-container' );
 
 // Colors.
 const boldWhite = chalk.bold.white;
@@ -99,9 +103,12 @@ module.exports = function cli() {
 		default: false,
 	} );
 
-	// Make sure any unknown arguments are passed to the command as arguments.
-	// This allows options to be passed to "run" without being quoted.
-	yargs.parserConfiguration( { 'unknown-options-as-args': true } );
+	yargs.parserConfiguration( {
+		// Treats unknown options as arguments for commands to deal with instead of discarding them.
+		'unknown-options-as-args': true,
+		// Populates '--' in the command options with arguments after the double dash.
+		'populate--': true,
+	} );
 
 	yargs.command(
 		'start',
@@ -184,8 +191,8 @@ module.exports = function cli() {
 		'Displays the latest logs for the e2e test environment without watching.'
 	);
 	yargs.command(
-		'run <container> [command..]',
-		'Runs an arbitrary command in one of the underlying Docker containers. The "container" param should reference one of the underlying Docker services like "development", "tests", or "cli". To run a wp-cli command, use the "cli" or "tests-cli" service. You can also use this command to open shell sessions like bash and the WordPress shell in the WordPress instance. For example, `wp-env run cli bash` will open bash in the development WordPress instance. When using long commands with arguments and quotation marks, you need to wrap the "command" param in quotation marks. For example: `wp-env run tests-cli "wp post create --post_type=page --post_title=\'Test\'"` will create a post on the tests WordPress instance.',
+		'run <container> [command...]',
+		'Runs an arbitrary command in one of the underlying Docker containers. A double dash can be used to pass arguments to the container without parsing them. This is necessary if you are using an option that is defined below. You can use `bash` to open a shell session and both `composer` and `phpunit` are available in all WordPress and CLI containers. WP-CLI is also available in the CLI containers.',
 		( args ) => {
 			args.option( 'env-cwd', {
 				type: 'string',
@@ -196,7 +203,10 @@ module.exports = function cli() {
 			} );
 			args.positional( 'container', {
 				type: 'string',
-				describe: 'The container to run the command on.',
+				describe:
+					'The underlying Docker service to run the command on.',
+				choices: RUN_CONTAINERS,
+				coerce: validateRunContainer,
 			} );
 			args.positional( 'command', {
 				type: 'array',

diff --git a/packages/env/lib/commands/run.js b/packages/env/lib/commands/run.js
@@ -10,7 +10,6 @@ const path = require( 'path' );
  */
 const initConfig = require( '../init-config' );
 const getHostUser = require( '../get-host-user' );
-const { ValidationError } = require( '../config' );
 
 /**
  * @typedef {import('../config').WPConfig} WPConfig
@@ -22,73 +21,42 @@ const { ValidationError } = require( '../config' );
  * @param {Object}   options
  * @param {string}   options.container The Docker container to run the command on.
  * @param {string[]} options.command   The command to run.
+ * @param {string[]} options.'--'      Any arguments that were passed after a double dash.
  * @param {string}   options.envCwd    The working directory for the command to be executed from.
  * @param {Object}   options.spinner   A CLI spinner which indicates progress.
  * @param {boolean}  options.debug     True if debug mode is enabled.
  */
 module.exports = async function run( {
 	container,
 	command,
+	'--': doubleDashedArgs,
 	envCwd,
 	spinner,
 	debug,
 } ) {
-	validateContainerExistence( container );
-
 	const config = await initConfig( { spinner, debug } );
 
-	command = command.join( ' ' );
+	// Include any double dashed arguments in the command so that we can pass them to Docker.
+	// This lets users pass options that the command defines without them being parsed.
+	if ( Array.isArray( doubleDashedArgs ) ) {
+		command.push( ...doubleDashedArgs );
+	}
 
 	// Shows a contextual tip for the given command.
-	showCommandTips( command, container, spinner );
+	const joinedCommand = command.join( ' ' );
+	showCommandTips( joinedCommand, container, spinner );
 
 	await spawnCommandDirectly( config, container, command, envCwd, spinner );
 
-	spinner.text = `Ran \`${ command }\` in '${ container }'.`;
+	spinner.text = `Ran \`${ joinedCommand }\` in '${ container }'.`;
 };
 
-/**
- * Validates the container option and throws if it is invalid.
- *
- * @param {string} container The Docker container to run the command on.
- */
-function validateContainerExistence( container ) {
-	// Give better errors for containers that we have removed.
-	if ( container === 'phpunit' ) {
-		throw new ValidationError(
-			"The 'phpunit' container has been removed. Please use 'wp-env run tests-cli --env-cwd=wp-content/path/to/plugin phpunit' instead."
-		);
-	}
-	if ( container === 'composer' ) {
-		throw new ValidationError(
-			"The 'composer' container has been removed. Please use 'wp-env run cli --env-cwd=wp-content/path/to/plugin composer' instead."
-		);
-	}
-
-	// Provide better error output than Docker's "service does not exist" messaging.
-	const validContainers = [
-		'mysql',
-		'tests-mysql',
-		'wordpress',
-		'tests-wordpress',
-		'cli',
-		'tests-cli',
-	];
-	if ( ! validContainers.includes( container ) ) {
-		throw new ValidationError(
-			`The '${ container }' container does not exist. Valid selections are: ${ validContainers.join(
-				', '
-			) }`
-		);
-	}
-}
-
 /**
  * Runs an arbitrary command on the given Docker container.
  *
  * @param {WPConfig} config    The wp-env configuration.
  * @param {string}   container The Docker container to run the command on.
- * @param {string}   command   The command to run.
+ * @param {string[]} command   The command to run.
  * @param {string}   envCwd    The working directory for the command to be executed from.
  * @param {Object}   spinner   A CLI spinner which indicates progress.
  */
@@ -108,31 +76,30 @@ function spawnCommandDirectly( config, container, command, envCwd, spinner ) {
 		envCwd
 	);
 
-	const isTTY = process.stdout.isTTY;
 	const composeCommand = [
 		'-f',
 		config.dockerComposeConfigPath,
 		'exec',
-		! isTTY ? '-T' : '',
 		'-w',
 		envCwd,
 		'--user',
 		hostUser.fullUser,
-		container,
-		...command.split( ' ' ), // The command will fail if passed as a complete string.
 	];
 
+	if ( ! process.stdout.isTTY ) {
+		composeCommand.push( '-T' );
+	}
+
+	composeCommand.push( container, ...command );
+
 	return new Promise( ( resolve, reject ) => {
 		// Note: since the npm docker-compose package uses the -T option, we
 		// cannot use it to spawn an interactive command. Thus, we run docker-
 		// compose on the CLI directly.
 		const childProc = spawn(
 			'docker-compose',
 			composeCommand,
-			{
-				stdio: 'inherit',
-				shell: true,
-			},
+			{ stdio: 'inherit' },
 			spinner
 		);
 		childProc.on( 'error', reject );
@@ -153,17 +120,17 @@ function spawnCommandDirectly( config, container, command, envCwd, spinner ) {
  * bash) may have weird behavior (exit with ctrl-d instead of ctrl-c or ctrl-z),
  * so we want the user to have that information without having to ask someone.
  *
- * @param {string} command   The command for which to show a tip.
- * @param {string} container The container the command will be run on.
- * @param {Object} spinner   A spinner object to show progress.
+ * @param {string} joinedCommand The command for which to show a tip joined by spaces.
+ * @param {string} container     The container the command will be run on.
+ * @param {Object} spinner       A spinner object to show progress.
  */
-function showCommandTips( command, container, spinner ) {
-	if ( ! command.length ) {
+function showCommandTips( joinedCommand, container, spinner ) {
+	if ( ! joinedCommand.length ) {
 		return;
 	}
 
-	const tip = `Starting '${ command }' on the ${ container } container. ${ ( () => {
-		switch ( command ) {
+	const tip = `Starting '${ joinedCommand }' on the ${ container } container. ${ ( () => {
+		switch ( joinedCommand ) {
 			case 'bash':
 				return 'Exit bash with ctrl-d.';
 			case 'wp shell':

diff --git a/packages/env/lib/validate-run-container.js b/packages/env/lib/validate-run-container.js
@@ -0,0 +1,41 @@
+'use strict';
+
+/**
+ * A list of the containers that we can use `run` on.
+ */
+const RUN_CONTAINERS = [
+	'mysql',
+	'tests-mysql',
+	'wordpress',
+	'tests-wordpress',
+	'cli',
+	'tests-cli',
+];
+
+/**
+ * Custom parsing and validation for the "run" command's container argument.
+ *
+ * @param {string} value The user-set container.
+ *
+ * @return {string} The container name to use.
+ */
+function validateRunContainer( value ) {
+	// Give special errors for deprecated containers.
+	if ( value === 'phpunit' ) {
+		throw new Error(
+			"The 'phpunit' container has been removed. Please use 'wp-env run tests-cli --env-cwd=wp-content/path/to/plugin phpunit' instead."
+		);
+	}
+	if ( value === 'composer' ) {
+		throw new Error(
+			"The 'composer' container has been removed. Please use 'wp-env run cli --env-cwd=wp-content/path/to/plugin composer' instead."
+		);
+	}
+
+	return value;
+}
+
+module.exports = {
+	RUN_CONTAINERS,
+	validateRunContainer,
+};