Skip to content

Conversation

@wojtekn
Copy link
Contributor

@wojtekn wojtekn commented Nov 20, 2024

Related issues

Proposed Changes

I propose to return an empty array when Studio gets an error from WP CLI when it tries to get a list of plugins or themes for export purposes.

Testing Instructions

  1. Export site from Studio
  2. Open package
  3. Locate and open meta.json file
  4. Confirm that file includes list of themes and plugins
  5. Confirm that regression test passes

Pre-merge Checklist

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

@wojtekn wojtekn self-assigned this Nov 20, 2024
@nightnei
Copy link
Contributor

Is see in DoD of the task Application doesn't crash and send error to Sentry when WP CLI can't get plugins or themes details, but according to the code, looks like we just do console.error, so I wonder - will it be definitely logged to Sentry, somehow under the hood? What disturbs me is it looks like we are muting errors now. Or maybe I am missing something.

Also, I am thinking - is it expected to have empty array? What is we have some plugins/theme, looks like they will be ignored, won't they?

@wojtekn
Copy link
Contributor Author

wojtekn commented Nov 20, 2024

Is see in DoD of the task Application doesn't crash and send error to Sentry when WP CLI can't get plugins or themes details, but according to the code, looks like we just do console.error, so I wonder - will it be definitely logged to Sentry, somehow under the hood? What disturbs me is it looks like we are muting errors now. Or maybe I am missing something.

console.error() was not the code that was responsible for triggering the error sent to Sentry. Without that return, code execution continued and resulted in throwing a parse error in return JSON.parse( stdout );.

Also, I am thinking - is it expected to have empty array? What is we have some plugins/theme, looks like they will be ignored, won't they?

Yes, in such a case, an entry in meta.json would be empty. It's not great, but without that, the exported package will still be usable in most cases.

@nightnei
Copy link
Contributor

nightnei commented Nov 20, 2024

console.error() was not the code that was responsible for triggering the error sent to Sentry. Without that return, code execution continued and resulted in throwing a parse error in return JSON.parse( stdout );.

I mean, here, we have "Done is: Application doesn't crash and send error to Sentry when WP CLI can't get plugins or themes details".
But with current changes we don't "send error to Sentry when WP CLI can't get plugins or themes details".
So looks like we just mute errors and won't get them in Sentry. Should we manually send them without crashing the flow?

Yes, in such a case, an entry in meta.json would be empty. It's not great, but without that, the exported package will still be usable in most cases.

I am not enough in context, but I have a personal feeling that it's better to show the errors instead of allowing using a site that doesn't have all the original data. Since it can make frustration for users, with feeling like "There should be plugins/theme, but I don't have them and I didn't know why, since I didn't have errors".
Do we know the reason for crashing and can we handle it somehow? It's inside Studio, so looks like we have all the control.

@wojtekn
Copy link
Contributor Author

wojtekn commented Nov 20, 2024

Application doesn't crash and send error to Sentry when WP CLI can't get plugins or themes details".

And I think it exactly does that now: it doesn't crash and doesn't send errors to Sentry.

I am not enough in context, but I have a personal feeling that it's better to show the errors instead of allowing using a site that doesn't have all the original data. Since it can make frustration for users, with feeling like "There should be plugins/theme, but I don't have them and I didn't know why, since I didn't have errors".

The exported package will include all plugins and themes inside wp-content/. It won't include entries for them in meta.json if the WP CLI command fails, though.

Do we know the reason for crashing and can we handle it somehow? It's inside Studio, so looks like we have all the control.

I think that a potential case is when a user installed a plugin that results in dumping some HTML when the wp cli command is executed.

@wojtekn wojtekn requested a review from a team November 20, 2024 14:49
@nightnei
Copy link
Contributor

nightnei commented Nov 20, 2024

And I think it exactly does that now: it doesn't crash and doesn't send errors to Sentry.

Oh, I see 😅 When I was reading, I applied "doesn't" only for the first part of the sentence :)

Thanks for the answers, LGTM 👍

@wojtekn wojtekn merged commit 2e79f0d into trunk Nov 21, 2024
16 checks passed
@wojtekn wojtekn deleted the update/export-error-handling branch November 21, 2024 09:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants