wp-env: Add multisite support #67845
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mcsf
added
[Type] Enhancement
|
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 Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @jonnynews. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
This is really cool. It will also help us provide e2e test for multi-site as well if needed. :) |
|
|
||
| // -eo pipefail exits the command as soon as anything fails in bash. | ||
| const setupCommands = [ 'set -eo pipefail', installCommand ]; | ||
|
|
||
| // Bootstrap .htaccess for multisite |
There was a problem hiding this comment.
I did it in a similar way. #67852
Should we check the file permission on this PR too?
There was a problem hiding this comment.
Oh, funny coincidence that we should be working on the same thing on the same day! 😀
About the file permissions: did you run into any trouble in the current setup, or are you concerned with anything? Since in this PR we are creating the htaccess file by invoking shell commands inside the container, I expect the resulting file to have "reasonable" permissions and ownership — unless someone really messed up the container's mask.
In contrast, in my experience, those problems arise in scenarios where files are copied from the host to the container or when mounts are set up.
There was a problem hiding this comment.
It could be considered defensive programming, I have had file permissions issues since I started setting servers 😆 . It's true that everything by default should work and that the container's mask could be the only issue.
There was a problem hiding this comment.
I pushed 0e2b031, but I'm of two minds between:
- Yes, it's defensive programming, so it's better to do it — what's the harm?
- But if this is ever needed then there's something really wrong with the container (and a fresh container at that!), so isn't this a form of cargo cult programming?
There was a problem hiding this comment.
I think you can skip it. We don't recommend wp-env for production sites, right?
There was a problem hiding this comment.
I don't think so. The documentation's headline reads: wp-env lets you easily set up a local WordPress environment for building and testing plugins and themes. It’s simple to install and requires no configuration.
I'll revert. :)
There was a problem hiding this comment.
I thought that this .htaccess file was written as part of the multisite install?
|
For referenace - WordPress/wordpress-develop@7750458 |
cbravobernal
approved these changes
Dec 12, 2024
|
|
||
| // -eo pipefail exits the command as soon as anything fails in bash. | ||
| const setupCommands = [ 'set -eo pipefail', installCommand ]; | ||
|
|
||
| // Bootstrap .htaccess for multisite |
There was a problem hiding this comment.
I thought that this .htaccess file was written as part of the multisite install?
A single-site instance can be converted to multisite, but not vice-versa. Whenever it detects config changes, wp-env triggers — among other things — So it's done implicitly. That's why the opposite conversion doesn't happen. We would have to:
I think that, for the purposes of the tool, the current situation is good enough for it to be merged. Thoughts? |
mcsf
force-pushed
the
add/wp-env-multisite
branch
from
4b89664 to
74f7b98
Compare
|
Size Change: 0 B Total Size: 1.83 MB ℹ️ View Unchanged
|
|
I'm going to merge this based on Carlos's approval — @spacedmonkey, we can follow up as needed. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
closes #27961
Add multisite support as an option to wp-env.
{ "env": { "development": { "multisite": trueWhy?
Makes it easier for developers to bootstrap multisite environments, especially for ease of feature testing and quality control.
How?
multisite) in the form of a boolean, defaulting to false.startcommand, if the environment has themultisiteflag, then:clicontainer to runwp core installon the environment'swordpresscontainer, runwp core multisite-install..htaccessfile in the environment'swordpresscontainer containing the rewrite rules required by the multisite setup.Testing Instructions
Remember to
npx wp-env destroyin between tests..wp-env.jsonsets the multisite flag to true./wp-admin/network/sites.php).