Remove url guesswork

[mboynes]

When working with a multisite installation with variable wp-config files, e.g.

if ( file_exists( dirname( __FILE__ ) . '/wp-config-local.php' ) ) {
    require_once dirname( __FILE__ ) . '/wp-config-local.php';
} else {
    ...
}

or even variable DOMAIN_CURRENT_SITE declarations, WP-CLI doesn't correctly parse the url (and, in fact, fails silently). To resolve this, WP_CLI requires using the --url flag, or using a yaml config to explicitly set the url.

This PR aims to resolve that by removing the guess, and setting the URL after the config is loaded. Perhaps there's rationale as to why the url must be set before the config is loaded; if so, can anyone explain why that is?

If there's interest in moving forward with this, I'll write tests for it.

Refs #263.

[danielbachhuber]

To summarize, because we'd be maybe changing the URL until config has been eval'd, --url is now potentially incorrect until the entirety of Runner::before_wp_load() has run.

There's a lot of code that runs in Runner::before_wp_load(). For instance, wp core *, wp config *, and wp db * all run in that method, as well as any commands hooked into the before_wp_load hook. It seems like making this change could break something in a bad way. I'd want to see lots of test coverage for various scenarios before I'd seriously consider it.

An alternative: what if we added some fancy regex to sniff out and execute those conditional includes?

To resolve this, WP_CLI requires using the --url flag, or using a yaml config to explicitly set the url.

This will always be the most explicit, and best, way to define your URL context.

[mboynes]

It seems like making this change could break something in a bad way.

I know, I actually didn't expect it to work. And when it did, I really didn't expect the existing tests to all pass; you can imagine my surprise when they did.

The url isn't really required for single-site installs. It is for multisite installs, because they will redirect the request if the url doesn't match (which is why the initial example fails silently), but single-site installs are a bit more flexible.

An alternative: what if we added some fancy regex to sniff out and execute those conditional includes?

We could certainly try for some common exceptions, like wp-config-local.php, but I don't think it's worth it, because we'd still just be guessing.

wp core *, wp config *, and wp db * all run in that method, as well as any commands hooked into the before_wp_load hook.

If it weren't for the hook, I think we'd be safe. I checked the core, core config, and db commands and as far as I can tell none of them use the url except install and multisite-install which both explicitly require it as a flag. The hook is enough to take me back to the drawing board, I think.

The gist of what I'm trying to resolve here is two-fold. First, it's too easy to forget to explicitly set the URL, and the current guesswork is good enough to make the anomalies a head-scratcher. Second, having a yaml config or passing the url flag seems unnecessary, because the data is available -- if not in the config, then in the DB. If we can suss out the url without requiring it from the user, that makes wp-cli that much more simple to use. Here are a couple other ideas:

• What if we read the value of siteurl from the DB if the url isn't explicitly set otherwise? Can we do that if the db creds haven't been read from the config yet?
• What if we processed the config earlier in the process? Aside from core config, would anything else be affected by that? The hook is still a problem in this case, I suppose.

I'm open to other ideas and happy to work on the solution if we come up with a good option.

[danielbachhuber]

This sounds similar to #854, which I'm really tempted to reopen.

However, if your database credentials are in local-config.php, we're back at square one.

[scribu]

Why exactly is the before_wp_load hook relevant here? It seems like commands that run before WP is loaded shouldn't care about the URL (unless they explicitly accept it as a parameter, of course).

If this change allows us to get rid of those ugly PHP-parsing regexes, without making things worse on multisite, I think it's worth pursuing.

[scribu]

That URL guessing code was first introduced in 6c59680 (#42). Back then we were loading the whole wp-config.php, which prevented us from accessing the multisite constants before the rest of WP got loaded, like you do here.

Bottom line: this might actually work.

[mboynes]

I think the next step then would be to add a battery of tests for it. I'll try to get that done this weekend and we'll go from there.

[mboynes]

I apparently got distracted and forgot all about this. I'll try to work up some tests soon, but if anyone else can pitch in that would be great. I think this would help solve a ton of issues surrounding #1631 (and the slew of tickets that it references, like #1514, #1596, and #1667).

[danielbachhuber]

Continued in #2079. Thanks for the idea, @mboynes !