[Blueprints v2] Add support for "enableMultisite" step

[JanJakes]

Resolves #130.
Closes #131.

[adamziel]

It's a promising start @JanJakes, thank you! We could use a couple of unit tests here – LMK if you'd like to connect on how to build them.

[JanJakes]

@adamziel I'm just finishing the tests; I will push it soon. Should the behavior be as close to the v1 behavior as possible? For example, when a site is already multisite, should it fail with This already is a multisite installation., as v1 does?

[adamziel]

Should the behavior be as close to the v1 behavior as possible? For example, when a site is already multisite, should it fail with This already is a multisite installation, as v1 does?

For now, yes. It may be cumbersome in some cases but could prevent catastrophic failures in other cases.

[JanJakes]

There are some interesting test failures in the macOS and Win builds, while Ubuntu passes. It's likely related to how some $_SERVER variables are set by WP CLI. I'll check it out further.

[JanJakes]

@adamziel This should be ready now. The test failures seem to be unrelated/flaky.

[adamziel]

Nitpick: Spaces vs tabs – the comments are visually less indented than the lines they refer to

[adamziel]

Really good work @JanJakes! My only notes are about code formatting – feel free to merge once that's sorted out!

[JanJakes]

@adamziel There's one more thing that came to my mind: The populate_network() function calls wp_guess_url() and that relies on some $_SERVER variables like $_SERVER['HTTP_HOST'] and $_SERVER['REQUEST_URI'] being set. I think this affects which site URL the main site in the network will have set.

When using the CLI via php bin/blueprint.php exec, these may be required to be set based on the --site-url parameter. It seems that WP CLI does something like that, so we may mirror the behavior.

I will do some testing via the php bin/blueprint.php exec command and compare the behavior with WP CLI.

[JanJakes]

@adamziel OK, so particularly for the enable-multisite path, the $_SERVER values are not important as the populate_network path seems to be fetching the siteurl option for both the network and the initial blog.

That said, would it be safer to add something like the below to the subprocess script prefix? It's as per WP CLI.

[adamziel]

@JanJakes would we be adding that just in case? Or did you run into an actual case where lacking those values is an issue? I'm worried about confusing WordPress into thinking we're running via a CGI SAPI when we're actually running as CLI.

[JanJakes]

@JanJakes would we be adding that just in case? Or did you run into an actual case where lacking those values is an issue? I'm worried about confusing WordPress into thinking we're running via a CGI SAPI when we're actually running as CLI.

@adam At this stage, it only seems to be a precaution. It doesn't affect the multisite step as it is, and likely not other steps we have now. What's a bit tricky, though, is that in many of the steps we call wp-load.php, and some of the WP functionality may rely on these constants. WP CLI sets these as per the --url parameter and we have a mandatory --site-url parameter that's actually passed to WP CLI for the WP installation but not set in this way in our steps.

[JanJakes]

@adam At this stage, it only seems to be a precaution. It doesn't affect the multisite step as it is, and likely not other steps we have now. What's a bit tricky, though, is that in many of the steps we call wp-load.php, and some of the WP functionality may rely on these constants. WP CLI sets these as per the --url parameter and we have a mandatory --site-url parameter that's actually passed to WP CLI for the WP installation but not set in this way in our steps.

@adamziel Just for the record, I didn't add this. If you'd prefer to do it, I can create a new PR.

[adamziel]

I'm on the fence. I looked up why WP-CLI does that but the decisions are not well documented. It seems like that's just their way of supporting the --blog and --url CLI options, not something inherently required by WP:

• wp-cli/wp-cli@ed8f887
• --url global parameter wp-cli/wp-cli#69
• wp-cli/wp-cli@8d96313

Lacking these attributes didn't give me any troubles yet. At the same time, when I had them set, I occasionally saw WordPress trying to send the Location: header from a CLI script.

[JanJakes]

@adamziel We can also wait and see if it's needed at some point. In WP CLI, I noticed it seems to be needed for multisite install but not multisite conversion (as we're doing). There are some cases when the CLI commands land on wp_guess_url() which needs these variables. In general, I think WP Core is not really built with CLI in mind, so you can definitely run into header() calls that will be a noop on the CLI.