Env: mu-plugins and parent directories #21229
Conversation
noahtallen
added
[Type] Enhancement
|
Size Change: 0 B Total Size: 842 kB ℹ️ View Unchanged
|
noahtallen
force-pushed
the
add/env-mu-plugins-support
branch
from
c8a1a49
to
4995d27
Compare
noahtallen
force-pushed
the
add/env-mu-plugins-support
branch
from
1e039d6
to
f047b5b
Compare
|
Alright, this should be ready for another look. I also added two more improvements:
|
noahtallen
force-pushed
the
add/env-mu-plugins-support
branch
from
f047b5b
to
e38aab7
Compare
|
@noisysocks not sure why, but the build in Travis fails running |
|
It might be that you ran |
packages/env/lib/env.js
Outdated
| const [ themeSource ] = config.themeSources; | ||
| if ( themeSource ) { | ||
| // Index 0 contains "name" (the header of the output), so use next index. | ||
| const [ , firstTheme ] = themeOutput.out.split( '\n' ); |
There was a problem hiding this comment.
Are we guaranteed that the first theme listed by wp theme list is the first theme specified in .wp-env.json's "themes" array? It may be that WP-CLI lists themes alphabetically.
There was a problem hiding this comment.
good point. I changed it to still activate the first theme in the array if we are using the array format.
I wonder, do we want this functionality? If the user activates a different theme while using wp-env, wouldn't this overwrite their choice every time they restart the container?
There was a problem hiding this comment.
That's a good question. I think it makes sense to have wp-env activate a theme because it mirrors what it does for plugins. But now you have me questioning myself... 😀
There was a problem hiding this comment.
Yeah.. I mean, either are easy enough to activate internally. I guess the difference is that if you list all plugins, you probably want them all to be running most of the time. But with themes, only one can run at a time, so why not let the user choose in wp-admin? (I bet we'll get a "bug report" about the behavior at some point ;))
packages/env/lib/env.js
Outdated
| ); | ||
| await dockerCompose.run( | ||
| containerName, | ||
| 'wp plugin activate --all', |
There was a problem hiding this comment.
I just realised that this approach of activating all plugins using wp plugin activate --all will mean that all of the Gutenberg test plugins (packages/e2e-tests/plugins) will be activated by default. That's probably not what we want for the default Gutenberg development environment.
I'm not sure of the best way around this. Some ideas:
- We could look at adding support for plugins that are not activated automatically.
- We could look at adding support for plugins that are only loaded into the tests environment.
- We could go back to hardcoding this for Gutenberg.
There was a problem hiding this comment.
That's an interesting conundrum. My intuition would be to do number 2:
- number 3 isn't an ideal choice any way you cut it
- number 1 means we wouldn't directly support test environment mu-plugins. I assume we want to automatically activate them for the test environment :)
So for the api, maybe we change the source format in wp-env to accept string | object where the string is a normal source string (current approach) and the object has the format:
{
"source": SourceString,
"activateInEnv": "development" | "tests" | "none"
}Then, we activate the plugin depending on the environment, or don't activate it at all if none is passed. (just an idea on how to make this a bit more flexible in the long run).
BTW, will this approach work out of the box with the current docker setup? i.e. does test-cli wp activate plugin only activate the plugin on the test WP and not on the production one?
There was a problem hiding this comment.
- I assume we want to automatically activate them for the test environment :)
The E2E tests themselves are responsible for activating the plugins that they need, e.g:
So for the api, maybe we change the source format in wp-env to accept string | object where the string is a normal source string (current approach) and the object has the format:
I'm not a huge fan on the extra weight this adds to .wp-env.json. I worry about making this file so complex that it doesn't provide much benefit over using docker-compose.yml.
BTW, will this approach work out of the box with the current docker setup? i.e. does test-cli wp activate plugin only activate the plugin on the test WP and not on the production one?
Yes, that will work because the instances use different databases.
There was a problem hiding this comment.
I'm leaning towards option one because it maintains one principal of wp-env that I like which is that the two instances it spins up are identical: same core code, same plugins, same themes. It's really nice being able to make this assumption when e.g. writing and debugging E2E tests.
In other words, I think let's add some way of defining a plugin that isn't activated, similar to what you suggested above:
{
"plugins": [
".",
{ "source": "./packages/e2e-tests/plugins", "active": false }
]
}This is tough to implement, though. We can't use wp plugin activate --all since that will activate everything. We also can't iterate through the plugins with .active = true and perform wp plugin activate ${plugin} because some of the paths will be to directories containing plugins instead of to plugins themselves. We might have to do some weird stuff involving wp plugin list, wp plugin path, and wp plugin activate.
😕
There was a problem hiding this comment.
I like that idea. For example, active could default to true. I'm exploring this in another branch -- I think it's a good opportunity to make sure directory support works in general too.
There was a problem hiding this comment.
what about mu-plugins that only need to be activated in the test environment?
noahtallen
force-pushed
the
add/env-mu-plugins-support
branch
from
b72453f
to
93ef9a6
Compare
- Sources can be a single source or array of sources. Single sources map to the matching wp-content directory. Arrays of sources are each placed inside. - "mu-plugins" can be used in the config object with the same source string format as themes and plugins.
- Use `wp plugin activate --all` for plugins - Use first listed theme from wp theme list
noahtallen
force-pushed
the
add/env-mu-plugins-support
branch
from
c9c1c5f
to
3fd6ad1
Compare
|
@noisysocks I think this is ready for another look (just rebased on master and verified some things) see #21710 for my investigation into adding support for an The only outstanding questions I see are:
I'm not sure if either of those questions should block the PR though ;) |
Environment DifferencesI think it's clear now that the different requirements of the development and tests environments require an API like that of In Gutenberg, for example, we can't load Theme ActivationChoosing a theme to activate for the user based on a list's order is not a nice explicit API. Also, overwriting their current theme every time they run the command could get annoying. I think that we shouldn't activate any theme by default but instead have a new top-level key, In Gutenberg, for example, we wouldn't want the development theme to be anything specific, and we wouldn't want to overwrite it every time we ran Adding an extra key to the theme sources is also an option, but it's more verbose and opens up questions of what happens when two or more theme sources have the same environment key. |
|
That's a fair analysis, thanks @epiqueras! I tend to agree to with you about being able to specify an environment for the different options. I'm pondering the situation, and wonder if there is any concern about making the API too complex? This does seem like a very basic requirement for most tools like this, though. I know @noisysocks has mentioned that he would like the dev and tests environment to be as similar in possible, so I'm curious to hear his thoughts about it.
I think this would definitely be a much better API than the implicit one we have now. |
|
@epiqueras in the mean time, I still think we want to add parent directory/mu-plugin support. (Additionally, one could add an override file to not load any of the test mu plugins as a workaround for now.) I'll rebase this and add some of the suggestions (like not loading themes). Then we could review/ship this and (IMO) abandon #21710 |
|
Got some feedback here: #21595 (comment) Where they mentioned that their exceptions for a wp-content directory would not just be to load the content directory, but also to load any other specified plugins and items. Currently, this PR wouldn't work with that approach, since we treat the |
|
I think that instead of:
We should add {
"mappings": { "wp-content/themes": "../path/to/themes" }
}This will be way more flexible, easier to understand, it can be done in parallel to what we already do with |
|
Neat idea :) the only reasons I could see not going this route:
|
I think "volumes" is a less understood term, but the transparency might be worth it. @noisysocks, what do you think?
They let you combine multiple individual themes/plugins from a multitude of different sources and do more out of the box like activation. Mappings are a power user feature.
It's better than supporting a bloated and hard to document configuration schema. I don't think its too advanced if we dumb it down to "mapping folders" and avoid getting into more complex docker volume options. |
|
What does Docker do, though, if you try to map a volume which has an ancestor that is already mapped? {
"plugins": [ "/foo" ],
"mappings": {
"wp-content/plugins": "/bar"
}
}Will the plugin in |
|
Yeah, I think we just need to order them correctly. |
|
cool. if i get a chance, I'll refactor my PRs and stuff to work with this approach. I hope I can do that before next week? |
|
Closing in favor of #22256 (for parent directories) and #22257 (for mu-plugins) I haven't done anything to replace ##21710 yet.
I tested this in #22256 and it does work. We also currently map |
Description
This PR accomplishes two goals:
Resolves #20790
Fixes #20923 (because we no longer use flatMap)
How has this been tested?
Ran the tool locally in Gutenberg and another plugin I develop.
Types of changes
New feature.
Approach
For mu-plugins, specify an array of any valid
Sourcein your.wp-env.jsonfile:{ "mu-plugins": [ "../mu-plugin-thing" ] }For parent directories, specify a single string instead of an array:
{ "themes": "../path/to/themes" }Then, that themes directory is mounted as
wp-content/themes. This way, you can control the entire plugin/mu-plugin/theme directories if needed.Checklist: