Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 9 commits into from
Dec 18, 2024
Merged

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

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
59fecfd
Skip plugins and themes by default when executing wp-cli commands ins…
sejas Dec 17, 2024
ef7a900
fix lint errors
sejas Dec 17, 2024
f94e8ca
add default values to parameters
sejas Dec 18, 2024
afd4d1b
include plugins and themes on assistant wp-cli execution
sejas Dec 18, 2024
3e99a7f
invert skip condition and add param to Studio executions
sejas Dec 18, 2024
f4091ad
be specific about skipPluginsAndThemes false for cli hook and assistant
sejas Dec 18, 2024
aeb71eb
update tests
sejas Dec 18, 2024
74f58c4
fix chat context tests
sejas Dec 18, 2024
fba5ec8
Merge branch 'trunk' of github.com:Automattic/studio into add/exclude…
sejas Dec 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions src/hooks/tests/use-chat-context.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,10 +123,12 @@ describe( 'useChatContext hook', () => {
expect( getIpcApi().executeWPCLiInline ).toHaveBeenNthCalledWith( 1, {
siteId: SELECTED_SITE.id,
args: 'plugin list --format=json --status=active',
skipPluginsAndThemes: true,
} );
expect( getIpcApi().executeWPCLiInline ).toHaveBeenNthCalledWith( 2, {
siteId: SELECTED_SITE.id,
args: 'theme list --format=json',
skipPluginsAndThemes: true,
} );
expect( getIpcApi().executeWPCLiInline ).toHaveBeenCalledTimes( 2 );
} );
Expand Down Expand Up @@ -234,10 +236,12 @@ describe( 'useChatContext hook', () => {
expect( getIpcApi().executeWPCLiInline ).toHaveBeenNthCalledWith( 1, {
siteId: ANOTHER_SITE.id,
args: 'plugin list --format=json --status=active',
skipPluginsAndThemes: true,
} );
expect( getIpcApi().executeWPCLiInline ).toHaveBeenNthCalledWith( 2, {
siteId: ANOTHER_SITE.id,
args: 'theme list --format=json',
skipPluginsAndThemes: true,
} );
expect( getIpcApi().executeWPCLiInline ).toHaveBeenCalledTimes( 2 );
} );
Expand Down Expand Up @@ -288,10 +292,12 @@ describe( 'useChatContext hook', () => {
expect( getIpcApi().executeWPCLiInline ).toHaveBeenNthCalledWith( 1, {
siteId: SELECTED_SITE.id,
args: 'plugin list --format=json --status=active',
skipPluginsAndThemes: true,
} );
expect( getIpcApi().executeWPCLiInline ).toHaveBeenNthCalledWith( 2, {
siteId: SELECTED_SITE.id,
args: 'theme list --format=json',
skipPluginsAndThemes: true,
} );
expect( getIpcApi().executeWPCLiInline ).toHaveBeenCalledTimes( 2 );
} );
Expand Down Expand Up @@ -391,10 +397,12 @@ describe( 'useChatContext hook', () => {
expect( getIpcApi().executeWPCLiInline ).toHaveBeenNthCalledWith( 1, {
siteId: NEW_SITE.id,
args: 'plugin list --format=json --status=active',
skipPluginsAndThemes: true,
} );
expect( getIpcApi().executeWPCLiInline ).toHaveBeenNthCalledWith( 2, {
siteId: NEW_SITE.id,
args: 'theme list --format=json',
skipPluginsAndThemes: true,
} );
expect( getIpcApi().executeWPCLiInline ).toHaveBeenCalledTimes( 2 );
} );
Expand Down Expand Up @@ -436,10 +444,12 @@ describe( 'useChatContext hook', () => {
expect( getIpcApi().executeWPCLiInline ).toHaveBeenNthCalledWith( 1, {
siteId: SELECTED_SITE.id,
args: 'plugin list --format=json --status=active',
skipPluginsAndThemes: true,
} );
expect( getIpcApi().executeWPCLiInline ).toHaveBeenNthCalledWith( 2, {
siteId: SELECTED_SITE.id,
args: 'theme list --format=json',
skipPluginsAndThemes: true,
} );
expect( getIpcApi().executeWPCLiInline ).toHaveBeenCalledTimes( 2 );
} );
Expand Down
2 changes: 2 additions & 0 deletions src/hooks/use-chat-context.tsx
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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..?

Copy link
Member Author

@sejas sejas Dec 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ export const ChatProvider: React.FC< ChatProviderProps > = ( { children } ) => {
const { stdout, stderr } = await getIpcApi().executeWPCLiInline( {
siteId,
args: 'plugin list --format=json --status=active',
skipPluginsAndThemes: true,
} );
if ( stderr ) {
return [];
Expand All @@ -92,6 +93,7 @@ export const ChatProvider: React.FC< ChatProviderProps > = ( { children } ) => {
const { stdout, stderr } = await getIpcApi().executeWPCLiInline( {
siteId,
args: 'theme list --format=json',
skipPluginsAndThemes: true,
} );
if ( stderr ) {
return [];
Expand Down
1 change: 1 addition & 0 deletions src/hooks/use-execute-cli.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ export function useExecuteWPCLI(
const result = await getIpcApi().executeWPCLiInline( {
siteId: siteId || '',
args: args.join( ' ' ),
skipPluginsAndThemes: false,
} );

const msTime = Date.now() - startTime;
Expand Down
14 changes: 12 additions & 2 deletions src/ipc-handlers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -815,7 +815,15 @@ export async function saveOnboarding(

export async function executeWPCLiInline(
_event: IpcMainInvokeEvent,
{ siteId, args }: { siteId: string; args: string }
{
siteId,
args,
skipPluginsAndThemes = false,
}: {
siteId: string;
args: string;
skipPluginsAndThemes?: boolean;
}
): Promise< WpCliResult > {
if ( SiteServer.isDeleted( siteId ) ) {
return {
Expand All @@ -828,7 +836,9 @@ export async function executeWPCLiInline(
if ( ! server ) {
throw new Error( 'Site not found.' );
}
return server.executeWpCliCommand( args );
return server.executeWpCliCommand( args, {
skipPluginsAndThemes,
} );
}

export async function getThumbnailData( _event: IpcMainInvokeEvent, id: string ) {
Expand Down
15 changes: 12 additions & 3 deletions src/lib/import-export/export/export-database.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,10 @@ export async function exportDatabaseToFile(

// Execute the command to export directly to the temp file
const { stderr, exitCode } = await server.executeWpCliCommand(
`sqlite export ${ tempFileName } --require=/tmp/sqlite-command/command.php`
`sqlite export ${ tempFileName } --require=/tmp/sqlite-command/command.php`,
{
skipPluginsAndThemes: true,
}
);

if ( stderr ) {
Expand Down Expand Up @@ -47,7 +50,10 @@ export async function exportDatabaseToMultipleFiles(
}

const tablesResult = await server.executeWpCliCommand(
`sqlite tables --format=csv --require=/tmp/sqlite-command/command.php`
`sqlite tables --format=csv --require=/tmp/sqlite-command/command.php`,
{
skipPluginsAndThemes: true,
}
);
if ( tablesResult.stderr ) {
throw new Error( `Database export failed: ${ tablesResult.stderr }` );
Expand All @@ -69,7 +75,10 @@ export async function exportDatabaseToMultipleFiles(

// Execute the command to export directly to a temporary file in the project directory
const { stderr, exitCode } = await server.executeWpCliCommand(
`sqlite export ${ fileName } --tables=${ table } --require=/tmp/sqlite-command/command.php`
`sqlite export ${ fileName } --tables=${ table } --require=/tmp/sqlite-command/command.php`,
{
skipPluginsAndThemes: true,
}
);

if ( stderr ) {
Expand Down
10 changes: 8 additions & 2 deletions src/lib/import-export/export/exporters/default-exporter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -268,7 +268,10 @@ export class DefaultExporter extends EventEmitter implements Exporter {
}

const { stderr, stdout } = await server.executeWpCliCommand(
'plugin list --status=active,inactive --fields=name,status,version --format=json'
'plugin list --status=active,inactive --fields=name,status,version --format=json',
{
skipPluginsAndThemes: true,
}
);

if ( stderr ) {
Expand All @@ -287,7 +290,10 @@ export class DefaultExporter extends EventEmitter implements Exporter {
}

const { stderr, stdout } = await server.executeWpCliCommand(
'theme list --fields=name,status,version --format=json'
'theme list --fields=name,status,version --format=json',
{
skipPluginsAndThemes: true,
}
);

if ( stderr ) {
Expand Down
11 changes: 8 additions & 3 deletions src/lib/import-export/import/importers/importer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ abstract class BaseImporter extends EventEmitter implements Importer {
const { stderr, exitCode } = await server.executeWpCliCommand(
`sqlite import ${ sqlTempFile } --require=/tmp/sqlite-command/command.php`,
// SQLite plugin requires PHP 8+
{ targetPhpVersion: DEFAULT_PHP_VERSION }
{ targetPhpVersion: DEFAULT_PHP_VERSION, skipPluginsAndThemes: true }
);

if ( stderr ) {
Expand Down Expand Up @@ -86,7 +86,9 @@ abstract class BaseImporter extends EventEmitter implements Importer {
throw new Error( 'Site not found.' );
}

const { stdout: currentSiteUrl } = await server.executeWpCliCommand( `option get siteurl` );
const { stdout: currentSiteUrl } = await server.executeWpCliCommand( `option get siteurl`, {
skipPluginsAndThemes: true,
} );

if ( ! currentSiteUrl ) {
console.error( 'Failed to fetch site URL after import' );
Expand All @@ -96,7 +98,10 @@ abstract class BaseImporter extends EventEmitter implements Importer {
const studioUrl = `http://localhost:${ server.details.port }`;

const { stderr, exitCode } = await server.executeWpCliCommand(
`search-replace '${ currentSiteUrl.trim() }' '${ studioUrl.trim() }'`
`search-replace '${ currentSiteUrl.trim() }' '${ studioUrl.trim() }'`,
{
skipPluginsAndThemes: true,
}
);

if ( stderr ) {
Expand Down
5 changes: 4 additions & 1 deletion src/lib/import-export/tests/export/exporters/sql-exporter.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,10 @@ describe( 'SqlExporter', () => {

const siteServer = SiteServer.get( '123' );
expect( siteServer?.executeWpCliCommand ).toHaveBeenCalledWith(
'sqlite export studio-backup-db-export-2024-08-01-12-00-00.sql --require=/tmp/sqlite-command/command.php'
'sqlite export studio-backup-db-export-2024-08-01-12-00-00.sql --require=/tmp/sqlite-command/command.php',
{
skipPluginsAndThemes: true,
}
);
} );

Expand Down
2 changes: 2 additions & 0 deletions src/lib/import-export/tests/import/importer/default-importer.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,9 +80,11 @@ describe( 'JetpackImporter', () => {
'sqlite import studio-backup-sql-2024-08-01-12-00-00.sql --require=/tmp/sqlite-command/command.php';
expect( siteServer?.executeWpCliCommand ).toHaveBeenNthCalledWith( 1, expectedCommand, {
targetPhpVersion: '8.1',
skipPluginsAndThemes: true,
} );
expect( siteServer?.executeWpCliCommand ).toHaveBeenNthCalledWith( 2, expectedCommand, {
targetPhpVersion: '8.1',
skipPluginsAndThemes: true,
} );

const expectedUnlinkPath = '/path/to/studio/site/studio-backup-sql-2024-08-01-12-00-00.sql';
Expand Down
4 changes: 3 additions & 1 deletion src/lib/import-export/tests/import/importer/playground-importer.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,9 @@ describe( 'localImporter', () => {
const siteServer = SiteServer.get( mockStudioSiteId );

const expectedCommand = 'option get siteurl';
expect( siteServer?.executeWpCliCommand ).toHaveBeenNthCalledWith( 1, expectedCommand );
expect( siteServer?.executeWpCliCommand ).toHaveBeenNthCalledWith( 1, expectedCommand, {
skipPluginsAndThemes: true,
} );

expect( move ).toHaveBeenNthCalledWith(
1,
Expand Down
13 changes: 12 additions & 1 deletion src/site-server.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,13 @@ export class SiteServer {

async executeWpCliCommand(
args: string,
{ targetPhpVersion }: { targetPhpVersion?: string } = {}
{
targetPhpVersion,
skipPluginsAndThemes = false,
}: {
targetPhpVersion?: string;
skipPluginsAndThemes?: boolean;
} = {}
): Promise< WpCliResult > {
const projectPath = this.details.path;
const phpVersion = targetPhpVersion ?? this.details.phpVersion;
Expand All @@ -180,6 +186,11 @@ export class SiteServer {

const wpCliArgs = parse( args );

if ( skipPluginsAndThemes ) {
wpCliArgs.push( '--skip-plugins' );
wpCliArgs.push( '--skip-themes' );
}

// 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
Expand Down
Loading