Refactored root_relative_url()

[joemaller]

• Switched from $_SERVER['SERVER_NAME'] to WP_SITEURL
• Fixed Undefined Index warning in wp-cli export
• Fixed bug where ports aren't correctly matched
• Added handling of scheme-less urls
• Simplified logic
• Improved readability by spreading long conditional across several lines

[joemaller]

This was supposed to be a small refactor, but I found a bit more than just rearranging the conditional and making an exception for wp-cli.

Was there a specific reason to pull $site_url from SERVER_NAME instead WP_SITEURL? In most cases, the urls to be cleaned are derived from WP_SITEURL anyway and setting WP_SITEURL from SERVER_NAME is common practice. $_SERVER could be used to reconstruct the url if we need to, but I don't think that will be necessary.

Running with that, @aaronhuisinga basically gave us a perfect solution in #91 with network_site_url(). That function conveniently falls back to site_url() if multisite is not enabled. So if we're using the value of WP_SITEURL anyway, the surrounding condition can be dropped and multisite is supported with the same code as standard WP.

That also fixes the problem with $_SERVER context, so the wp-cli warnings are gone.

Switching to parse_url() in #91 likely broke http scheme matching and almost certainly broke port matching -- though concatenating SERVER_PORT might have always been a little dodgy. If ports exist, they're extracted to their own index in the parse_url array. $_SERVER usually returns a "80" (as a string) unless there's an explicit port set.

Scheme matching also now handles the unlikely case of a scheme-less local url.

Whenever #93 is merged, I have a handful of unit tests ready to go as well.

[austinpray]

This is pro: your conditionals are so much better than that awful if statement lol

[Foxaii]

This is great. What are people's though on aligning the conditionals? I think it makes it easier to follow.

$hosts_match   = $site_url['host'] === $url['host'];
$schemes_match = $site_url['scheme'] === $url['scheme'];
$ports_exist   = isset($site_url['port']) && isset($url['port']);
$ports_match   = ($ports_exist) ? $site_url['port'] === $url['port'] : true;

[joemaller]

No problem, I forgot "hacker spacing" is a project preference. 😉 Gonna go look for a Sublime Text package to do that for me

[swalkinshaw]

I favour not aligning things these days. Just makes it harder to maintain in the long run and things break down with longer variable names.

[joemaller]

@swalkinshaw that's my preference as well. So can we just leave it?

[austinpray]

@joemaller it's only hacker spacing if I do it

[QWp6t]

meh on alignment. i agree it looks better. i agree it's not necessary.

[kalenjohnson]

We should keep it consistent throughout the project... and wouldn't that discussion be better on an issue if it's not consistent?

[JulienMelissas]

Good point @kalenjohnson - looks like we're using "hacker spacing" here, but that's it.

Personally, I prefer it, but that doesn't matter. It's more of what we're doing now. If we don't want to do it, there should make a separate PR IMO. Would take 10 seconds.

[austinpray]

http://en.m.wikipedia.org/wiki/Bikeshedding

This PR looks fine to me.

[QWp6t]

👍

Looks like everyone agrees spacing is non-issue. I can't merge from my phone (app I'm using doesn't have that option), so I'll let someone else handle that.