Support Ports on Multisite Domains #5675
Conversation
Rather than sanitizing the domain as if it was a username we should sanitize it using the character set defined in the `site-new.php` form.
You can now install WordPress as multisite by setting the `LOCAL_MULTISITE` environment variable to `true`.
|
Thanks @JJJ, any suggestions on moving this forward? I didn't get a reply to anyone in #core on Slack and I can't seem to get any replies on the Trac ticket. |
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
|
👋 @ObliviousHarmony thanks for working on this! Just wanted to acknowledge that I have seen this, but I can't promise that I'll have the capacity to take a look for the next few weeks. I know it's frustrating, but it's not uncommon for it to take months or years for a given change to get the needed feedback and testing. Especially since a few of the contributors previously active on the ticket have no sponsored contribution time at the moment. One thing I recommend for clarity's sake. If you could go through the existing concerns and just briefly document your thinking around how you solved them, that would help someone dive in looking at the ticket history. |
| @@ -53,6 +53,9 @@ LOCAL_DB_TYPE=mysql | |||
| ## | |||
| LOCAL_DB_VERSION=5.7 | |||
|
|
|||
| # Whether or not to enable multisite. | |||
| LOCAL_MULTISITE=false | |||
There was a problem hiding this comment.
With this now available, would it make sense to turn this to true for the multisite test jobs for PHPUnit?
If not, then we should at least have some type of test that ensures the environment starts and works when multisite is enabled. That could potentially be a separate workflow that tests all of the related local environment commands, and perhaps better off as a separate ticket.
There was a problem hiding this comment.
It's worth keeping in mind that PHPUnit tests create their own tables and wp-config.php file. The environment variable just switches install to multisite-install and it wouldn't have any effect on the PHPUnit tests.
It's probably not worth creating a test for installing since all it does is run multisite-install instead of install. The rewrite rule change is covered by the existing test workflows and the parsing is covered by PHPUnit tests.
It's been months and I'm not totally sure why I left this here in the first place. Subdomains can have ports too, obviously!
There were a number of tests that failed when `WP_TESTS_DOMAIN` has a port. This commit fixes all of those tests so that they will pass with both bare hostnames and those with ports. To discover these problems I added a port to my local `wp-tests-config.php` and ran the test suite.
In order to make it easily possible to run the test suite with a different domain this new environment variable changes what's set in the tests config. This is good for things like testing it with ports.
Previously, the port was not being used when requesting the site for multisite installations. This breaks oEmbed when a port is used.
The self link should include the port instead of just using the host. This way, it will link to the correct site.
We only set the protocol to `https://` when the url is for the current request. Since `HTTP_HOST` will include a port we need to consider that too.
Not only does this let us test this PR in CI but it also ensures that future PRs will still work with ports in the test domain.
There was a problem hiding this comment.
Thanks for taking a peek @desrosj,
Since it's been a few months I decided to take another pass and make absolutely sure this change works as expected. A big part of that was buttoning up the use of WP_TESTS_DOMAIN in the PHPUnit suite so that it can be ran with a port in WP_HOME. In the process, I even found a few really small cases where WordPress breaks when on a host using a non-standard port 😄
One thing I recommend for clarity's sake. If you could go through the existing concerns and just briefly document your thinking around how you solved them, that would help someone dive in looking at the ticket history.
Really, the only concern in the ticket from folks is uncertainty around what aspects of multisite might break with a port present in the domain. There are, however, also comments making similarly minimalistic changes as I have and reporting that everything works correctly.
@spacedmonkey noted two years ago that "adding a port to the domain breaks things all over WordPress" but that hasn't been my experience. Even WordPress' own development environment uses a non-standard HTTP port and that use-case seems completely supported.
The only potential issue I see with storing the port in the domain for the site is that WP_Site_Query now requires the port if one is set. This wouldn't impact any existing multisite installations and I seriously doubt many users are creating production sites with a port in the first place.
In my case, I'd like to support ports so that we can shard E2E tests using a single environment. This is great for running tests more quickly without having to spin up a bunch of environments. The same command can be used both in CI and locally with much better performance. It's also super useful for isolating groups of tests due to them affecting some global state.
| @@ -53,6 +53,9 @@ LOCAL_DB_TYPE=mysql | |||
| ## | |||
| LOCAL_DB_VERSION=5.7 | |||
|
|
|||
| # Whether or not to enable multisite. | |||
| LOCAL_MULTISITE=false | |||
There was a problem hiding this comment.
It's worth keeping in mind that PHPUnit tests create their own tables and wp-config.php file. The environment variable just switches install to multisite-install and it wouldn't have any effect on the PHPUnit tests.
It's probably not worth creating a test for installing since all it does is run multisite-install instead of install. The rewrite rule change is covered by the existing test workflows and the parsing is covered by PHPUnit tests.
|
@ObliviousHarmony I pushed a couple of changes to get the PHPUnit tests working on this PR. There's one failing job which is the newly introduced job that tests a single site installation with a domain name that includes a port number. The failure is only caused by the |
|
Thanks @johnbillion! Yeah, that's definitely tricky. I wouldn't want to do anything as broad as removing the port before writing the file but I don't see any real alternative. Modifying the test fixture this generates feels like a bad idea anyway. How would you feel about expanding the action's step to allow for certain differences in certain cases? The matrix is pretty extensive and I don't know that an exception in this one case would be that bad. I added this test because (as this PR shows) there are a ton of tests that break with a port that needed updating. It seems wise to have a test that uses a port to make sure everything works correctly. In the interest of moving this forward, however, it might make sense to pull that test out. The tests pass and so I think that's a good indicator for this PR at least. The problem |
| @@ -36,7 +36,7 @@ jobs: | |||
| # | |||
| test-with-mysql: | |||
| name: PHP ${{ matrix.php }} | |||
| uses: WordPress/wordpress-develop/.github/workflows/phpunit-tests-run.yml@trunk | |||
| uses: ObliviousHarmony/wordpress-develop/.github/workflows/phpunit-tests-run.yml@fix/multisite-port | |||
There was a problem hiding this comment.
Just noting @johnbillion that this needs to be changed back before merging so that it doesn't break the workflow. We don't need to run actions using a random contributor's branch 😄
There was a problem hiding this comment.
I pushed two commits:
- 0665ca4 which skips the REST API client fixture test when the tests run with a port in the host name, I couldn't think of any other way around this.
- 49365cb which ensures the
homeandsiteurloptions are updated with the new value when the path is updated from the site editing screen.
I've reviewed everything else in this PR and it looks good.
|
Thanks for the review and helping get this across the finish line @johnbillion, I've been pretty swamped this week 😄
It'll be nice to get this across the finish line. I'm planning to use this to run E2E test shards in WooCommerce without needing to spin up a bunch of identical environments. Faster tests both locally and in CI 😄 |
This should cover all of the areas that break when using multisite with a port.
Trac ticket: https://core.trac.wordpress.org/ticket/21077
This also addresses https://core.trac.wordpress.org/ticket/52088 since I had to get it working in order to test this PR out locally while developing it.
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.