Route requests more like a normal web server #1539

brandonpayton · 2024-06-24T19:00:33Z

Motivation for the change, related issues

This PR updates PHPRequestHandler to route HTTP requests more like a regular web server.

Prior to this PR, we could not easily tell whether we should request a missing static asset from the web or delegate a request for a missing file to WordPress.

Related to #1365 and #1187

Implementation details

This PR:

Updates PHPRequestHandler to support custom file-not-found responses
Updates worker-thread to configure PHPRequestHandler to handle file-not-found conditions properly for WordPress
Updates worker-thread to flag remote WP assets for retrieval by the Service Worker

Testing Instructions (or ideally a Blueprint)

CI tests
Test manually with npm run dev and observe that Playground loads normally with no unexpected errors in the console

packages/php-wasm/universal/src/lib/php-request-handler.ts

packages/playground/wordpress/src/boot.ts

packages/php-wasm/universal/src/lib/php-request-handler.ts

brandonpayton · 2024-06-24T23:20:47Z

This is ready for review and can merge after #1531.

packages/php-wasm/universal/src/lib/php-request-handler.ts

packages/php-wasm/universal/src/lib/php.ts

packages/php-wasm/universal/src/lib/php-request-handler.ts

adamziel · 2024-06-25T09:40:35Z

packages/php-wasm/universal/src/lib/php-request-handler.ts

+				// so the service worker knows it can issue a real fetch() to the server.
+				return new PHPResponse(
+					404,
+					{ 'x-file-type': ['static'] },


That's a good opportunity to rename this to more clearly communicate the intention. Perhaps 'x-file-source': 'remote-server', 'x-backfill-from': 'remote-host', something similar?

That's a good opportunity to rename this to more clearly communicate the intention. Perhaps 'x-file-source': 'remote-server', 'x-backfill-from': 'remote-host', something similar?

@adamziel Is it OK for us to leave this magic header as-is for now? I made the update but quickly discovered that running with an older Service Worker causes remote asset requests to fail until the updated Service Worker becomes active.

For now, we are adding both headers in an attempt to be backward compatible with clients running stale service workers.

Oh, both headers is a lovely idea ❤️

adamziel · 2024-06-25T09:45:58Z

@brandonpayton let's find a way to not introduce the coupling between PHP and WordPress. The decoupled design paid off dozens of times and once we start coupling, it will quickly become impossible to untangle.

brandonpayton · 2024-06-26T04:01:52Z

@adamziel thanks for the review! I reviewed all your comments and think we're on the same page.

@brandonpayton let's find a way to not introduce the coupling between PHP and WordPress. The decoupled design paid off dozens of times and once we start coupling, it will quickly become impossible to untangle.

I think we can decouple by using a single callback to determine a fallback for all file-not-found conditions. I have a messy version working locally. The gist is:

when a file is not found:
  get fallback action from callback
  if fallback is internal redirect:
    // This allows us to maintain the general redirect to /index.php outside of PHPRequestHandler
    set FS path to internal redirect path
  else if fallback is response:
    // This allows worker-thread to return special 404s for known remote static files
    // but may be useful in additional ways
    return response
  else
    return regular 404 response

This allows PHPRequestHandler to be decoupled from WordPress. With this approach, the remote worker-thread maintains the WordPress-specific parts, including remote static file responses and handling file-not-found conditions with WP index.php.

Planning to clean this up and push tomorrow morning unless you disagree with the direction.

adamziel · 2024-06-26T08:05:10Z

That sounds great @brandonpayton, thank you!

packages/php-wasm/universal/src/lib/php-request-handler.ts

packages/playground/wordpress/src/boot.ts

brandonpayton · 2024-06-27T02:29:50Z

@adamziel PHPRequestHandler is decoupled from WordPress again, and this is ready for another review.

I'm really happy with how the file-not-found callback approach allows us to keep special file-not-found handling logic within the packages that need it.

adamziel · 2024-06-28T20:02:19Z

packages/php-wasm/node/src/test/php-request-handler.spec.ts

+							type: 'response',
+							response: new PHPResponse(
+								404,
+								{ 'x-file-type': ['static'] },


X-file-type static doesnt mean much anymore in this context when it also works for .php paths, it would be nice to tell the service worker „please fetch from the server” using a different object shape

(I’m just pointing at the string used)

Yeah, it's not great. Unfortunately, I haven't figured out how to adjust this and not break for clients still running with the older Service Worker:

From this comment:

That's a good opportunity to rename this to more clearly communicate the intention. Perhaps 'x-file-source': 'remote-server', 'x-backfill-from': 'remote-host', something similar?

@adamziel Is it OK for us to leave this magic header as-is for now? I made the update but quickly discovered that running with an older Service Worker causes remote asset requests to fail until the updated Service Worker becomes active.

One thing we could do for now is send the new preferred header while continuing to send the old, like:

{ 'x-backfill-from': [ 'remote-host' ], // Include this header for remote assets via out-of-date service worker 'x-file-type': [ 'static' ], }

I'm planning to go this route for now.

brandonpayton · 2024-07-13T04:37:31Z

@adamziel This is ready for another review.

packages/php-wasm/universal/src/lib/php-request-handler.ts

packages/playground/remote/src/lib/worker-thread.ts

adamziel · 2024-07-15T20:54:02Z

Almost there @brandonpayton! Let's also rebase to make sure this works well with the latest trunk and the last few PRs @bgrgicak merged

brandonpayton · 2024-07-18T04:40:02Z

This should be ready to merge, but there are some odd test failures in CI that I have not been able to reproduce locally. Planning to take another look tomorrow.

This PR: - Updates PHPRequestHandler to support custom file-not-found responses - Updates worker-thread to configure PHPRequestHandler to handle file-not-found conditions properly for WordPress - Updates worker-thread to flag remote WP assets for retrieval by the Service Worker Prior to this PR, we could not easily tell whether we should request a missing static asset from the web or delegate a request for a missing file to WordPress. Related to #1365 where we need better information to judge whether to handle a request for a missing static file as PHP. - CI tests - Test manually with `npm run dev` and observe that Playground loads normally with no unexpected errors in the console

brandonpayton · 2024-07-19T01:36:31Z

I am currently troubleshooting why we are getting unit test failures under this PR in CI only. My current theory is that we've added so many tests that we might be hitting some sort of memory limit. Unfortunately, we cannot continue looking into this at the moment because GH actions are not running properly:

Planning to resume in the morning.

This reverts commit 4b09a5f.

…ution into multiple processes

…est execution into multiple processes" This reverts commit 55d9830.

When these are included, GH auto-cancels the CI unit test workflow.

brandonpayton · 2024-07-19T22:57:23Z

The CI unit test errors appear to go away when we reduce the number of tests for PHPRequestHandler. To confirm this isn't a real PHPRequestHandler issue, we can switch between

disabling new test permutations for multiple docRoots and absolute URLs
running all those permutations but only for a single PHP version

If both of those pass, I plan to reduce the number of docRoot/absoluteURL permutations for the time being.

If we have to revert the request routing changes, it may leave some users with a Service Worker from those routing changes, so we want to make sure the service worker still supports loading remote assets in response to the x-file-type header.

This reverts commit a0e853d.

brandonpayton · 2024-07-19T23:49:04Z

The CI unit test errors appear to go away when we reduce the number of tests for PHPRequestHandler. To confirm this isn't a real PHPRequestHandler issue, we can switch between

disabling new test permutations for multiple docRoots and absolute URLs

running all those permutations but only for a single PHP version

If both of those pass, I plan to reduce the number of docRoot/absoluteURL permutations for the time being.

Both combinations passed with no issues. For now, we can reduce the number of docRoot/absoluteURL permutations until we can figure out why the CI unit test workflow is being auto-canceled. (guess: hitting some kind of memory limit)

brandonpayton added [Type] Bug An existing feature does not function as intended [Type] Enhancement New feature or request [Package][@php-wasm] Web [Package][@php-wasm] Universal labels Jun 24, 2024

brandonpayton requested a review from a team June 24, 2024 19:00

brandonpayton self-assigned this Jun 24, 2024

brandonpayton commented Jun 24, 2024

View reviewed changes

packages/php-wasm/universal/src/lib/php-request-handler.ts Outdated Show resolved Hide resolved

brandonpayton force-pushed the fix-request-routing branch from 70498dd to ab5d83d Compare June 24, 2024 20:30

brandonpayton commented Jun 24, 2024

View reviewed changes

packages/playground/wordpress/src/boot.ts Show resolved Hide resolved

brandonpayton commented Jun 24, 2024

View reviewed changes

packages/php-wasm/universal/src/lib/php-request-handler.ts Outdated Show resolved Hide resolved

brandonpayton marked this pull request as ready for review June 24, 2024 23:12

brandonpayton mentioned this pull request Jun 25, 2024

Backfill remote asset listing when needed #1531

Merged

Base automatically changed from remote-asset-list-on-demand to trunk June 25, 2024 01:38

adamziel reviewed Jun 25, 2024

View reviewed changes

packages/php-wasm/universal/src/lib/php-request-handler.ts Outdated Show resolved Hide resolved

adamziel reviewed Jun 25, 2024

View reviewed changes

packages/php-wasm/universal/src/lib/php.ts Outdated Show resolved Hide resolved

adamziel reviewed Jun 25, 2024

View reviewed changes

packages/php-wasm/universal/src/lib/php-request-handler.ts Outdated Show resolved Hide resolved

adamziel reviewed Jun 25, 2024

View reviewed changes

packages/php-wasm/universal/src/lib/php-request-handler.ts Outdated Show resolved Hide resolved

adamziel reviewed Jun 25, 2024

View reviewed changes

packages/php-wasm/universal/src/lib/php-request-handler.ts Outdated Show resolved Hide resolved

adamziel reviewed Jun 25, 2024

View reviewed changes

brandonpayton mentioned this pull request Jun 25, 2024

Download all WordPress assets on boot #1532

Merged

brandonpayton force-pushed the fix-request-routing branch from e04fdb8 to ed7bf01 Compare June 27, 2024 01:54

brandonpayton commented Jun 27, 2024

View reviewed changes

packages/php-wasm/universal/src/lib/php-request-handler.ts Show resolved Hide resolved

brandonpayton commented Jun 27, 2024

View reviewed changes

packages/php-wasm/universal/src/lib/php-request-handler.ts Outdated Show resolved Hide resolved

brandonpayton commented Jun 27, 2024

View reviewed changes

packages/playground/wordpress/src/boot.ts Show resolved Hide resolved

adamziel reviewed Jun 28, 2024

View reviewed changes

brandonpayton force-pushed the fix-request-routing branch from fa6d45f to 0489507 Compare July 13, 2024 04:00

adamziel reviewed Jul 15, 2024

View reviewed changes

packages/php-wasm/universal/src/lib/php-request-handler.ts Outdated Show resolved Hide resolved

adamziel reviewed Jul 15, 2024

View reviewed changes

packages/playground/remote/src/lib/worker-thread.ts Outdated Show resolved Hide resolved

adamziel reviewed Jul 15, 2024

View reviewed changes

packages/playground/remote/src/lib/worker-thread.ts Show resolved Hide resolved

This was referenced Jul 16, 2024

Incorrect user capabilities result in unexpected 401 responses from the REST API #1611

Open

Make WP known-remote-asset checks dynamic, based on FS at runtime #1622

Open

brandonpayton force-pushed the fix-request-routing branch from c24f038 to 5010b1d Compare July 18, 2024 19:40

brandonpayton mentioned this pull request Jul 18, 2024

Throwaway PR: Try to repro and debug CI-specific test failures from #1539 #1627

Closed

Temporarily reduce number of new php-request-handler tests

4b09a5f

brandonpayton added 2 commits July 19, 2024 11:31

Revert "Temporarily reduce number of new php-request-handler tests"

826eead

This reverts commit 4b09a5f.

See if we can avoid GH workflow auto-cancelation if we fork test exec…

55d9830

…ution into multiple processes

brandonpayton changed the title ~~Route requests for missing static files using remote asset metadata~~ Route requests more like a normal web server Jul 19, 2024

brandonpayton added 2 commits July 19, 2024 18:33

Revert "See if we can avoid GH workflow auto-cancelation if we fork t…

7ba7f58

…est execution into multiple processes" This reverts commit 55d9830.

Reduce number of new PHPRequestHandler tests

e093702

When these are included, GH auto-cancels the CI unit test workflow.

brandonpayton added 4 commits July 19, 2024 19:16

Try different docRoots and absoluteUrls

a0e853d

Disable unstable e2e test for now

e337597

Revert "Try different docRoots and absoluteUrls"

9398af9

This reverts commit a0e853d.

brandonpayton merged commit 88accae into trunk Jul 19, 2024
5 checks passed

brandonpayton deleted the fix-request-routing branch July 19, 2024 23:51

adamziel mentioned this pull request Jul 23, 2024

issue #1186 add sitemap.*.xml$ to the things that generate a .php request. #1187

Closed

This was referenced Jul 24, 2024

Static files run as PHP when path includes directory like <some-dir>.php/ #1365

Closed

Redirect /wp-admin to /wp-admin/ as not to break the admin navigation menu #461

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Route requests more like a normal web server #1539

Route requests more like a normal web server #1539

brandonpayton commented Jun 24, 2024 •

edited

Loading

brandonpayton commented Jun 24, 2024

adamziel Jun 25, 2024

brandonpayton Jun 27, 2024 •

edited

Loading

brandonpayton Jul 13, 2024

adamziel Jul 15, 2024

adamziel commented Jun 25, 2024 •

edited

Loading

brandonpayton commented Jun 26, 2024 •

edited

Loading

adamziel commented Jun 26, 2024

brandonpayton commented Jun 27, 2024

adamziel Jun 28, 2024

adamziel Jun 28, 2024

brandonpayton Jul 13, 2024

brandonpayton commented Jul 13, 2024

adamziel commented Jul 15, 2024

brandonpayton commented Jul 18, 2024

brandonpayton commented Jul 19, 2024

brandonpayton commented Jul 19, 2024

brandonpayton commented Jul 19, 2024

Route requests more like a normal web server #1539

Route requests more like a normal web server #1539

Conversation

brandonpayton commented Jun 24, 2024 • edited Loading

Motivation for the change, related issues

Implementation details

Testing Instructions (or ideally a Blueprint)

brandonpayton commented Jun 24, 2024

adamziel Jun 25, 2024

Choose a reason for hiding this comment

brandonpayton Jun 27, 2024 • edited Loading

Choose a reason for hiding this comment

brandonpayton Jul 13, 2024

Choose a reason for hiding this comment

adamziel Jul 15, 2024

Choose a reason for hiding this comment

adamziel commented Jun 25, 2024 • edited Loading

brandonpayton commented Jun 26, 2024 • edited Loading

adamziel commented Jun 26, 2024

brandonpayton commented Jun 27, 2024

adamziel Jun 28, 2024

Choose a reason for hiding this comment

adamziel Jun 28, 2024

Choose a reason for hiding this comment

brandonpayton Jul 13, 2024

Choose a reason for hiding this comment

brandonpayton commented Jul 13, 2024

adamziel commented Jul 15, 2024

brandonpayton commented Jul 18, 2024

brandonpayton commented Jul 19, 2024

brandonpayton commented Jul 19, 2024

brandonpayton commented Jul 19, 2024

brandonpayton commented Jun 24, 2024 •

edited

Loading

brandonpayton Jun 27, 2024 •

edited

Loading

adamziel commented Jun 25, 2024 •

edited

Loading

brandonpayton commented Jun 26, 2024 •

edited

Loading