Detect actual, loaded WP version by brandonpayton · Pull Request #1503 · WordPress/wordpress-playground

brandonpayton · 2024-06-11T02:03:21Z

Motivation for the change, related issues

When loading from browser storage, Playground can incorrectly assume the loaded WP version is the default version when it is actually something else. This can cause issues where remote assets are requested from the wrong version of WP.

In addition, this work is needed to support a follow-up PR where we retrieve the wordpress-remote-asset-paths listing from the website when it is not already present within browser storage.

Implementation details

This PR adds two functions, one for detecting the version using wp-includes/version.php, and another for determining whether the detected version is one of the WP versions supported through the Playground web app.

These functions are used when initializing worker threads to detect the version of WP that was booted.

If detection fails, the WordPress version is left set to the default version.

Testing Instructions (or ideally a Blueprint)

CI, including new unit tests

bgrgicak · 2024-06-11T08:16:04Z

+	});
+
+	if (result.errors.length > 0) {
+		// TODO: Find a way to warn


You can use the logger for this

bgrgicak · 2024-06-11T08:17:28Z

+
+	if (!loadedVersion) {
+		// TODO: Is there something better than throwing at this stage?
+		return undefined;


Could we just return the default version?

If this is empty, something is very wrong – like a broken or missing WordPress installation. I'd rather show a big error.

OK, I will make this throw. That seems preferable for clarity, especially if we are planning to remove the uncertainty around requesting remote assets from older point releases. In that case, we are no longer running OK by coincidence but because we're actually doing the right thing.

adamziel · 2024-06-11T10:15:32Z

@brandonpayton If we're loading a minified build from the browser storage, all bets are off. We might be loading WP 6.4.1 while playground.wordpress.net already has assets from 6.4.3. The files list may differ, and mixing assets from the two might lead to a broken state.

In addition, this work is needed to support a follow-up PR where we retrieve the wordpress-remote-asset-paths listing from the website when it is not already present within browser storage.

Let's always put it in the browser storage and expect to find it there. If it's missing from a minified build, that's an error. That would only happen if it gets deleted after we boot the minified bundle and before we finish downloading the zip file with the rest of the assets.

Also, let's find a way to always load all the assets from 6.4.1. This will always work, but what do we do if those assets are missing from the browser storage? I have a few ideas:

Host all the historical assets bundles, including wordpress-remote-asset-paths, on playground.wordpress.net. It will increase the storage requirements by ~500MB a year. That sounds manageable for now and perhaps we'll have a better solution in 3 years.
Stream the assets from an official release bundle. We have a partial ZIP downloader in @php-wasm/stream-compression. Downside: It's more complex, we need to deal with CORS, it relies on ranges request support which we don't have in proxy.php. I'd leave that for an unspecified future.

adamziel · 2024-06-11T10:21:05Z

+): Promise<string | undefined> {
+	const result = await php.run({
+		code: `<?php
+			if (is_file('${wordpressRoot}/wp-includes/version.php')) {


php.documentRoot should do the trick here. The entire codebase assumes WordPress lives there – if it doesn't, we have a larger problem.

php.documentRoot should do the trick here. The entire codebase assumes WordPress lives there – if it doesn't, we have a larger problem.

Thanks, I ended up using requestHandler.documentRoot because php.documentRoot is marked as deprecated.

adamziel · 2024-06-11T10:22:11Z

+		`,
+	});
+
+	if (result.errors.length > 0) {


AFAIR php.run() throws on failure. result.errors is just stderr

AFAIR php.run() throws on failure. result.errors is just stderr

Thanks! Will skip considering result.errors then.

adamziel · 2024-06-11T10:26:11Z

+	// from browser storage is the default version when it is actually something else.
+	// Incorrectly assuming WP version can break things like remote asset retrieval
+	// for minified WP builds.
+	const loadedWordPressVersion = await getLoadedWordPressVersion(


Is this relevant for non-minified builds? Assets-wise, probably not, but it seems like it would enable displaying "6.6-nightly" in the UI, correct?

Is this relevant for non-minified builds? Assets-wise, probably not, but it seems like it would enable displaying "6.6-nightly" in the UI, correct?

Yes, I wasn't planning to do it as part of this PR but was thinking we could show the detected version in the UI.

brandonpayton · 2024-06-11T13:13:45Z

In addition, this work is needed to support a follow-up PR where we retrieve the wordpress-remote-asset-paths listing from the website when it is not already present within browser storage.

Let's always put it in the browser storage and expect to find it there. If it's missing from a minified build, that's an error. That would only happen if it gets deleted after we boot the minified bundle and before we finish downloading the zip file with the rest of the assets.

@adamziel the main case I am thinking of is today's browser storage contents before we started shipping the remote asset listing. If people used browser storage before then, they would not have the listing, and requests for remote assets break. It's why I reverted the second request routing attempt. We're no longer deleting that file, so it should exist for all new browser storage. But for people who are loading from older browser storage, it is broken.

What do you think we should do here? Retrieve the listing file, just let it be broken, or something else?

brandonpayton · 2024-06-11T13:18:56Z

What do you think we should do here? Retrieve the listing file, just let it be broken, or something else?

@adamziel nevermind, I see you are discussing that path here :)

Also, let's find a way to always load all the assets from 6.4.1. This will always work, but what do we do if those assets are missing from the browser storage? I have a few ideas:

brandonpayton · 2024-06-11T15:40:23Z

Also, let's find a way to always load all the assets from 6.4.1. This will always work, but what do we do if those assets are missing from the browser storage?

I think we should do this work in the following steps, as separate PRs:

This PR detects the current major/minor version
Another PR downloads the remote asset listing for the detected WP version. Playground may still break if the stored version of WP is a different point release than the latest, but it is not worse than our current behavior which is to assume WP loaded from browser storage is "latest".
Merge the request routing updates which depend on the remote asset listing. This gives us better request routing and shouldn't be worse than our current behavior of assuming WP from browser storage is "latest".
Support providing/backfilling remote assets for all supported WP point releases, including corresponding remote asset listings.
Update Playground to request WordPress assets from specific point releases

@WordPress/playground-maintainers what do you think?

note: We also need to consider what to do about "nightly" as a special case.

brandonpayton · 2024-06-11T17:59:53Z

With the new unit tests, we encountered this failure in CI:

   ❯ src/test/version-detect.spec.ts > Test WP version detection > detects WP nightly
     → expected 'beta' to equal 'nightly'

Is it possible this is due to some test parallelization and shared FS? Otherwise, I'm not sure why this would be failing. The WP nightly zip here does not have a wp_version that includes the string "beta".

brandonpayton · 2024-06-11T18:08:26Z

Is it possible this is due to some test parallelization and shared FS? Otherwise, I'm not sure why this would be failing. The WP nightly zip here does not have a wp_version that includes the string "beta".

The latest WP nightly build on trunk does include "beta" in its version string, which explains why the test is failing. We can't assume the nightly version contains "alpha".

brandonpayton · 2024-06-11T18:23:03Z

Since the "nightly" version is transient and unstable day-to-day, it is now explicitly skipped in the version detection unit tests.

As an aside:
I wonder if it makes sense to download a full WP nightly zip when using browser storage. That way, all needed assets for working with the specific nightly version will be stored together, and that version should have everything it needs regardless of how stale it becomes over time.

brandonpayton · 2024-06-11T20:01:08Z

Since the "nightly" version is transient and unstable day-to-day, it is now explicitly skipped in the version detection unit tests.

It looks like we can still detect the nightly version.

The nightly version with "beta" in the string has a numeric suffix and looks like:
6.6-beta1-58376

The actual beta version looks like:
6.6-beta2

I've tightened nightly version detection and re-enabled testing for it.

Users might bring their own builds, and we want to allow any declared version as long as it is non-empty.

brandonpayton · 2024-06-11T21:49:55Z

This is ready for another review. Please see this comment for a proposed path forward.

brandonpayton · 2024-06-18T14:33:16Z

@adamziel @bgrgicak do you have any concerns about landing this? If not, I will merge and use this to pull the wordpress-remote-asset-paths dynamically as needed. Then we should have everything we need to land the request routing changes, even with stale browser storage.

brandonpayton · 2024-06-18T14:33:53Z

Please see this comment for the overall plan:
#1503 (comment)

adamziel · 2024-06-18T16:44:20Z

I’ll re-review tomorrow

adamziel · 2024-06-19T14:31:41Z

+	const loadedWordPressVersion = await getLoadedWordPressVersion(
+		requestHandler
+	);
+	if (isSupportedWordPressVersion(loadedWordPressVersion)) {


With this check we won't always expose apiEndpoint.wordPressVersion which seems confusing. It makes it seem like we're not running any WordPress version if the version string doesn't match the pattern. What's an upside?

Ah, this actually overrides the previous value of wordPressVersion. Still, can't we always override?

Ah, this actually overrides the previous value of wordPressVersion. Still, can't we always override?

@adamziel This is the version used to download static assets. I think we should make it so that we just don't attempt static asset downloads for unsupported versions but plan to do that in a follow up PR.

I changed my mind and updated this PR to tolerate an undefined staticAssetsDirectory when loading an unsupported WP version. It didn't seem right to land this in an in-between state.

In addition, instead of clobbering the requested WordPress version with the detected version, I made them different properties:

requestedWordPressVersion

loadedWordPressVersion

adamziel

I've left a question about the version number check, but other than that this PR looks good to me!

brandonpayton added [Type] Bug An existing feature does not function as intended [Package][@wp-playground] WordPress [Feature] Boot Flow labels Jun 11, 2024

brandonpayton requested a review from a team June 11, 2024 02:03

brandonpayton self-assigned this Jun 11, 2024

bgrgicak reviewed Jun 11, 2024

View reviewed changes

adamziel reviewed Jun 11, 2024

View reviewed changes

brandonpayton added 9 commits June 11, 2024 14:26

Detect actual, loaded WP version

951013c

Ignore stderr for version check

0427b43

Throw when WP version fails

581c875

Move version detection to dedicated module

e0a854e

Add unit tests

620720e

Stop using deprecated documentRoot member

82036d5

Remove unnecessary blank line

e5e56e1

Skip testing detection of nightly version

be1697e

Stop allowing undefined detection result type

fe3c268

brandonpayton force-pushed the derive-wp-version-from-browser-storage branch from 0a796e6 to fe3c268 Compare June 11, 2024 19:16

Fix nightly version detection

b27f9a3

Restore mountpoint as a string literal

1be4138

brandonpayton added 4 commits June 11, 2024 16:53

Test with more version strings

672d925

Allow for custom versions

585619c

Users might bring their own builds, and we want to allow any declared version as long as it is non-empty.

Note why we allow custom version strings

f313c34

Remove addressed TODO

5227336

brandonpayton requested review from adamziel and bgrgicak June 11, 2024 21:49

adamziel reviewed Jun 19, 2024

View reviewed changes

adamziel approved these changes Jun 19, 2024

View reviewed changes

brandonpayton added 3 commits June 20, 2024 16:22

Merge branch 'trunk' into derive-wp-version-from-browser-storage

7f63e24

Add TODO for follow-up PR

08f4a48

Allow omission of staticAssetsDirectory

b581210

brandonpayton merged commit d6a90af into trunk Jun 21, 2024

brandonpayton deleted the derive-wp-version-from-browser-storage branch June 21, 2024 00:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Detect actual, loaded WP version - #1503

Detect actual, loaded WP version#1503
brandonpayton merged 18 commits into
trunkfrom
derive-wp-version-from-browser-storage

brandonpayton commented Jun 11, 2024 •

edited

Loading

bgrgicak Jun 11, 2024

bgrgicak Jun 11, 2024

adamziel Jun 11, 2024 •

edited

Loading

brandonpayton Jun 11, 2024

adamziel commented Jun 11, 2024 •

edited

Loading

adamziel Jun 11, 2024 •

edited

Loading

brandonpayton Jun 11, 2024

adamziel Jun 11, 2024 •

edited

Loading

brandonpayton Jun 11, 2024

adamziel Jun 11, 2024

brandonpayton Jun 11, 2024 •

edited

Loading

brandonpayton commented Jun 11, 2024

brandonpayton commented Jun 11, 2024

brandonpayton commented Jun 11, 2024

brandonpayton commented Jun 11, 2024

brandonpayton commented Jun 11, 2024

brandonpayton commented Jun 11, 2024 •

edited

Loading

brandonpayton commented Jun 11, 2024 •

edited

Loading

brandonpayton commented Jun 11, 2024

brandonpayton commented Jun 18, 2024

brandonpayton commented Jun 18, 2024 •

edited

Loading

adamziel commented Jun 18, 2024

adamziel Jun 19, 2024

adamziel Jun 19, 2024

brandonpayton Jun 20, 2024

brandonpayton Jun 20, 2024

adamziel Jun 21, 2024

adamziel left a comment

Labels

3 participants

Uh oh!

Conversation

brandonpayton commented Jun 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation for the change, related issues

Implementation details

Testing Instructions (or ideally a Blueprint)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamziel Jun 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamziel commented Jun 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

adamziel Jun 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamziel Jun 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brandonpayton Jun 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

brandonpayton commented Jun 11, 2024

brandonpayton commented Jun 11, 2024

brandonpayton commented Jun 11, 2024

brandonpayton commented Jun 11, 2024

brandonpayton commented Jun 11, 2024

brandonpayton commented Jun 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

brandonpayton commented Jun 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

brandonpayton commented Jun 11, 2024

brandonpayton commented Jun 18, 2024

brandonpayton commented Jun 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

adamziel commented Jun 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamziel left a comment

Choose a reason for hiding this comment

Labels

3 participants

brandonpayton commented Jun 11, 2024 •

edited

Loading

adamziel Jun 11, 2024 •

edited

Loading

adamziel commented Jun 11, 2024 •

edited

Loading

adamziel Jun 11, 2024 •

edited

Loading

adamziel Jun 11, 2024 •

edited

Loading

brandonpayton Jun 11, 2024 •

edited

Loading

brandonpayton commented Jun 11, 2024 •

edited

Loading

brandonpayton commented Jun 11, 2024 •

edited

Loading

brandonpayton commented Jun 18, 2024 •

edited

Loading