Use CLI site set command to edit site details from Studio #2360
Conversation
bcotrim
force-pushed
the
stu-1149-check-edit-site-details-from-studio
branch
from
5ad33ba to
52dbfca
Compare
fredrikekelund
left a comment
fredrikekelund
left a comment
There was a problem hiding this comment.
LGTM 👍 Sensible and manageable diff. Nice work!
| const loggerError = new LoggerError( __( 'Failed to configure site' ), error ); | ||
| logger.reportError( loggerError ); | ||
| } | ||
| process.exit( 1 ); |
| processEventHandler = ( event: unknown ) => { | ||
| const result = pm2ProcessEventSchema.safeParse( event ); | ||
| if ( ! result.success ) { | ||
| return; | ||
| } | ||
|
|
||
| if ( result.data.process.name === processName && result.data.event === 'exit' ) { | ||
| reject( new Error( 'WordPress server process exited unexpectedly' ) ); | ||
| } | ||
| }; |
There was a problem hiding this comment.
This seems sensible. Out of curiosity, did you add this because of something related to the core purpose of this PR, or was it just something that happened to come up here?
There was a problem hiding this comment.
This came up during testing this PR. When changing WP version to an incompatible one (e.g., 6.4.7 with Twenty Twenty-Five theme which requires WP 6.5+), the CLI would hang forever instead of reporting the error.
| } | ||
| } | ||
|
|
||
| export async function updateSite( |
There was a problem hiding this comment.
Aside from being exposed as an IPC function, this function is also called in from multiple places in this file. It seems like the old purpose was to:
- Write changes to the appdata file.
- Update the
SiteServerinstance itself (which might have been entirely redundant, given that we passSiteServer.detailsto this function already).
I'm curious if we really need to keep calling this function from startServer, stopServer, etc..?
There was a problem hiding this comment.
Good question, I think it deserves a closer look. I added a note on STU-1193 and will tackle it in that PR if you agree.
| // Check for WordPress critical error in HTML output | ||
| const wpDieMatch = message.match( /<div class="wp-die-message"[^>]*>([\s\S]*?)<\/div>/ ); | ||
| if ( wpDieMatch ) { | ||
| // Extract text from HTML, removing tags | ||
| const htmlContent = wpDieMatch[ 1 ]; | ||
| const textContent = htmlContent | ||
| .replace( /<[^>]+>/g, ' ' ) | ||
| .replace( /\s+/g, ' ' ) | ||
| .trim(); | ||
| if ( textContent ) { | ||
| return `WordPress error: ${ textContent }`; | ||
| } | ||
| } | ||
|
|
||
| // Check for PHP fatal error pattern | ||
| const fatalMatch = message.match( /PHP Fatal error:\s*(.+?)(?:\sin\s|$)/i ); | ||
| if ( fatalMatch ) { | ||
| return `PHP Fatal error: ${ fatalMatch[ 1 ].trim() }`; | ||
| } |
There was a problem hiding this comment.
I'm curious what these two conditions are based on. XDebug errors?
There was a problem hiding this comment.
It also related to the WordPress issue I came across during testing.
I just tried to clean the errors a bit so it's more UI friendly.
There was a problem hiding this comment.
It's a small thing, but I still love the changes to this file. This is the kind of business logic consolidation that I think will do Studio a lot of good 👍
| tlsKey: site.tlsKey, | ||
| tlsCert: site.tlsCert, |
There was a problem hiding this comment.
Note for the future: we don't need to load these details in the Studio app anymore
| } ); | ||
|
|
||
| emitter.on( 'success', () => { | ||
| console.log( `[CLI Site Editor] Command completed successfully` ); |
Related issues
Proposed Changes
cli-site-editor.tsmodule to invoke the CLI'sstudio site setcommandupdateSite()to delegate to CLI for site changes (domain, HTTPS, PHP version, WP version)edit-site-details.tsx- UI now just callsupdateSite()and lets CLI handle restart logicsite-server.tsupdateSiteDetails()since CLI handles hosts file, SSL certs, and WP updatessiteNeedsRestart()incommon/lib/so CLI and Studio UI share the same restart logicTesting Instructions
Pre-merge Checklist