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

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

None yet

4 participants