Error handling improvements in phpGetThemeDetails - #1087
Conversation
sejas
left a comment
sejas
left a comment
There was a problem hiding this comment.
I confirm that the theme details are still working, and after changing the theme, it is saved in appData.
Great work! I'm a big fan of reducing Sentry noise.
| import SiteServerProcess from 'src/lib/site-server-process'; | ||
|
|
||
| export const themeDetailsSchema = z.object( { | ||
| name: z.string().catch( '' ), |
There was a problem hiding this comment.
I included this because it wasn't 100% clear to me if WP_Theme::get( 'Name' ) (docs) is guaranteed to return a string. It seems theoretically possible that it would also return a boolean or array. Defaulting to an empty string in those cases seemed reasonable.
|
Thanks for the review, @sejas! Just a quick note that I updated the PR description after realizing that this PR also fixes https://github.com/Automattic/dotcom-forge/issues/7291 (at least in part). No new testing is needed, but I wanted to bring it to your attention |
Co-authored-by: Antonio Sejas <antonio.sejas@automattic.com>
After looking closer at STUDIO-9J and STUDIO-9Q (the issues related to creating or starting sites), I realized that they seem to only affect older releases (circa 1.2.0). So instead of opening issues to fix them, I marked the Sentry issues as resolved. We can tackle them if it turns out later they also affect new releases. |
| $theme = wp_get_theme(); | ||
| echo json_encode([ | ||
| 'name' => $theme->get('Name'), | ||
| 'uri' => $theme->get('ThemeURI'), |
There was a problem hiding this comment.
In case it wasn't already clear, we don't actually use this data. uri is not included in the StartedSiteDetails[ 'themeDetails' ] type definition.
* Error handling improvements in `phpGetThemeDetails` * Update src/lib/php-get-theme-details.ts Co-authored-by: Antonio Sejas <antonio.sejas@automattic.com> * Small tweaks --------- Co-authored-by: Antonio Sejas <antonio.sejas@automattic.com>
2 participants
Related issues
Proposed Changes
phpGetThemeDetailsto Sentry. These exceptions indicate more general errors with the PHP-WASM environments and don't tell us anything useful.Important
This PR should also fix some of the Sentry issues connected with https://github.com/Automattic/dotcom-forge/issues/7291. The underlying cause of timeout errors varies depending on the message name (
start-server,stop-server, orrun-php).run-phpindicates an error inphpGetThemeDetailsand it'll be fixed by this PR (by simply not reporting those errors to Sentry anymore).Not reporting to Sentry may seem like a non-solution, but I believe it's viable because the fact that PHP execution fails is not always within our control (it could be due to PHP-WASM problems, or issues with the WP installation, etc). We also recently improved the logic for killing unresponsive processes in #1067.
https://github.com/Automattic/dotcom-forge/issues/7291 has grouped several timeout errors, and the ones related to
start-servermessages will not be affected by this PR. I will create new GitHub issues for tracking those.Testing Instructions
appdata-v1.jsonfile in your editorthemeDetailsvalue for your site was updated correctly inappdata-v1.jsonPre-merge Checklist