-
Notifications
You must be signed in to change notification settings - Fork 276
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Route requests for missing static files using remote asset metadata #1417
Conversation
Apparently, a commit hook auto-committed a bunch of other changes that I had left outstanding. I will fix this and force push shortly. |
No, I think that was me with an errant |
4225de3
to
9a63d47
Compare
The minified WP builds need to be updated as part of this PR. I had some trouble rebuilding them locally because the command in the README didn't produce updated zips. I ended up running the following commands to rebuilds the zips:
This didn't update the zip for WP 6.1. It looks like we're still showing that as an option on playground.wordpress.net, but we only build the 3 previous major versions. Do we need to manually remove WP 6.1? |
} | ||
|
||
#seemsLikeAPHPFile(path: string) { | ||
return path.endsWith('.php') || path.includes('.php/'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should .php/
also check for endsWith? This would prevent paths like /test.php/index.js
from being executed.
return path.endsWith('.php') || path.includes('.php/'); | |
return path.endsWith('.php') || path.endsWith('.php/'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that check is wrong, but I was leaving it for a follow up PR because it is fixing a different problem from this PR. Originally, I'd actually removed that check altogether, which is also the wrong thing to do.
I have a note to make a different PR for this and will make sure the something.php/
case is tested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a note to make a different PR for this and will make sure the something.php/ case is tested.
Nevermind, we can go ahead and refactor everything here.
// TODO: Move file to /internal? | ||
// TODO: This feels like some kind of metadata we mentioned when discussing boot protocol | ||
const remoteAssetListPath = `${requestHandler.documentRoot}/playground-remote-asset-paths`; | ||
if (primaryPhp.fileExists(remoteAssetListPath)) { | ||
const remoteAssetPaths = primaryPhp | ||
.readFileAsText(remoteAssetListPath) | ||
.split('\n'); | ||
requestHandler.addRemoteAssetPaths(remoteAssetPaths); | ||
// TODO: Delete the listing file? | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes to all TODOs. It's a custom file that's not part of WP and we should keep it in internal
.
Deleting the file should help with preventing issues when the WP structure is updated, for example after a WP update, or version switch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes to all TODOs. It's a custom file that's not part of WP and we should keep it in internal
Sounds good!
# List assets that will be available to remotely so Playground can judge whether | ||
# to request a remote asset or delegate the request for a missing file to PHP. | ||
# TODO: What is a better name for this file? | ||
RUN find wordpress-static -type f | sed 's#^wordpress-static/##'> playground-remote-asset-paths |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe wordpress-remote-asset-paths
?
RUN find wordpress-static -type f | sed 's#^wordpress-static/##'> playground-remote-asset-paths | |
RUN find wordpress-static -type f | sed 's#^wordpress-static/##'> wordpress-remote-asset-paths |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But playground-remote-asset-paths
also works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 The main point was to communicate that this is a special setup for minified WP builds, and it seemed a playground-specific thing to me. If I think about someone listing the files in one of the zips, I think it is clearer to see "playground-" than "wordpress-" when they ask "What is this thing?".
I was on the fence about this, but after talking it out, I think I'll leave it alone unless someone else feels strongly otherwise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heh, I save this comment and then think the opposite. Within playground, I would rather read /internal/wordpress-remote-asset-paths
, so let's go with that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note this is only useful on the web and only for some WordPress builds but not for others. Also, the "active" WordPress build may change in runtime.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, so it sounds like it belongs with some kind of Wordpress-associated metadata.
I'll start by acting as if WP cannot be swapped out and address the request handling behavior and then address the possibility of WP switching, all within the scope of this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the "active" WordPress build may change in runtime.
@adamziel it looks like the web interface reloads the page every time the config changes. Where can the "active" WordPress build change anytime during runtime? Is this something that can happen with wp-now without reloading everything? (Planning to look further but wanted to get the question out there)
@@ -313,7 +316,7 @@ export class PHPRequestHandler<PHP extends BasePHP> { | |||
this.rewriteRules | |||
); | |||
const fsPath = joinPaths(this.#DOCROOT, normalizedRequestedPath); | |||
if (!seemsLikeAPHPRequestHandlerPath(fsPath)) { | |||
if (!(await this.#seemsLikeAPHPRequestHandlerPath(fsPath))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At this point I'd refactor the routing logic completely. There's no more need for "seems" functions – we can resolve the URL to something like { "filePath": string, "isRemote": boolean }
with 100% certainty. Then we'd test whether filePath
is a file with .php
extension. A requested path like /index.php/a/b/c
could mean different things, e.g. I'm requesting a static file called "c" that's inside a directory tree like $DOCROOT/index.php/a/b/
, or I'm requesting index.php and /a/b/c is my "nice URL"
. Let's do what a web server would do here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we do a full refactor let's also take this use case into account. , I started working on it, but lacked the metadata to complete it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's do what a web server would do here.
Right. Let's get it done. I'll work on that.
If we do a full refactor let's also take this use case into account.
Got it. Hopefully that will just naturally work after the refactor, but we can test it.
Here is a rough outline of how I think the handling should work: let fsPath = joinPaths(this.#DOCROOT, normalizedRequestedPath);
// Simply handle PHP as PHP
if (fsPath.endsWith('.php')) {
if (primaryPhp.isDir(fsPath)) {
fsPath = joinPaths(fsPath, 'index.php');
}
if (primaryPhp.fileExists(fsPath)) {
// TODO: Serve as PHP
} else {
// TODO: 404
}
}
// Try as regular file
if (primaryPhp.fileExists(fsPath) && primaryPhp.isFile(fsPath)) {
// TODO: Serve static file
} else if (this.#remoteAssetPaths.has(fsPath)) {
// TODO: Request and serve static file
}
// Try as directory with default index.php
const localDefaultFile = joinPaths(fsPath, 'index.php');
if (
primaryPhp.isDir(fsPath) &&
primaryPhp.fileExists(localDefaultFile) &&
primaryPhp.isFile(localDefaultFile)
) {
// TODO: Serve as PHP
}
// TODO: Delegate request to WordPress /index.php, retaining query args and relaying original request URI I still need to work through:
|
It may require going left to right segment by segment until you reach a file, e.g. in |
I have been working based on this bit of Nginx config which I do not believe does anything as complicated as that: location / {
# This is cool because no php is touched for static content.
# include the "?$args" part so non-default permalinks doesn't break when using query string
try_files $uri $uri/ /index.php?$args;
}
location ~ \.php$ {
#NOTE: You should have "cgi.fix_pathinfo = 0;" in php.ini
include fastcgi.conf;
fastcgi_intercept_errors on;
fastcgi_pass php;
} That is not to say we cannot do something more involved. But, as an example, WP Cloud doesn't directly run |
Hmm, unlike yesterday's sample logic, WP Cloud does delegate requests for missing PHP files to WordPress. I don't think the WP.org's example nginx config would do that though for |
a72bc92
to
86fcaed
Compare
86fcaed
to
f3d01c5
Compare
I updated the routing logic in 3f69fdc to do roughly the following:
This behavior is close to this bit of the WordPress.org nginx example location / {
# This is cool because no php is touched for static content.
# include the "?$args" part so non-default permalinks doesn't break when using query string
try_files $uri $uri/ /index.php?$args;
}
location ~ \.php$ {
#NOTE: You should have "cgi.fix_pathinfo = 0;" in php.ini
include fastcgi.conf;
fastcgi_intercept_errors on;
fastcgi_pass php;
} But the implementation in this PR differs from the above config, unless I'm mistaken, because non-existent PHP files are delegated to WordPress. (In the config above, the @adamziel I am not familiar with serving URLs like So far, there are two main things left to do in this PR:
|
One more note: |
Regarding sitemap.xml:
|
One thing I'm wondering: When plain permalinks are selected and WordPress is handling a random, non-existent request URI, it responds with the home page and a 200, all without a redirect. @adamziel @bgrgicak would it make sense to default WordPress within Playground to use pretty permalinks? Otherwise, all requests for non-existent files that we dedicate to WordPress will receive default homepage HTML and a 200 response status. It seems safer and clearer to actually have WordPress respond with 404s. |
Given #1454 "Add missing trailing slash on landingPage wp-admin path", I think we should probably be responding with a redirect when a URI points to a directory without a trailing slash. |
It is no longer needed because request handler auto-redirects to trailing slash paths when the path points to an existing directory.
I added logic to redirect to add a trailing slash when an existing directory is requested without one. And the tests are now updated to cover the refactored routing logic. The main thing remaining is to understand how the active WordPress build can change at runtime (as @adamziel mentioned) and accommodate that in how we maintain the collection of known remote asset paths. I guess the active WordPress build can change as simply as the filesystem can be changed over time. But are there other ways the entire WordPress version can change? Can multiple scopes use the same PHP request handler? These are the remaining questions for this PR. cc @bgrgicak |
I would prefer to stick to the default WP behavior for permalink structures. Does WP on Apache/NGINX also return 200? |
Yes, this is my experience on WP Cloud as well when no permalink structure ("plain" permalinks) is selected.
It turns out pretty permalinks are the default during WP install: That is good because features like sitemap.xml generation appear to depend on them being enabled. Unfortunately, WP in Playground does not default to pretty permalinks, and I don't yet know why. WP install does issue a request against the site to check whether pretty permalinks can be enabled by default: I think that would require the changes in this PR to work. But it doesn't default to pretty permalinks with these changes either. Perhaps we're not running WP install yet or it runs differently than usual. I made a note to file an issue for this if it isn't resolved in this PR. |
@@ -24,7 +24,7 @@ describe('Blueprint step installPlugin', () => { | |||
it('should log the user in', async () => { | |||
await login(php, {}); | |||
const response = await requestHandler.request({ | |||
url: '/wp-admin', | |||
url: '/wp-admin/', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fixes the test because the handler now returns a redirect when the request URI identifies an existing dir without including a trailing slash.
@@ -49,14 +49,6 @@ export async function setupPlatformLevelMuPlugins(php: UniversalPHP) { | |||
await php.writeFile( | |||
'/internal/shared/mu-plugins/0-playground.php', | |||
`<?php | |||
// Redirect /wp-admin to /wp-admin/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should no longer be necessary since the request handler now redirects to add a trailing slash for existing dirs.
I merged the latest from trunk and adapted these changes to work within the new bootWordPress() function. If I understand that function correctly, we get a new PHPRequestHandler each time it is called, so it seems like that should be enough to update the list of remote assets each time a new WordPress build is selected. If a build doesn't include a list of remote assets, we don't attempt to read it and assume all assets are included in the zip. I believe this is ready for review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I try to open an unexisting static file like /sitemap.xml
this PR will redirect to /sitemap.xml/
while trunk will return 404.
Ideally static files that don't exist should execute/index.php
, but the path should stay unchanged.
if (primaryPhp.isFile(localDefaultPath)) { | ||
fsPath = localDefaultPath; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we check for index.html
if index.php
doesn't exist?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That seems like a good idea. I don't know of a reason we wouldn't want to serve index.html when there is no index.php.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added in f655598
This appears to be default WP behavior when plain permalinks are selected. The same occurs when testing from the WP codebase using |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left a nitpick that can be either addressed here or in a follow-up PR alongside removing the /wp-admin redirect from platform mu-plugins. Otherwise this looks good. Thank you!
|
||
// We can only satisfy requests for directories with a default file | ||
// so let's first resolve to a default path when available. | ||
if (primaryPhp.isDir(fsPath)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add a more extensive note explaining how this is default WP behavior - it's not obvious this logic is the right one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add a more extensive note explaining how this is default WP behavior - it's not obvious this logic is the right one.
Good point! Added this:
// Ensure directory URIs have a trailing slash. Otherwise,
// relative URIs in index.php or index.html files are relative
// to the next directory up.
//
// Example:
// For a request to "/wp-admin", the relative link "edit.php"
// resolves to "/edit.php" rather than "/wp-admin/edit.php".
//
// This is correct behavior for the browser:
// https://www.rfc-editor.org/rfc/rfc3986#section-5.2.3
//
// But the intent for `/wp-admin/index.php` is that its relative
// URIs are relative to `/wp-admin/`.
//
// In fact, WordPress also redirects like this when given a chance.
// - https://github.com/WordPress/wordpress-develop/blob/b6a3b9c7d1ce33cbeca6f95871a26c48141e524b/src/wp-includes/canonical.php#L696
// - https://github.com/WordPress/wordpress-develop/blob/b6a3b9c7d1ce33cbeca6f95871a26c48141e524b/src/wp-includes/canonical.php#L1036-L1045
// - https://github.com/WordPress/wordpress-develop/blob/b6a3b9c7d1ce33cbeca6f95871a26c48141e524b/src/wp-includes/link-template.php#L3558
…tadata (#1417)" (#1474) ## What is this PR doing? This reverts commit 36b126e. ## What problem is it solving? When using browser storage and reloading Playground after an initial load, WordPress renders as if it cannot find remote CSS assets.  The issue should be something small, but I am reverting until we can fix it.
…1417) This PR: - Includes a list of remote asset paths with minified WP builds - Updates request handling to only handle missing files as static when they are explicitly listed as remote assets (perhaps this should be split into multiple PRs) - Updates request routing to behave more like a regular web server Related to #1365 in that we need better information to judge whether to handle a request for a missing static file as PHP. Prior to this PR, we could not easily tell whether we should request a missing static asset from the web or delegate the request to PHP. By including a list of remote asset paths with minified WP builds, we can judge which missing static file paths represent remote assets and which can be delegated to PHP. - CI tests - Examine a minified WP zip and the contents of its wordpress-remote-asset-paths file - Test manually with `npm run dev` and observe that Playground loads normally with no unexpected errors in the console
…1417) This PR: - Includes a list of remote asset paths with minified WP builds - Updates request handling to only handle missing files as static when they are explicitly listed as remote assets (perhaps this should be split into multiple PRs) - Updates request routing to behave more like a regular web server Related to #1365 in that we need better information to judge whether to handle a request for a missing static file as PHP. Prior to this PR, we could not easily tell whether we should request a missing static asset from the web or delegate the request to PHP. By including a list of remote asset paths with minified WP builds, we can judge which missing static file paths represent remote assets and which can be delegated to PHP. - CI tests - Examine a minified WP zip and the contents of its wordpress-remote-asset-paths file - Test manually with `npm run dev` and observe that Playground loads normally with no unexpected errors in the console
## Motivation for the change, related issues This PR adds a remote asset listing to minified WP builds so we can later tell which files are expected to exist remotely and which should be considered missing if they are not present locally. This was originally part of request routing PRs #1417 and #1490, but since there are some sensitive edge cases around the routing changes and browser storage, I am breaking the build changes into their own PR so the more sensitive changes can be reviewed more easily on their own. ## Implementation details This PR updates the minified WP build process to generate a wordpress-remote-asset-paths file containing the WP-relative paths of all assets not included in the minified build. That way, we have the necessary information to judge whether a requested resource can be requested from playground.wordpress.net when it does not exist locally. We include this listing with new minified WP builds. In case browser storage has already cached a minified WP build without this listing, we also make it available remotely so it can be retrieved as needed. ## Testing Instructions (or ideally a Blueprint) - CI tests
What is this PR doing?
This PR:
(perhaps this should be split into multiple PRs)
Related to #1365 in that we need better information to judge whether to handle a request for a missing static file as PHP.
What problem is it solving?
Prior to this PR, we could not easily tell whether we should request a missing static asset from the web or delegate the request to PHP.
How is the problem addressed?
By including a list of remote asset paths with minified WP builds, we can judge which missing static file paths represent remote assets and which can be delegated to PHP.
Testing Instructions
npm run dev
and observe that Playground loads normally with no unexpected errors in the console