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

Default url rewrites to /index.php #1072

Merged
merged 4 commits into from
Mar 1, 2024
Merged

Conversation

bgrgicak
Copy link
Collaborator

@bgrgicak bgrgicak commented Feb 29, 2024

Fixes #1054

What is this PR doing?

If a requested file path can't be resolved (file or folder don't exist) the request file path is rewritten to /index.php.

What problem is it solving?

Ensures a response when requesting WordPress permalink URLs like /category/uncategorized/ using php.request.

How is the problem addressed?

The code assumed that a path which isn't a PHP file is a folder and requested the index.php file in that folder.
In the case of WordPress URLs the path might not be a folder so adding index.php would break a valid request.

To address this, index.php is added to the end of the path only if the path is a valid directory.
If not, the path is rewritten to index.php in the root folder.

Testing Instructions

  • Confirm that tests pass

@bgrgicak bgrgicak self-assigned this Feb 29, 2024
@bgrgicak
Copy link
Collaborator Author

@adamziel I found that the root cause of #1054 is in the way we modify the file path and not a problem with us not supporting WordPress permalinks so I didn't add another request handler.

@akirk
Copy link
Contributor

akirk commented Mar 1, 2024

Thains! My usecase is the export-translations URL on GlotPress and it uses URL parameters, so an example URL might be /glotpress/projects/local-plugins/friends/de/default/export-translations/?format=po&filters[user_login]=admin. Will it also work? What do you think about adding a unit test with GET parameters to be sure?

@bgrgicak
Copy link
Collaborator Author

bgrgicak commented Mar 1, 2024

It will work. This changes the PHP file that is requested, but the headers and globals like $_GET aren't changed.

I updated the tests to confirm that $_GET values are working.

@akirk
Copy link
Contributor

akirk commented Mar 1, 2024

Brilliant, thanks!

@adamziel
Copy link
Collaborator

adamziel commented Mar 1, 2024

@bgrgicak This solves the problem at hand and the tests pass so let's merge it. Nice!

My follow-up question would be: do we still need this code?

if (workerResponse.status === 404) {
for (const url of rewriteWordPressUrl(unscopedUrl, scope!)) {
rewrittenUrlString = url.toString();
workerResponse = await convertFetchEventToPHPRequest(
await cloneFetchEvent(event, rewrittenUrlString)
);
if (
workerResponse.status !== 404 ||
workerResponse.headers.get('x-file-type') === 'static'
) {
break;
}
}
}

If yes, then #1054 is still relevant and we'll still need to support that logic at the request handler level, otherwise the service worker will rewrite multisite URLs correctly, but other PHP.wasm consumers like WP-NOW and VS Code won't.

If not, then #1054 is indeed solved.

@adamziel adamziel merged commit 8447d91 into trunk Mar 1, 2024
5 checks passed
@adamziel adamziel deleted the update/1054-wp-request-rewrites branch March 1, 2024 15:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move URL rewriting from the service worker to the request handler
3 participants