Site PHP runtime & File Access options - #3786
Conversation
…untime, and surface it in site status
…lt runtime, and keep status JSON labels consistent
…e-switch-ui-and-cli # Conflicts: # apps/studio/src/migrations/index.ts
📊 Performance Test ResultsComparing 179150d vs trunk app-size
site-editor
site-startup
Results are median values from multiple test runs. Legend: 🟢 Improvement (faster) | 🔴 Regression (slower) | ⚪ No change (<50ms diff) |
fredrikekelund
left a comment
There was a problem hiding this comment.
I need to do some more testing here, but leaving a review with an initial round of feedback. Overall, this is looking really good, but the two bigger questions I think we should still tackle are:
- Improve the metrics tracking
- Remove the beta feature
| { selectedRuntime === SITE_RUNTIME_PLAYGROUND | ||
| ? __( 'The sandbox can only access the site directory.' ) | ||
| : usedFileAccess === SITE_FILE_ACCESS_ALL_FILES | ||
| ? __( 'PHP can access any file your user account can access.' ) | ||
| : __( "Restricts the site's file access to the site directory." ) } |
There was a problem hiding this comment.
Let's avoid the nested ternary
| { selectedRuntime === SITE_RUNTIME_PLAYGROUND | ||
| ? __( 'The sandbox can only access the site directory.' ) | ||
| : usedFileAccess === SITE_FILE_ACCESS_ALL_FILES | ||
| ? __( 'PHP can access any file your user account can access.' ) |
There was a problem hiding this comment.
| ? __( 'PHP can access any file your user account can access.' ) | |
| ? __( 'PHP can access any file on your system.' ) |
The existing label is not incorrect – I was just a little bit confused by the wording at first. I understand that "your user account" refers to my operating system user account, but it seems like a less discussed concept these days…
On the other hand, "your system" could arguably mean "the system accessible to your user account". Above all, I think it's more immediately understood.
| <div className="grid grid-cols-2 gap-4 mt-4"> | ||
| <div className="flex flex-col gap-1.5 leading-4"> | ||
| <label className="font-semibold" htmlFor="php-runtime-select"> | ||
| { __( 'PHP runtime' ) } | ||
| </label> |
There was a problem hiding this comment.
We should have a Learn more link somewhere in here. That can be a follow-up, though. I added RSM-4363 to track
| export async function recordSiteRuntimeUsage( site: SiteRuntimeUsage ): Promise< void > { | ||
| const now = Date.now(); | ||
| let shouldBump = false; | ||
|
|
||
| try { | ||
| await lockAppdata(); | ||
| const userData = await loadUserData(); | ||
| const metadata = userData.siteMetadata[ site.id ]; | ||
| if ( ! metadata?.runtimeStatBumpedAt || ! isSameWeek( metadata.runtimeStatBumpedAt, now ) ) { | ||
| userData.siteMetadata[ site.id ] = { ...metadata, runtimeStatBumpedAt: now }; | ||
| await saveUserData( userData ); | ||
| shouldBump = true; | ||
| } | ||
| } finally { | ||
| await unlockAppdata(); | ||
| } | ||
|
|
||
| if ( shouldBump ) { | ||
| bumpStat( StatsGroup.STUDIO_SITE_RUNTIME_WEEKLY, getSiteRuntimeStat( site ) ); | ||
| } | ||
| } |
There was a problem hiding this comment.
This fails to capture cases where the user switches the runtime. Let's say they pick "Native" to start with, then switch to "Sandbox" after 5 mins. The "Sandbox" stat would not be captured until after 7 days.
The perfect solution to this scenario would be for the "Sandbox" metric to override the original "Native" metric on the remote. I don't see a practical way of achieving that, but what we're looking to understand is how many users make a persistent choice to use the non-default.
If the stats show us that 5% of users are running "Sandbox" sites, but in reality, those 5% just tested "Sandbox" and immediately switched back to "Native", then the stats are misleading.
There was a problem hiding this comment.
That makes sense.
Here is my new approach:
- Changed from weekly to daily - increase volume and responsiveness
- Records unique daily + runtime stats - a site can have both runtime in a given day
These metrics should let us know the amount of sites that run native/sandbox in a given day. It won't track how much time it ran on a given runtime, but over time the counts will be dominated by whatever runtime each site actually runs most, so experiments average out.
There was a problem hiding this comment.
Sounds like a sensible approach 👍
There was a problem hiding this comment.
Would it not be better to track this at the CLI level instead of in the app?
There was a problem hiding this comment.
How do you see that working with --avoid-telemetry? The app runs the CLI with it for everything, so CLI-side tracking would be suppressed for app-driven starts unless we ignore the opt-out.
I'd say adding matching CLI-side tracking makes sense, but it could be done in a follow-up.
What do you think?
There was a problem hiding this comment.
unless we ignore the opt-out
The --avoid-telemetry option isn't included in the --help output, so there's no implicit contract with users that this option exists and should suppress all telemetry.
In fact, the way --avoid-telemetry is used now is more about distinguishing when the CLI was spawned by a user directly from the terminal versus when it's used as the back-end for the app.
adding matching CLI-side tracking makes sense, but it could be done in a follow-up
Fine by me 👍
There was a problem hiding this comment.
I think we might as well remove the beta feature now and rely on the individual site controls entirely. This also entails defaulting to "Native" and "Site directory"
| await using command = await runGlobalWpCliCommand( args, { runtime } ); | ||
| await using command = await runGlobalWpCliCommand( args, { | ||
| runtime: SITE_RUNTIME_PLAYGROUND, | ||
| } ); |
There was a problem hiding this comment.
I'd consider defaulting to native PHP here for the performance improvements. Side note: are we even using the --studio-no-path option anywhere these days? It doesn't look like it, and I can't remember exactly what we used it for before.
fredrikekelund
left a comment
There was a problem hiding this comment.
Fundamentally, this is solid 👍
The big remaining thing I see here is that we should switch the default runtime for existing sites from Playground to native, per our previous discussions on this topic.
Moreover, tracking the runtime stat when a site is edited from Studio is something we should fix.
I left a few more comments that would be nice to address, but they're only about simplifying the code and not about any behavioral change, so no big deal.
I'll come back for a final approval once you've addressed the high-priority issues, @bcotrim.
| export function getSiteRuntime( site: { runtime?: SiteRuntime } ): SiteRuntime { | ||
| return site.runtime ?? SITE_RUNTIME_PLAYGROUND; | ||
| } |
There was a problem hiding this comment.
@bcotrim and I discussed this approach and concluded that we should stick with the previously stated launch plan, which is to default existing sites to native PHP (see pfHvTO-1r1-p2)
| <SettingsRow label={ __( 'PHP runtime' ) }> | ||
| { /* translators: value for the PHP runtime setting on the site settings screen */ } | ||
| <span>{ isNativePhpRuntime ? __( 'Native' ) : __( 'Sandbox' ) }</span> | ||
| </SettingsRow> | ||
| <SettingsRow label={ __( 'File access' ) }> | ||
| { /* translators: value for the File access setting on the site settings screen */ } | ||
| <span> | ||
| { getSiteFileAccess( selectedSite ) === SITE_FILE_ACCESS_ALL_FILES | ||
| ? __( 'All files' ) | ||
| : __( 'Site directory' ) } | ||
| </span> | ||
| </SettingsRow> |
There was a problem hiding this comment.
I would consider adding the same explainer texts here that we have in EditSiteDetails. Putting it in a tooltip might be the best strategy, design-wise.
The reason I think this would be nice is that this is the first place where this new setting is visible for existing sites. We want to give users a clue about what this is without necessarily needing to open the Edit site dialog.
| // New sites default to the native PHP runtime; existing sites keep | ||
| // whatever they have (sandbox when unset, via getSiteRuntime). | ||
| const runtime = options.runtime ?? SITE_RUNTIME_NATIVE_PHP; |
There was a problem hiding this comment.
Just calling out that we need to change this spot, too
| .option( 'runtime', { | ||
| type: 'string', | ||
| describe: __( | ||
| 'Run the site with native PHP ("native") or in the Playground sandbox ("sandbox")' | ||
| ), | ||
| choices: [ SITE_MODE_NATIVE, SITE_MODE_SANDBOX ], | ||
| defaultDescription: SITE_MODE_SANDBOX, | ||
| } ) |
There was a problem hiding this comment.
| .option( 'runtime', { | |
| type: 'string', | |
| describe: __( | |
| 'Run the site with native PHP ("native") or in the Playground sandbox ("sandbox")' | |
| ), | |
| choices: [ SITE_MODE_NATIVE, SITE_MODE_SANDBOX ], | |
| defaultDescription: SITE_MODE_SANDBOX, | |
| } ) | |
| .option( 'runtime', { | |
| type: 'string', | |
| describe: __( | |
| 'Run the site with native PHP ("native") or in the Playground sandbox ("sandbox")' | |
| ), | |
| choices: [ SITE_MODE_NATIVE, SITE_MODE_SANDBOX ] as const, | |
| default: SITE_MODE_SANDBOX, | |
| } ) |
I think this behavior is already the intent here, but we aren't actually getting the type guarantees because of a few missing tweaks. We need this and a small tweak in tools/common/lib/site-runtime.ts – see my comment there
| .option( 'file-access', { | ||
| type: 'string', | ||
| describe: __( | ||
| 'Which files PHP can access with the native PHP runtime: the site directory only, or all files' | ||
| ), | ||
| choices: [ SITE_FILE_ACCESS_SITE_DIRECTORY, SITE_FILE_ACCESS_ALL_FILES ], | ||
| defaultDescription: SITE_FILE_ACCESS_SITE_DIRECTORY, | ||
| } ) |
There was a problem hiding this comment.
| .option( 'file-access', { | |
| type: 'string', | |
| describe: __( | |
| 'Which files PHP can access with the native PHP runtime: the site directory only, or all files' | |
| ), | |
| choices: [ SITE_FILE_ACCESS_SITE_DIRECTORY, SITE_FILE_ACCESS_ALL_FILES ], | |
| defaultDescription: SITE_FILE_ACCESS_SITE_DIRECTORY, | |
| } ) | |
| .option( 'file-access', { | |
| type: 'string', | |
| describe: __( | |
| 'Which files PHP can access with the native PHP runtime: the site directory only, or all files' | |
| ), | |
| choices: [ SITE_FILE_ACCESS_SITE_DIRECTORY, SITE_FILE_ACCESS_ALL_FILES ] as const, | |
| default: SITE_FILE_ACCESS_SITE_DIRECTORY, | |
| } ) |
Same thing as for runtime. Again, small tweak in tools/common/lib/site-file-access.ts needed
| export const SITE_FILE_ACCESS_SITE_DIRECTORY = 'site-directory'; | ||
| export const SITE_FILE_ACCESS_ALL_FILES = 'all-files'; |
There was a problem hiding this comment.
| export const SITE_FILE_ACCESS_SITE_DIRECTORY = 'site-directory'; | |
| export const SITE_FILE_ACCESS_ALL_FILES = 'all-files'; | |
| export const SITE_FILE_ACCESS_SITE_DIRECTORY = 'site-directory' as const; | |
| export const SITE_FILE_ACCESS_ALL_FILES = 'all-files' as const; |
Same as in site-runtime.ts, this is mainly for improved yargs typings.
| runtime?: SiteRuntime; | ||
| fileAccess?: SiteFileAccess; |
There was a problem hiding this comment.
| runtime?: SiteRuntime; | |
| fileAccess?: SiteFileAccess; | |
| runtime: SiteRuntime; | |
| fileAccess: SiteFileAccess; |
These don't need to be optional if we follow my other suggestions
| const siteRuntime = options.runtime ?? SITE_RUNTIME_NATIVE_PHP; | ||
| if ( options.fileAccess && ! isFileAccessAllowedForRuntime( siteRuntime, options.fileAccess ) ) { | ||
| throw new LoggerError( | ||
| __( | ||
| 'File access "all-files" requires the native PHP runtime. The sandbox only has access to the site directory.' | ||
| ) | ||
| ); | ||
| } |
There was a problem hiding this comment.
| const siteRuntime = options.runtime ?? SITE_RUNTIME_NATIVE_PHP; | |
| if ( options.fileAccess && ! isFileAccessAllowedForRuntime( siteRuntime, options.fileAccess ) ) { | |
| throw new LoggerError( | |
| __( | |
| 'File access "all-files" requires the native PHP runtime. The sandbox only has access to the site directory.' | |
| ) | |
| ); | |
| } | |
| if ( ! isFileAccessAllowedForRuntime( options.runtime, options.fileAccess ) ) { | |
| throw new LoggerError( | |
| __( | |
| 'File access "all-files" requires the native PHP runtime. The sandbox only has access to the site directory.' | |
| ) | |
| ); | |
| } |
Again, we can make this simpler if runtime and fileAccess are required
| siteId?: string; | ||
| wpVersion: string; | ||
| phpVersion: SupportedPHPVersion; | ||
| phpVersion: string; |
There was a problem hiding this comment.
| phpVersion: string; | |
| phpVersion: SupportedPHPVersion; |
This is actually still fine, AFAICT. It'd be good to keep the stricter type if there's no clear reason to drop it.
| * captured on the next start rather than at the next day boundary). The per-site | ||
| * marker lives in `app.json`'s site metadata. | ||
| */ | ||
| export async function recordSiteRuntimeUsage( site: SiteRuntimeUsage ): Promise< void > { |
There was a problem hiding this comment.
This doesn't get called from the updateSite function in apps/studio/src/ipc-handlers.ts. Maybe consider dropping a recordSiteRuntimeUsage() call into editSiteViaCli()?
fredrikekelund
left a comment
There was a problem hiding this comment.
Looks great 👍
I left two final comments that would be nice to address (though I'll leave them to your judgement, @bcotrim). I also took the liberty of pushing a small tweak.
| // Per-site daily dedup markers for the runtime adoption stat (RSM-3958). | ||
| siteRuntimeStats: z | ||
| .record( z.string(), z.object( { bumpedAt: z.number(), stat: z.string() } ) ) | ||
| .optional(), |
There was a problem hiding this comment.
Not a show stopper, but we should have defined cliConfigSchema with z.looseObject from the start 🤦♂️ Might as well do it now.
Theoretically, old CLI versions will strip out siteRuntimeStats when loading cli.json, but this should average out over time.
There was a problem hiding this comment.
I'd make this a React component instead and load __ with the standard useI18n() hook. We can return a JSX fragment.
The Tooltip component accepts a React component for the text prop, so this strategy should work.
…t native-php e2e jobs (#3889) ## Related issues - Related to #3786, which made native PHP the default site runtime and surfaced this on the Linux e2e suite. - Related to RSM-2593 (Linux E2E setup). ## How AI was used in this PR Diagnosed and implemented with Claude Code. From the failing CI log, Claude traced an `EACCES` raised while applying a blueprint to the native runtime writing a temporary sidecar file next to the uploaded blueprint, and to Linux CI's user-remapped Docker checkout being read-only. I directed the approach — fix it in the runtime (mirroring the temp-dir pattern blueprint bundles already use) rather than working around it in the test, and separately drop the now-redundant native-php e2e jobs. Claude made the edits and verified them (eslint, YAML validity); the fix is exercised by the existing blueprint e2e suite, which fails on Linux without it. I reviewed the diff. ## Proposed Changes Since native PHP became the default runtime (#3786), creating a site from a blueprint runs the native path, which writes a temporary sidecar file next to the blueprint as it applies it. If that blueprint lives in a **read-only directory** — a locked-down or mounted location for a real user, or the user-remapped Docker checkout on Linux CI — the write fails with `EACCES` and site creation aborts. The native runtime now **falls back to a private temp directory** when the blueprint's own directory isn't writable (and keeps co-locating when it is, so a bundle's sibling resources still resolve). Creating a site from a blueprint now works regardless of where the blueprint file lives; there's no other user-visible behavior change. Separately, this removes the two "… with native PHP" e2e jobs and their `STUDIO_RUNTIME` env. That variable's consumer was deleted when the beta runtime toggle was removed, so the jobs were redundant no-ops — the regular e2e jobs already exercise native PHP now that it's the default. ## Testing Instructions - **e2e (Linux is the meaningful platform):** the blueprint suite (`apps/studio/e2e/blueprints.test.ts`) creates sites from fixtures in the read-only checkout; it previously failed with `EACCES … studio-blueprint-<id>.json` and now passes. The test itself is unchanged — the fix is in the runtime. - **Manual:** create a site from a `.json` blueprint placed in a read-only directory — it should succeed instead of erroring. - **Pipeline:** the E2E group shows only the regular jobs (no "… with native PHP" variants); `grep STUDIO_RUNTIME .buildkite/pipeline.yml` returns nothing. ## Pre-merge Checklist - [ ] Have you checked for TypeScript, React or other console errors? 🤖 Generated with [Claude Code](https://claude.com/claude-code)
## Related issues _No linked issue._ Addresses the gap where the Sandbox (WordPress Playground) runtime had no end-to-end coverage after Native PHP became the default site runtime in #3786. ## How AI was used in this PR Investigated the e2e harness, the Add Site modal UI, and the runtime plumbing, then wrote the test and page-object helpers with Claude Code (Opus 4.8). Selectors were checked against the real components (the `#php-runtime-select` dropdown and the Settings "PHP runtime" row), the additive `completeOnboarding` change was confirmed backward-compatible with all existing callers, and ESLint + `tsc` were run clean on the changed files. ## Proposed Changes Studio supports two per-site PHP runtimes: Native PHP (the default since #3786) and the Sandbox (WordPress Playground / PHP-WASM). Every existing e2e test runs against the default runtime, so the Sandbox path had **zero** end-to-end coverage — and site/blueprint creation on Playground lost the implicit coverage it had back when Playground was the default. This adds a single e2e test that exercises the Sandbox runtime through the real UI: - Creates a site via onboarding with **Sandbox** selected under Advanced settings. - Verifies the site actually boots under Playground (waits for the "Running" state). - Verifies the Settings tab reports the site's PHP runtime as **"Sandbox"**. It runs as part of the normal e2e suite on every platform/arch — no dedicated CI job and no gating. The supporting page-object additions (runtime selection, the runtime display row) are reusable for future runtime-specific tests. No product code is changed. ## Testing Instructions 1. Build the app and run just this test: ``` npm run e2e -- sandbox.test.ts ``` (or `npx playwright test sandbox.test.ts` against an existing build) 2. The test creates a Playground site, waits up to ~3 min for first-run asset downloads, then asserts the Settings tab shows **PHP runtime = Sandbox**. 3. Requires network access — Playground downloads the PHP-WASM build and WordPress on first run. ## Pre-merge Checklist - [x] Have you checked for TypeScript, React or other console errors? — ESLint `--fix` and `tsc` are clean for the changed e2e files. The test is statically verified but **not yet run green**; please run it locally before merging.
Related issues
How AI was used in this PR
Implemented end-to-end with Claude Code, starting from the design agreed in Slack (two per-site settings instead of a 3-option dropdown). I directed the key decisions — per-site
runtime/fileAccess, native PHP as the default, narrowing the PHP version list, and the usage metric — and reviewed the diff. Tests were written/updated by AI and verified locally (typecheck, lint, full suite, built-CLI smoke tests).Proposed Changes
Adds two per-site settings that control how a site's PHP runs, replacing the old global toggle (and avoiding "safe/unsafe/trusted" wording):
Native(native PHP binaries) orSandbox(WordPress Playground). Native is the default for both new and existing sites — existing sites switch to Native on their next start (clamping any unsupported stored PHP version to the closest supported one); a site explicitly set to Sandbox keeps it.Site directory(default:open_basedirjail +disable_functions) orAll files(neither restriction). The invalid "Sandbox + All files" combination is disabled in the UI with an explainer and rejected by the CLI with a clear error.Both settings are selectable when creating a site and when editing one, shown read-only in the site settings tab (with explainer tooltips), exposed via the CLI (
studio site create|set --runtime <native|sandbox> --file-access <site-directory|all-files>), and reported bystudio site status. PHP version choices are unified to 8.2–8.5 for both runtimes. Adoption is tracked with a privacy-preserving daily count of active sites by runtime (no per-site identifiers), recorded in the CLI at site start so it covers every start path — desktop-spawned and terminal alike.Testing Instructions
Desktop
CLI (
npm run cli:buildfirst)studio site create --path ~/Studio/test --file-access all-files --no-start→ new site is Native + All files (Native is the default).studio site set --path ~/Studio/test --runtime sandbox→ errors asking to reset file access; add--file-access site-directory→ succeeds and restarts if running.studio site status --path ~/Studio/test→ shows PHP runtime and File access rows;--format jsonincludesruntime/fileAccess.--phpchoices show8.5, 8.4, 8.3, 8.2onsite create,site set, andblueprint use.Verifying file-access enforcement —
open_basedir/disable_functionsapply to the PHP server workers, not WP-CLI (intentionally exempt: trusted, user-initiated, and it needs paths outside the site for imports/exports). So probe over HTTP rather than viastudio wp eval:open_basedirjailed to the site dir (+ router/phpMyAdmin/tmp),disable_functionspopulated,can_read_outside: false.can_read_outside: true:can_read_outside: false; the WASM filesystem still confines PHP to the site directory. Clean up withrm ~/Studio/test/probe.php:Pre-merge Checklist
🤖 Generated with Claude Code