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

Add multisite rewrite rules #1083

Merged
merged 14 commits into from
Mar 20, 2024
Merged

Conversation

bgrgicak
Copy link
Collaborator

@bgrgicak bgrgicak commented Mar 5, 2024

What is this PR doing?

This PR adds support for multisite URL rewrites.

What problem is it solving?

It ensures that all types of WordPress URLs work on Playground.

How is the problem addressed?

By adding support for rewrite rules in PHP WASM and adding a rule to resolve WordPress multisite URLs.

Testing Instructions

  • Checkout this branch
  • Start a new multisite (ensure playground.test proxy is running)
  • Confirm that the subsite loaded
  • Open WP-admin of that site and confirm that all assets loaded correctly

@adamziel
Copy link
Collaborator

adamziel commented Mar 5, 2024

Why does WordPress ship these .htaccess rules, though, if routing can be as easy as resolving the index.php file?

@bgrgicak
Copy link
Collaborator Author

bgrgicak commented Mar 5, 2024

Why does WordPress ship these .htaccess rules, though, if routing can be as easy as resolving the index.php file?

It's required to handle pretty permalinks. Otherwise, WordPress could work without it.
If a URL is site.com/something, Apache would attempt to load index.html in the something folder, this is where .htaccess ensures all requests end up on index.php in the root folder and get properly routed.

Not sure if this is the explanation you were searching for or is there something more complex?

@adamziel adamziel changed the title Remove/1054 wordpress redirect from sw Service worker: Remove WordPress URL rewrite logic Mar 5, 2024
@adamziel
Copy link
Collaborator

adamziel commented Mar 5, 2024

Why does WordPress ship these .htaccess rules, though, if routing can be as easy as resolving the index.php file?

It's required to handle pretty permalinks. Otherwise, WordPress could work without it. If a URL is site.com/something, Apache would attempt to load index.html in the something folder, this is where .htaccess ensures all requests end up on index.php in the root folder and get properly routed.

Not sure if this is the explanation you were searching for or is there something more complex?

Aha, thank you! Thinking about this more, I'm not sure. Say we're requesting either these URLs:

  • /site-1/wp-content/plugins/testplugin/handler.php
  • /site-1/wp-content/themes/twentytwentyfour/style.css

Without the rewrite rules, the .php request would be rewritten to the main index.php file while the style.css request would yield error 404. I think the only way to resolve these requests correctly is to implement these rewrite rules in the request handler:

RewriteRule ^([_0-9a-zA-Z-]+/)?(wp-(content|admin|includes).*) $2 [L]
RewriteRule ^([_0-9a-zA-Z-]+/)?(.*\.php)$ $2 [L]

@bgrgicak
Copy link
Collaborator Author

bgrgicak commented Mar 6, 2024

Looking at the code, this is where it fails to fetch static files.

The static file path is /wordpress/.... I'm not sure why the file doesn't exist when it's there.

@bgrgicak
Copy link
Collaborator Author

bgrgicak commented Mar 7, 2024

I spent some time today working on this and have created a regex const multisiteRegex = /^\/(scope:([.0-9])+\/)?([_0-9a-zA-Z-]+\/)?(wp-(content|admin|includes).*)/g; that we can use to detect multisite URLs.

Now I'm trying to understand whats the best place to do there rewrites.

We want rewrites to happen in Playground, not PHP-wasm to avoid adding WP specific code to PHP-wasm.

I tested adding a regex rewrite to the request handler and it worked, but static files that aren't part of the filesystem would still break.

const multisiteRegex =
			/^\/(scope:([.0-9])+\/)?([_0-9a-zA-Z-]+\/)?(wp-(content|admin|includes).*)/g;
		if (multisiteRegex.test(requestedPath)) {
			const replaceRegex = /\/(wp-(content|admin|includes).*)/g;
			requestedPath = requestedPath.replace(
				requestedPath.replace(replaceRegex, ''),
				''
			);
		}

@bgrgicak
Copy link
Collaborator Author

@adamziel I'm close. The code is all in place and rewrites work, but I can't get it to work consistently.
On the first load static assets are loaded correctly, and on reload (press enter on the path input in the header) remote assets break while the page content loads correctly.

I'm probably not adding rewrite rules in all places or they are not added to WebPHP in all cases.

@adamziel
Copy link
Collaborator

Let's look into that on our call later today

@bgrgicak
Copy link
Collaborator Author

Here is the error I wasn't able to recreate yesterday.

URL

Screen.Recording.2024-03-13.at.12.14.05.mov

@adamziel
Copy link
Collaborator

404 on static assets looks like the browser tries to fetch() from https://playground.test/scope:0.7227214969914926/test/wp-content/themes/twentytwentyfour/assets/fonts/cardo/cardo italic 400.woff2, but vite dev server cannot find anything under that URL. If you add a bunch of console.log() statements in the service worker to see what the input URL is and how is it handled (routed to worker thread vs handled as fetch()), what would it say about those requests?

@bgrgicak
Copy link
Collaborator Author

I'm getting closer. There are now tests to confirm rewrites work.

There are still some issues with fetching remote assets that I need to resolve.

@bgrgicak
Copy link
Collaborator Author

@adamziel this PR is now ready for review. The failing unit test is the same as in #1095.

@bgrgicak
Copy link
Collaborator Author

Issue for the failing unit test #1112

@adamziel
Copy link
Collaborator

I merged the other PR and rebased this one, let's see if the tests pass.

@bgrgicak bgrgicak requested a review from adamziel March 20, 2024 08:02
@bgrgicak bgrgicak requested a review from adamziel March 20, 2024 08:19
Copy link
Collaborator

@adamziel adamziel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great, thank you @bgrgicak! My last two asks would be:

  • The 0-playground.php comment I left
  • Updating the PR title and description to communicate what this PR does now

Thank you for your amazing work! 🎉

@bgrgicak bgrgicak changed the title Service worker: Remove WordPress URL rewrite logic Add WordPress rewrite rules Mar 20, 2024
@bgrgicak bgrgicak changed the title Add WordPress rewrite rules Add multisite rewrite rules Mar 20, 2024
@bgrgicak bgrgicak merged commit c988f06 into trunk Mar 20, 2024
5 checks passed
@bgrgicak bgrgicak deleted the remove/1054-wordpress-redirect-from-sw branch March 20, 2024 11:36
*/
export const wordPressRewriteRules: RewriteRule[] = [
{
match: /^\/(.*?)(\/wp-(content|admin|includes).*)/g,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this only match a single path segment? Or not? @bgrgicak

Suggested change
match: /^\/(.*?)(\/wp-(content|admin|includes).*)/g,
match: /^\/([^/]+?)(\/wp-(content|admin|includes).*)/g,

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, this should work with single and multiple paths. For example /scope:0.1/subsite/ and /subsite/.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 what about greedy vs non-greedy? E.g. how would this be handled /scope:03938/subsite/wp-content/plugins/myplugin/wp-content/image.jpg?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

E.g. how would this be handled /scope:03938/subsite/wp-content/plugins/myplugin/wp-content/image.jpg?

It would still work as expected and resolve as /wp-content/plugins/myplugin/wp-content/image.jpg.

Screenshot from 2024-03-22 10-57-41

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thank you for your patience! :-)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has been a great learning experience for me. I finally feel comfortable with Regex.

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

Successfully merging this pull request may close these issues.

2 participants