Skip to content
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

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

Closed
wants to merge 1 commit into from

Conversation

rhildred
Copy link

@rhildred rhildred commented Apr 3, 2024

What is this PR doing?

#1186 The request is handled as though it is for a static file in packages/php-wasm/universal/src/lib
/php-request-handler.ts.

What problem is it solving?

The leading sitemap plugins generate the sitemap.xml file on request

How is the problem addressed?

add sitemap.*.xml$ to the things that generate a .php request.

Testing Instructions

install the plugin https://github.com/RavanH/xml-sitemap-feed for instance and surf to /wp-sitemap.xml for instance.

@bgrgicak
Copy link
Collaborator

bgrgicak commented Apr 3, 2024

@rhildred this change looks specific to your plugin and I don't think that it would work well for Playground.

Are there other ways your plugin could detect the request and generate the sitemap?
For example it could regenerate it using cron jobs and have it as a static file.
How are other plugins handling this?

Alternatively you could add some rewrite rules to WordPress or even to your instance of Playground to redirect the request to a PHP script.

@adamziel
Copy link
Collaborator

adamziel commented Apr 3, 2024

@bgrgicak I wonder why that works with a "regular" WordPress and not with Playground. Could it be that Playground only resolves the URLs ending with / and .php as PHP files? I think Apache takes the opposite approach and resolves static files if they exist, and otherwise it falls back to the main index.php file via the rewrite rules. Could we get rid of the seemsLikeAPHPFile function entirely and follow that same pattern?

@rhildred
Copy link
Author

rhildred commented Apr 4, 2024

I agree @adamziel . I also think that apache looks to see if there is a static file, and then serves from further back in the path if it doesn't exist. Checking for a static file, rather than changing the pattern of seemsLikeAPHPFile is probably the correct approach.

@bgrgicak
Copy link
Collaborator

bgrgicak commented Apr 4, 2024

Thanks! This sounds like a good solution, I can explore it this week.

@bgrgicak
Copy link
Collaborator

bgrgicak commented Apr 5, 2024

I took some time to explore this today and it's not that simple.

Here is a "working" draft , but it needs more work.

The current logic is, if it's a static file, return file, otherwise run the path as PHP, but that means that every missing static file request will trigger a PHP request.

This wouldn't be that bad in a normal environment, but Playground lazy loads some WordPress assets, and in this implementation before loading that asset a PHP request runs. For example, if you open /wp-admin/ this will attempt to lazy load 68 files which means 68 additional PHP requests.
I will find a way to exclude these lazy loaded paths from PHP requests and that should fix the issue.

@adamziel adamziel added this to the PHP Feature Parity milestone Apr 5, 2024
@adamziel
Copy link
Collaborator

adamziel commented Apr 5, 2024

Aha, the big problem here is Apache has immediate access to the filesystem and knows whether a static file exists or not. Playground doesn't. A chunk of static files is served on https://playground.wordpress.net/ and needs to be requested via HTTP before we know if they're a valid rewrite target.

I'd say a solution here would be to ship a list of those hosted static files with a minified WordPress release, and then Playground could be certain whether a static file exists or not.

One up on that would be assuming that paths ending with .js, .css, .png, .jpg, and .gif are always static – just to avoid risking dispatching a burst of static assets request to PHP only to get 404s in return and having an unresponsive playground for a few seconds.

That being said, a "proper" solution might need to wait considering deeper problems like inability to update site settings.

@rhildred does the sitemap.xml file name matter for your use-case? Or would using sitemap.php unblock you for now?

@rhildred
Copy link
Author

rhildred commented Apr 6, 2024

I am looking at the sitemap.php approach right now. I really just need an array of all of the published routes that I can loop over rather than use "Select.... from wp-posts where post-status = published and post-type in ('page', 'post')

@bgrgicak
Copy link
Collaborator

bgrgicak commented Apr 8, 2024

I'd say a solution here would be to ship a list of those hosted static files with a minified WordPress release, and then Playground could be certain whether a static file exists or not.

@adamziel I planned to exclude static files in /wp- folders from being executed like PHP files. This would resolve the issue without a list.

@adamziel
Copy link
Collaborator

adamziel commented Apr 8, 2024

@adamziel I planned to exclude static files in /wp- folders from being executed like PHP files. This would resolve the issue without a list.

@bgrgicak How do you know which files are static, though? /wp-content/plugins/thumbnails/image-360x360.png could be handled by a PHP file just like /sitemap.xml.

@bgrgicak
Copy link
Collaborator

bgrgicak commented Apr 8, 2024

@bgrgicak How do you know which files are static, though? /wp-content/plugins/thumbnails/image-360x360.png could be handled by a PHP file just like /sitemap.xml.

I would just assume that all non PHP file in these paths are static. This is why the rule should probably be just for wp-admin and includes. It would be a quick change, compared to having a full list, so it might be a good tradeoff.

@adamziel
Copy link
Collaborator

adamziel commented Apr 8, 2024

@bgrgicak Makes sense as a stop-gap solution, as long as it wouldn't start dispatching 404 /wp-content requests to PHP.

brandonpayton added a commit that referenced this pull request Jul 19, 2024
## 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
@adamziel
Copy link
Collaborator

@brandonpayton is this one solved with #1539?

@brandonpayton
Copy link
Member

brandonpayton commented Jul 24, 2024

@brandonpayton is this one solved with #1539?

@adamziel this is solved with #1539 when pretty permalinks are enabled within WordPress. This is actually the WordPress default if it can be tested successfully during WordPress install. See:

But the WP default in Playground is currently no pretty permalinks.

Maybe it is because WordPress is installed during bootWordPress() before Playground is able to satisfy subrequests (just a guess that could be wrong). I was hoping to look into it yet today.

@brandonpayton
Copy link
Member

I created an issue for the pretty permalink default here:
#1644

@rhildred, thank you for this PR. 🙇 It turns out we were able to fix this and a variety of other issues by reworking request routing in #1539, so we can close this. Until #1644 is fixed, it is necessary to proactively enable pretty permalinks for sitemap.xml support to work properly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants