Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
18 changes: 12 additions & 6 deletions cli/commands/site/set.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { __, sprintf } from '@wordpress/i18n';
import { DEFAULT_WORDPRESS_VERSION, MINIMUM_WORDPRESS_VERSION } from 'common/constants';
import { getDomainNameValidationError } from 'common/lib/domains';
import { arePathsEqual } from 'common/lib/fs-utils';
import { siteNeedsRestart } from 'common/lib/site-needs-restart';
import {
getWordPressVersionUrl,
isValidWordPressVersion,
Expand Down Expand Up @@ -68,7 +69,7 @@ export async function runCommand(
throw new LoggerError( __( 'Site name cannot be empty.' ) );
}

if ( xdebug !== undefined && ! ( await isXdebugBetaEnabled() ) ) {
if ( xdebug === true && ! ( await isXdebugBetaEnabled() ) ) {
throw new LoggerError(
__( 'Xdebug support is a beta feature. Enable it in Studio settings first.' )
);
Expand All @@ -81,7 +82,7 @@ export async function runCommand(

const initialAppdata = await readAppdata();

if ( domain !== undefined ) {
if ( domain ) {
const existingDomainNames = initialAppdata.sites
.filter( ( s ) => s.id !== site.id )
.map( ( s ) => s.customDomain )
Expand Down Expand Up @@ -129,7 +130,13 @@ export async function runCommand(
);
}

const needsRestart = domainChanged || httpsChanged || phpChanged || wpChanged || xdebugChanged;
const needsRestart = siteNeedsRestart( {
domainChanged,
httpsChanged,
phpChanged,
wpChanged,
xdebugChanged,
} );
const oldDomain = site.customDomain;

try {
Expand All @@ -144,7 +151,7 @@ export async function runCommand(
foundSite.name = name!;
}
if ( domainChanged ) {
foundSite.customDomain = domain;
foundSite.customDomain = domain || undefined;
}
if ( httpsChanged ) {
foundSite.enableHttps = https;
Expand Down Expand Up @@ -185,7 +192,6 @@ export async function runCommand(
const phpVersion = validatePhpVersion( site.phpVersion );
const zipUrl = getWordPressVersionUrl( wp );

// Use the correct site URL to avoid corrupting WordPress URL options
let siteUrl: string | undefined;
if ( site.customDomain ) {
const protocol = site.enableHttps ? 'https' : 'http';
Expand Down Expand Up @@ -307,7 +313,6 @@ export const registerCommand = ( yargs: StudioArgv ) => {
wp: argv.wp,
xdebug: argv.xdebug,
} );
// WP-CLI leaves handles open, so we need to explicitly exit
// See: cli/lib/run-wp-cli-command.ts FIXME comment
if ( result.usedWpCli ) {
process.exit( 0 );
Expand All @@ -319,6 +324,7 @@ export const registerCommand = ( yargs: StudioArgv ) => {
const loggerError = new LoggerError( __( 'Failed to configure site' ), error );
logger.reportError( loggerError );
}
process.exit( 1 );
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice catch 👍

}
},
} );
Expand Down
1 change: 1 addition & 0 deletions cli/lib/types/wordpress-server-ipc.ts
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,7 @@ export const childMessagePm2Schema = z.object( {
export const pm2ProcessEventSchema = z.object( {
process: z.object( {
name: z.string(),
pm_id: z.number().optional(),
} ),
event: z.string(),
} );
21 changes: 20 additions & 1 deletion cli/lib/wordpress-server-manager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import { ProcessDescription } from 'cli/lib/types/pm2';
import {
ServerConfig,
childMessagePm2Schema,
pm2ProcessEventSchema,
ManagerMessagePayload,
} from 'cli/lib/types/wordpress-server-ipc';
import { Logger } from 'cli/logger';
Expand Down Expand Up @@ -125,6 +126,7 @@ export async function startWordPressServer(
await waitForReadyMessage( processDesc.pmId );
await sendMessage(
processDesc.pmId,
processName,
{
topic: 'start-server',
data: { config: serverConfig },
Expand Down Expand Up @@ -192,13 +194,15 @@ interface SendMessageOptions {

async function sendMessage(
pmId: number,
processName: string,
message: ManagerMessagePayload,
options: SendMessageOptions = {}
): Promise< unknown > {
const { maxTotalElapsedTime = PLAYGROUND_CLI_MAX_TIMEOUT, logger } = options;
const bus = await getPm2Bus();
const messageId = crypto.randomUUID();
let responseHandler: ( packet: unknown ) => void;
let processEventHandler: ( event: unknown ) => void;
let abortListener: () => void;

return new Promise( ( resolve, reject ) => {
Expand Down Expand Up @@ -228,6 +232,17 @@ async function sendMessage(
activityCheckIntervalId,
} );

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' ) );
}
};
Comment on lines +235 to +244
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.


responseHandler = ( packet: unknown ) => {
const validationResult = childMessagePm2Schema.safeParse( packet );
if ( ! validationResult.success ) {
Expand Down Expand Up @@ -274,11 +289,13 @@ async function sendMessage(
};
abortController.signal.addEventListener( 'abort', abortListener );

bus.on( 'process:event', processEventHandler );
bus.on( 'process:msg', responseHandler );

sendMessageToProcess( pmId, { ...message, messageId } ).catch( reject );
} ).finally( () => {
abortController.signal.removeEventListener( 'abort', abortListener );
bus.off( 'process:event', processEventHandler );
bus.off( 'process:msg', responseHandler );

const tracker = messageActivityTrackers.get( messageId );
Expand All @@ -299,6 +316,7 @@ export async function stopWordPressServer( siteId: string ): Promise< void > {
try {
await sendMessage(
runningProcess.pmId,
processName,
{ topic: 'stop-server', data: {} },
{ maxTotalElapsedTime: GRACEFUL_STOP_TIMEOUT }
);
Expand Down Expand Up @@ -376,6 +394,7 @@ export async function runBlueprint(
await waitForReadyMessage( processDesc.pmId );
await sendMessage(
processDesc.pmId,
processName,
{
topic: 'run-blueprint',
data: { config: serverConfig },
Expand Down Expand Up @@ -405,7 +424,7 @@ export async function sendWpCliCommand(
throw new Error( `WordPress server is not running` );
}

const result = await sendMessage( runningProcess.pmId, {
const result = await sendMessage( runningProcess.pmId, processName, {
topic: 'wp-cli-command',
data: { args },
} );
Expand Down
39 changes: 38 additions & 1 deletion cli/wordpress-server-child.ts
Original file line number Diff line number Diff line change
Expand Up @@ -344,11 +344,48 @@ const runWpCliCommand = sequential(
{ concurrent: 3, max: 10 }
);

function parsePhpError( error: unknown ): string {
if ( ! ( error instanceof Error ) ) {
return String( error );
}

const message = error.message;

// 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() }`;
}
Comment on lines +354 to +372
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious what these two conditions are based on. XDebug errors?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


// Check for generic PHP.run() failure - provide a cleaner message
if ( message.includes( 'PHP.run() failed with exit code' ) ) {
const exitCodeMatch = message.match( /exit code (\d+)/ );
const exitCode = exitCodeMatch ? exitCodeMatch[ 1 ] : 'unknown';
return `WordPress failed to start (PHP exit code ${ exitCode }). Check the site's debug.log for details.`;
}

return message;
}

function sendErrorMessage( messageId: string, error: unknown ) {
const errorResponse: ChildMessageRaw = {
originalMessageId: messageId,
topic: 'error',
errorMessage: error instanceof Error ? error.message : String( error ),
errorMessage: parsePhpError( error ),
errorStack: error instanceof Error ? error.stack : undefined,
cliArgs: lastCliArgs ?? undefined,
};
Expand Down
13 changes: 13 additions & 0 deletions common/lib/site-needs-restart.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
export interface SiteSettingChanges {
domainChanged?: boolean;
httpsChanged?: boolean;
phpChanged?: boolean;
wpChanged?: boolean;
xdebugChanged?: boolean;
}

export function siteNeedsRestart( changes: SiteSettingChanges ): boolean {
const { domainChanged, httpsChanged, phpChanged, wpChanged, xdebugChanged } = changes;

return !! ( domainChanged || httpsChanged || phpChanged || wpChanged || xdebugChanged );
}
60 changes: 60 additions & 0 deletions common/lib/tests/site-needs-restart.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
import { siteNeedsRestart } from '../site-needs-restart';

describe( 'siteNeedsRestart', () => {
it( 'returns false when no changes are provided', () => {
expect( siteNeedsRestart( {} ) ).toBe( false );
} );

it( 'returns false when all changes are false', () => {
expect(
siteNeedsRestart( {
domainChanged: false,
httpsChanged: false,
phpChanged: false,
wpChanged: false,
xdebugChanged: false,
} )
).toBe( false );
} );

it( 'returns true when domain changed', () => {
expect( siteNeedsRestart( { domainChanged: true } ) ).toBe( true );
} );

it( 'returns true when https changed', () => {
expect( siteNeedsRestart( { httpsChanged: true } ) ).toBe( true );
} );

it( 'returns true when php changed', () => {
expect( siteNeedsRestart( { phpChanged: true } ) ).toBe( true );
} );

it( 'returns true when wp changed', () => {
expect( siteNeedsRestart( { wpChanged: true } ) ).toBe( true );
} );

it( 'returns true when xdebug changed', () => {
expect( siteNeedsRestart( { xdebugChanged: true } ) ).toBe( true );
} );

it( 'returns true when multiple settings changed', () => {
expect(
siteNeedsRestart( {
phpChanged: true,
wpChanged: true,
} )
).toBe( true );
} );

it( 'returns true when one change is true among false values', () => {
expect(
siteNeedsRestart( {
domainChanged: false,
httpsChanged: false,
phpChanged: true,
wpChanged: false,
xdebugChanged: false,
} )
).toBe( true );
} );
} );
8 changes: 4 additions & 4 deletions src/components/tests/content-tab-settings.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -371,7 +371,8 @@ describe( 'ContentTabSettings', () => {

await waitFor( () => {
expect( updateSite ).toHaveBeenCalledWith(
expect.objectContaining( { phpVersion: '8.2' } )
expect.objectContaining( { phpVersion: '8.2' } ),
undefined
);
expect( stopServer ).not.toHaveBeenCalled();
expect( startServer ).not.toHaveBeenCalled();
Expand Down Expand Up @@ -441,10 +442,9 @@ describe( 'ContentTabSettings', () => {

await waitFor( () => {
expect( updateSite ).toHaveBeenCalledWith(
expect.objectContaining( { phpVersion: '8.2' } )
expect.objectContaining( { phpVersion: '8.2' } ),
undefined
);
expect( stopServer ).toHaveBeenCalled();
expect( startServer ).toHaveBeenCalled();
} );

rerenderWithProvider(
Expand Down
6 changes: 3 additions & 3 deletions src/hooks/use-site-details.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import type { Blueprint } from 'src/stores/wpcom-api';

interface SiteDetailsContext {
selectedSite: SiteDetails | null;
updateSite: ( site: SiteDetails ) => Promise< void >;
updateSite: ( site: SiteDetails, wpVersion?: string ) => Promise< void >;
sites: SiteDetails[];
setSelectedSiteId: ( selectedSiteId: string ) => void;
createSite: (
Expand Down Expand Up @@ -375,8 +375,8 @@ export function SiteDetailsProvider( { children }: SiteDetailsProviderProps ) {
[ selectedTab, setSelectedSiteId, setSelectedTab ]
);

const updateSite = useCallback( async ( site: SiteDetails ) => {
await getIpcApi().updateSite( site );
const updateSite = useCallback( async ( site: SiteDetails, wpVersion?: string ) => {
await getIpcApi().updateSite( site, wpVersion );
const updatedSites = await getIpcApi().getSiteDetails();
setSites( updatedSites );
}, [] );
Expand Down
Loading