Update WP CLI error handling #745

wojtekn · 2024-12-17T07:28:59Z

Related issues

Fixes https://github.com/Automattic/dotcom-forge/issues/8961

Proposed Changes

The Studio Assistant uses WP CLI to get the site's context data. When something is broken on the site's side, and the CLI command produces non-JSON output, the error is caught and logged to the browser console and Sentry. It's not helpful for the user as the user doesn't have access to the browser console log and a message itself isn't too useful. I propose to remove this logger line.

We might remove Sentry logging, as those are not really helpful, too:

https://a8c.sentry.io/issues/5243457569/?project=4506612776501248&query=is%3Aunresolved%20issue.priority%3A%5Bhigh%2C%20medium%5D&referrer=issue-stream&statsPeriod=14d&stream_index=16

I also propose to better catch errors received from the WP CLI when Studio fetches theme and plugin lists for meta.json purposes, and mark the export or push as failed with a proper message.

Testing Instructions

Warning

The PR depends on Automattic/wp-cli-sqlite-command#9

Push

Create a new local Studio site.
Open site in Terminal
Install and activate the plugin:

wp plugin install slider-revolution-search-replace
wp plugin activate slider-revolution-search-replace

Connect the site to one of your testing WPCOM sites.
Initiate the push operation.
Confirm there is a dialog with descriptive error and push is marked as failed

Export

Use the site from previous test
Navigate to Import / Export tab
Export entire site
Confirm there is a dialog with descriptive error

Browser console error

Open Studio's browser console
Switch between sites and tabs
Confirm there is no error in the browser console

Pre-merge Checklist

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

sejas

I confirm that after this PR, the export entire site works for sites with slider-revolution-search-replace which failed before, but it produces an empty meta.json . Check the image:

Confirm there is no error related to push

Pushing to a WPcom site is still failing, so I couldn't complete the testing instructions:

Error occurred in handler for 'exportSiteToPush': Error: Cannot execute wp-cli command with arguments: sqlite export <br />
<b>Warning</b>:  session_start(): Cannot start session when headers already sent in <b>/var/www/html/wp-content/plugins/slider-revolution-search-replace/admin/class-search-replace.php</b> on line <b>17</b><br />
wp_users.sql --tables=<br />
<b>Warning</b>:  session_start(): Cannot start session when headers already sent in <b>/var/www/html/wp-content/plugins/slider-revolution-search-replace/admin/class-search-replace.php</b> on line <b>17</b><br />
wp_users --require=/tmp/sqlite-command/command.php
    at SiteServer.<anonymous> (/Users/macbookpro/Documents/projects-m3.nosync/studio/.webpack/main/index.js:223235:23)
    at Generator.next (<anonymous>)
    at /Users/macbookpro/Documents/projects-m3.nosync/studio/.webpack/main/index.js:223061:71
    at new Promise (<anonymous>)
    at __webpack_modules__../src/site-server.ts.__awaiter (/Users/macbookpro/Documents/projects-m3.nosync/studio/.webpack/main/index.js:223057:12)
    at SiteServer.executeWpCliCommand (/Users/macbookpro/Documents/projects-m3.nosync/studio/.webpack/main/index.js:223224:16)
    at /Users/macbookpro/Documents/projects-m3.nosync/studio/.webpack/main/index.js:218221:55
    at Generator.next (<anonymous>)
    at fulfilled (/Users/macbookpro/Documents/projects-m3.nosync/studio/.webpack/main/index.js:218161:58)
Backup created at: /var/folders/_x/rbv26n3925q_01dbs4jzd8t80000gn/T/com.wordpress.studio/site_0c3e6050-2ab2-42d9-8094-34b35763629c.tar.gz

sejas · 2024-12-17T15:16:03Z

src/lib/import-export/export/exporters/default-exporter.ts

+		try {
+			return JSON.parse( stdout );
+		} catch ( error ) {
+			return [];
+		}


I'm not totally convinced that making this error silent is the best solution. Maybe we should report it to Sentry?, or to the user? Informing the user we couldn't grab the plugins/themes information.

It wouldn't help to pass it to Sentry, as we couldn't do much without knowing what PHP code caused the incorrect WP CLI response.

Informing the user we couldn't grab the plugins/themes information.

Would you fail the import entirely in such a case?

I thought about that more, and it may make sense to fail the import indeed. If the 3rd party system depended on the correct meta.json file, the package produced in such a case would be considered invalid.

wojtekn · 2024-12-17T15:45:57Z

Pushing to a WPcom site is still failing, so I couldn't complete the testing instructions:

The warning included with WP CLI commands leaks to the following command, and the command that gets a list of database tables is affected. We should improve the error message so that users can look into logs to see what causes the issue. I will take https://github.com/Automattic/dotcom-forge/issues/10143 to improve that part.

wojtekn · 2024-12-18T08:03:12Z

@sejas per our discussion, I've changed the approach to fail push/export when Studio can't generate a list of themes or plugins. I've updated the description to account for the changes. The error dialog will bu further improve under https://github.com/Automattic/dotcom-forge/issues/10143.

wojtekn · 2024-12-18T08:12:13Z

src/lib/import-export/export/export-database.ts

+	try {
+		tables = JSON.parse( tablesResult.stdout );
+	} catch ( error ) {
+		console.error(


This goes to the app log.

wojtekn · 2024-12-18T08:12:32Z

src/lib/import-export/export/export-database.ts

+		console.error(
+			`Could not get list of database tables. The WP CLI output: ${ tablesResult.stdout }`
+		);
+		throw new Error( 'Could not get list of database tables to export.' );


And this is the message that would end up in the dialog displayed to the user.

wojtekn · 2024-12-18T08:14:36Z

src/lib/import-export/export/export-database.ts


 	const tablesResult = await server.executeWpCliCommand(
-		`sqlite tables --format=csv --require=/tmp/sqlite-command/command.php`
+		`sqlite tables --format=json --require=/tmp/sqlite-command/command.php`


Switching to JSON, we can easily validate the output. If HTML or text content leaks into the command output, it will always produce invalid JSON.

wojtekn · 2024-12-18T08:15:15Z

src/hooks/use-chat-context.tsx

 		const data = JSON.parse( stdout );
 		return data?.map( ( item: { name: string } ) => item.name ) || [];
 	} catch ( error ) {
-		console.error( error, stdout );


This was logging to the browser console, but the user doesn't have access to that.

sejas

Works as expected and I'm able to export a health site correctly.
I like that now we are stopping the export process if a wp-cli command fails 💪 .
That way we can keep improving edge cases and keep evolving our features.

Great addition of console errors and "popup" messages to the user.

sejas · 2024-12-18T11:31:37Z

src/hooks/sync-sites/use-sync-push.ts

+					message: __(
+						'An error occurred while pushing the site. If this problem persists, please contact support.'
+					),


That's a good call.

When I was introducing the error message, I was looking at it from the perspective of a site export step (as a part of the whole push process). However, from the user's perspective I think it makes to use the error message you have proposed here.

sejas · 2024-12-18T11:54:47Z

src/lib/import-export/export/export-database.ts

+	let tables;
+
+	try {
+		tables = JSON.parse( tablesResult.stdout );


After applying Automattic/wp-cli-sqlite-command#9 it worked as expected, producing the output:

[ 'wp_users', 'wp_usermeta', 'wp_termmeta', 'wp_terms', 'wp_term_taxonomy', 'wp_term_relationships', 'wp_commentmeta', 'wp_comments', 'wp_links', 'wp_options', 'wp_postmeta', 'wp_posts' ]

Very smart! 💡

sejas · 2024-12-18T11:55:57Z

src/lib/import-export/tests/export/exporters/default-exporter.test.ts

 	} );

-	it( 'should not fail when can not get plugin or theme details', async () => {
+	it( 'should fail when can not get plugin or theme details', async () => {


sejas · 2024-12-18T11:58:51Z

@sejas per our discussion, I've changed the approach to fail push/export when Studio can't generate a list of themes or plugins. I've updated the description to account for the changes. The error dialog will bu further improve under https://github.com/Automattic/dotcom-forge/issues/10143.

Thanks for following this approach 🙏

wojtekn added 3 commits December 17, 2024 08:04

Avoid logging site context exception

d3bec6a

Handle theme and plugin list errors gracefully

cf3999d

Bring back Sentry log

150969a

wojtekn requested a review from a team December 17, 2024 07:29

wojtekn self-assigned this Dec 17, 2024

sejas reviewed Dec 17, 2024

View reviewed changes

wojtekn added 5 commits December 17, 2024 17:46

Change table list command format to json for better error handling

3336eab

Ensure export fails when it can't get theme or plugin lists

b0dde1e

Display underlying push error

eacb0ff

Fix test according to latest changes

ec24a92

Fix lint issue

85350ad

wojtekn commented Dec 18, 2024

View reviewed changes

sejas approved these changes Dec 18, 2024

View reviewed changes

wojtekn merged commit 4533c37 into trunk Dec 18, 2024
9 checks passed

wojtekn deleted the update/wp-cli-error-handling branch December 18, 2024 15:25

Update WP CLI error handling #745

Update WP CLI error handling #745

Uh oh!

Conversation

wojtekn commented Dec 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Related issues

Proposed Changes

Testing Instructions

Push

Export

Browser console error

Pre-merge Checklist

Uh oh!

sejas left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wojtekn commented Dec 17, 2024

Uh oh!

wojtekn commented Dec 18, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sejas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sejas commented Dec 18, 2024

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

wojtekn commented Dec 17, 2024 •

edited

Loading

sejas left a comment •

edited

Loading