Skip plugins and themes in internal Studio wp-cli commands like import export

[sejas]

Related issues

• https://github.com/Automattic/dotcom-forge/issues/8961
• Run commands after_wp_load wp-cli-sqlite-command#7 (comment)
• https://github.com/Automattic/dotcom-forge/issues/10059#issuecomment-2546362614

Proposed Changes

• Let's skip plugins and themes executing wp-cli for Studio internal commands.
• Keep the regular wp-cli behaviour for commands executed by the assistant

Testing Instructions

• Apply this diff to log the arguments being executed:

diff --git a/src/site-server.ts b/src/site-server.ts
index 102b650..4371b19 100644
--- a/src/site-server.ts
+++ b/src/site-server.ts
@@ -191,6 +191,8 @@ export class SiteServer {
                        wpCliArgs.push( '--skip-themes' );
                }
 
+               console.log( '\n\n\n\n wpCliArgs', wpCliArgs );
+
                // The parsing of arguments can include shell operators like `>` or `||` that the app don't support.
                const isValidCommand = wpCliArgs.every(
                        ( arg: unknown ) => typeof arg === 'string' || arg instanceof String

• Run npm start
• Create a Studio site
• Open the terminal
• Run: wp plugin install slider-revolution-search-replace --activate
• Go to the assistant tab
• Ask "what plugins do I have installed?"
• Observe the answer is correct
• Observe in the node console the --skip-plugins and --skip-themes are not present
• Go to the sync tab
• Connect a site
• Push the site
• Observe the push successfully works
• Observe in the node console the --skip-plugins and --skip-themes are added to each wp-cli command to export, get site url, etc.

Pre-merge Checklist

• Have you checked for TypeScript, React or other console errors?

[sejas]

I'm adding some parameters in case we can include the plugins when executing some specific wp-cli command, but maybe we could avoid those parameters and directly add the arguments:

wpCliArgs.push( '--skip-plugins' );
wpCliArgs.push( '--skip-themes' );

[sejas]

I also thought in adding this arguments at execute-wp-cli level, but that way we would adding the arguments also when we execute wp-cli from the CLI.

studio/vendor/wp-now/src/execute-wp-cli.ts

Line 73 in 2c27eaf

], ${ phpVar( args ) });

[wojtekn]

It fixes the issue with the plugin that was leaking PHP warnings to WP CLI commands output.

The only part I'm not convinced of is the default value. The executeWpCliCommand is a general purpose command, and it may be not clear that it skips plugins or themes internally.

I think it would be clearer if we changed includePlugins to skipPlugins and explicitly set it in all places which need to skip plugins. It would be also aligned with the WP CLI implementation.

[fredrikekelund]

I also thought in adding this arguments at execute-wp-cli level, but that way we would adding the arguments also when we execute wp-cli from the CLI.

Could you elaborate on this, @sejas?

[sejas]

I think it would be clearer if we changed includePlugins to skipPlugins and explicitly set it in all places which need to skip plugins. It would be also aligned with the WP CLI implementation.

Agreed! More often than not, magical code behavior produces more headaches.
I’ve inverted the conditions and added the flag where it’s needed (export, import, etc.).
3e99a7f

[sejas]

I also thought in adding this arguments at execute-wp-cli level, but that way we would adding the arguments also when we execute wp-cli from the CLI.

Could you elaborate on this, @sejas?

@fredrikekelund , my comment wants to highlight that server.executeWpCliCommand is the right place to add this parameter. Another option would be adding the parameter down to the [execute-wp-cli.ts](

studio/vendor/wp-now/src/execute-wp-cli.ts

Line 73 in 74f58c4

], ${ phpVar( args ) });

) function, but that would create more confusion and wouldn't add any value.

[wojtekn]

Thanks for the change @sejas , looks 100% clear now.

[fredrikekelund]

Nice initiative 👍 I agree this makes sense as a default for our hard-coded CLI calls.

The only issue I see with this is executeWPCLiInline (funny function name with that lowercase i, btw). I'm not sure which commands the Assistant can potentially suggests, but since it seems much more open ended compared to our hard-coded CLI calls, I might be inclined to not skip plugins and themes in that context.

Any thoughts, @sejas?

[fredrikekelund]

Suggested change

	includePlugins,
	includeThemes,
	includePlugins = false,
	includeThemes = false,

Nit, but I think this clarifies intent

[fredrikekelund]

I also thought in adding this arguments at execute-wp-cli level, but that way we would adding the arguments also when we execute wp-cli from the CLI.

Could you elaborate on this, @sejas?

[fredrikekelund]

Per my previous review, this is the only place I would consider not skipping plugins and themes. Do we know all the commands Assistant might suggest, and are we sure that the output of those commands couldn't be meaningfully affected by plugins or themes..?

[sejas]

@fredrikekelund Sure!, I totally agree with you, and the assistant should execute wp-cli command without any flag. Full experience. Skipping themes and plugins was my mistake in the first place.

Now we are skipping plugins and themes for Studio internal commands by passing skipPluginsAndThemes: true, and not including any flag for commands executed by the assistant.
I've added a skipPluginsAndThemes: false, to be more specific, but I can remove it if you think it adds confusion.

f4091ad

[fredrikekelund]

My bad for not looking closer at the code first 🙌 I see now that this code isn't for "general purpose" WP-CLI commands.

[sejas]

@wojtekn , @fredrikekelund , I would appreciate another review 🙏 .
Thank you!

[fredrikekelund]

LGTM and works as expected 👍