Function signature of wp_parse_url() does not match parse_url()

[johnbillion]

The function description for wp_parse_url() states that the function is:

A wrapper for PHP's parse_url() function that handles edgecases in < PHP 5.4.7

PHP's parse_url() function accepts an optional second parameter for specifying which URL part should be returned. This parameter is not supported in wp_parse_url(), therefore either wp_parse_url() should be modified so this parameter is accepted, or its documentation should be corrected.

The former is preferable, the latter is easier.

[jrf]

Add $component parameter - includes unit tests

[jrf]

The patch also includes a minor performance improvement to only execute the work-arounds for PHP < 5.4.7 compatibility when the code is actually run on a lower PHP version.

[paulschreiber]

+1

[johnbillion]

In 38449:

HTTP API: Add a $component parameter to wp_parse_url() to give it parity with PHP's parse_url() function.

Fixes #36356
Props jrf

[johnbillion]

This fatals on < 5.4.7 due to line 663 trying to unset array elements of $parts when $parts is a string.

Ref: ​https://travis-ci.org/aaronjorbin/develop.wordpress/jobs/156277415

[johnbillion]

In 38450:

HTTP API: Prevent a fatal error on PHP < 5.4.7 due to changes introduced in [38449].

Fixes #36356

[johnbillion]

More failures: ​https://travis-ci.org/aaronjorbin/develop.wordpress/builds/156288436

[johnbillion]

In 38452:

HTTP API: The tests for wp_parse_url() can't be strict on type because this causes the tests to fail on PHP 5.2 which, bizarrely, returns the results of parse_url() (when called with a $component parameter) in a different order to later PHP versions.

Fixes #36356

[johnbillion]

Okay we have an actual failure now, on PHP 5.2.

test_wp_parse_url_with_component with data set #17 ('/path/http://example.net/', 1, NULL)
Failed asserting that 'placeholder' matches expected null.
tests/phpunit/tests/http/http.php:125

[johnbillion]

In 38453:

HTTP API: Separate the test for wp_parse_url() with -1 as its component into a separate test, so the remaining tests can use strict type checking. This helps avoid gotches with the potentially empty values (ie. null) that we're testing for.

See #36356

[johnbillion]

In 38456:

HTTP API: Revert changes to wp_parse_url() while PHP 5.2 errors are investigated.

See #36356

[jorbin]

Anyone that has a 5.2 install handy and can help test, please do.

[jrf]

Complete patch including all previous changes and passing for PHP 5.2

[jrf]

Ok, I've finally had some time to take another look at this, including the build failures.

The patch I've just uploaded includes:

• The original patch
• The additional changes made by @johnbillion
• A fix for the PHP 5.2 failure - or rather a code logic fix which will prevent the PHP 5.2 failure.

The code logic change basically looks at what $component parameter was passed and for relative URLs bows out early if the $component is either scheme or host - which won't be available for relative URLs anyway.
It also only removes the placeholders when the $component is the default -1 as that's the only time the $parts array will actually be an array.

Minor additional change: changed the protected static $full_test_url in the test file to a class constant.

The change has been tested on PHP 5.2 - 7.1 and the unit tests pass.

I've also had travis run the unit tests (just for the http group which is affected by this change) and the build passes there as well: ​https://travis-ci.org/jrfnl/develop.wordpress/builds/164150049

[johnbillion]

LGTM

[johnbillion]

In 38694:

HTTP API: Add a $component parameter to wp_parse_url() to give it parity with PHP's parse_url() function.

Fixes #36356
Props jrf

[peterwilsoncc]

Reopening, PHP7 doesn't cope with protocol relative URLs in some circumstances.

This will need a dedicated test case in Tests_HTTP_HTTP::parse_url_testcases as it's only coincidence that caught it.

$ php -v
PHP 7.0.9-1+deb.sury.org~trusty+1 (cli) ( NTS )

$ wp eval 'var_dump( wp_parse_url( "//fonts.googleapis.com/css?family=Open+Sans:400&subset=latin" ) );'
array(2) {
  ["path"]=>
  string(26) "//fonts.googleapis.com/css"
  ["query"]=>
  string(33) "family=Open+Sans:400&subset=latin"
}

$ wp eval 'var_dump( wp_parse_url( "//fonts.googleapis.com/css/family/Open/Sans/400/subset/latin" ) );'
array(2) {
  ["host"]=>
  string(20) "fonts.googleapis.com"
  ["path"]=>
  string(38) "/css/family/Open/Sans/400/subset/latin"
}

[peterwilsoncc]

36356-tests.diff​ includes two forms of Google font urls that are particularly good at causing bugs in various versions of PHP.

ref: ​https://3v4l.org/IpTl1

[jrf]

Fixes the additional test cases and some more.

[jrf]

Unit tests for the new functions introduced

[jrf]

Sometimes PHP just drives me up the wall....

@peterwilsoncc Thanks for the additional test cases. I've fixed the handling of these.

I've also come up with two more test cases which previously weren't handled at all:

http://www.test.com/path1/path2/&q=a
http://www.test.com/path1/path2/file.php&q=a

PHP would return an empty query for these and have the &q=a part within the path.

Both @peterwilsoncc's as well my own additional test cases are now handled correctly cross-version PHP 5.2 - 7.1 + HHVM by the additional patch which I just uploaded.

The patch includes two new internal functions to prevent code duplication.
The patch includes updated documentation to reflect the changes.

The second patch I've uploaded contains perfunctory unit tests for the new functions I introduced.

A travis build which covers the groups which contain the http tests as well as the template tests which contained the Google URLs (now also added to the http tests) passes on the combination of the two new patches (fix + additional tests): ​https://travis-ci.org/jrfnl/develop.wordpress/builds/164235958

[johnbillion]

The colon in those Google Fonts URLs is actually an illegal character; the colon should be encoded. However, I'm not about to pick that fight.

[jrf]

@johnbillion You are completely correct and waiting for that to be solved is another matter. In the mean time, it will work.

One thing I'm now wondering though: should we maybe account for the fact that it is an illegal character and encode it within this function so the output will not contain the illegal character ?

[johnbillion]

I think that's beyond the scope of this particular ticket. Maybe it could happen in esc_url() though.

[jrf]

FYI: in the mean time I've also run a complete travis build on the master + patch (not just for the affected groups) and that passes as well: ​https://travis-ci.org/jrfnl/develop.wordpress

[swissspidy]

In the meantime we could also resolve this in wp_dependencies_unique_hosts() or in the relevant test so the build is not failing anymore

[jrf]

@swissspidy The 36356-wp_parse_url-additional-edge-cases.patch I send in two days ago will fix the build - see the travis build report: ​https://travis-ci.org/jrfnl/develop.wordpress/builds/164317694 - this is a complete run on master with both patches from two days ago applied.
The additional 36356-unit-tests-for-new-functions.patch contains unit tests for the two new functions the edge cases patch introduces.

[peterwilsoncc]

@jrf The === $url[0] in that patch will throw a notice if an empty string is passed to the function, are you able to add a couple of invalid use cases while refining the tests? I think this has been in WP since forever and I didn't think to test it over the weekend.

From rudimentary testing, placeholder schemes and hosts seem the most reliable across versions of PHP. Can they be used and unset generally? Version sniffing is proving unreliable.

[jrf]

@peterwilsoncc

The === $url[0] in that patch will throw a notice if an empty string is passed to the function, are you able to add a couple of invalid use cases while refining the tests? I think this has been in WP since forever and I didn't think to test it over the weekend.

While I agree that's an issue, it's not an issue with the patch as it's been - like you say - in WP since forever.

I detect a distinct case of scope creep here. This ticket is about adding the $component parameter (without breaking anything else), nothing more.
Variable & type checking has never been WP's strong suite, this function is no exception and the adding of the $component parameter has already made improvements in that respect (maybe even more than the ticket warrants).

I will add the empty string check in as it's an easy fix, but for anything else, I'd suggest opening a separate ticket.

From rudimentary testing, placeholder schemes and hosts seem the most reliable across versions of PHP. Can they be used and unset generally? Version sniffing is proving unreliable.

Version sniffing was already removed in my edge-cases patch from three days ago, so I don't understand what you're talking about.
Similarly, placeholder schemes and hosts are already being used and unset when appropriately, so what is your point here ?

[jrf]

Fix for = $url[0]. Includes additional unit tests.

[jrf]

Additional patch to fix the potential undefined notice uploaded.

A travis unit test run on the current master + last three patches (which have not been committed to core yet) for just the relevant groups has been run and passes: ​https://travis-ci.org/jrfnl/develop.wordpress/builds/164806773

A complete travis unit test run on the current master + last three patches is currently running and I expect it to pass as well: ​https://travis-ci.org/jrfnl/develop.wordpress/builds/164806903

[johnbillion]

@peterwilsoncc Can I assign you to take a look at this today please and expand on your feedback?

[peterwilsoncc]

@jrf Looks like I was looking at the wrong three day old version file re: version compare.

Combined version of your patches in 36356.diff​.

Looking over it this-eventing.

[peterwilsoncc]

@johnbillion 36356.2.diff​ incorporates my second comment above about simplfying matters by using placeholder schemes and hosts as a matter of course.

@jrf I've looked over the spec and can't find anything about query parameters beginning with &, are you able to send a pointer.

Somewhat complicated by the fact another part of the build is failing, I get consistent results at ​https://travis-ci.org/peterwilsoncc/WordPress/builds/164890491

[jrf]

@peterwilsoncc

I've looked over the spec and can't find anything about query parameters beginning with &, are you able to send a pointer.

This is similar to other invalidly formatted urls being passed to this function and being corrected by it. Invalid formats are not in the specs.

As you removed the fix now, you might as well remove the tests for it as they do not have any added value unless the fix for it is in (and the unfixed results just don't make sense).

I get consistent results

The HHVM fix has been removed in your patch - will that still be caught now ? HHVM will sometimes take the :### in the query string of the google font urls and use it as the port. See my previous fix for that.

Looked over the rest of the code.

I generally like the simplification. Wished I dared to do that. I considered it, but my experience with contributing to WP core has been that any time I change things drastically, the change gets rejected out of hand, so glad you have the whuffies to do it.

Only other point - I would suggest wrapping the unset foreach at the end of the function within a if ( false !== $parts ) {}

[peterwilsoncc]

36356.3.2.diff​ for history while I wait for the ​full test suite to run.

@jrf Thanks for looking over that, I've added the check before unsetting.

HHVM isn't failing on the Google URLs so it looks like the placeholders are covering that.

I'll leave the ampersand as is, php, chrome network tools and firefox inspector all show it as part of the path. So it's at least consistent. I've removed these tests.

[peterwilsoncc]

In 38726:

HTTP API: Simplify wp_parse_url() to ensure consistent results.

[38694] revealed some URL formats were been parsed incorrectly, including those used by Google Fonts. This change simplifies the function to use placeholder values which cause PHP's parsing to behave consistently.

Props jrf, peterwilsoncc.
Fixes #36356.

[jrf]

I've added the check before unsetting.

Except you are returning $parts early in that case which will give issues when a $component has been requested (in that case, they would expect null, not false as a return).

Either return using _get_component_from_parsed_url_array( $parts, $component ); or wrap the foreach() in the false check like I suggested.

[peterwilsoncc]

Replying to jrf:

@peterwilsoncc
Except you are returning $parts early in that case which will give issues when a $component has been requested (in that case, they would expect null, not false as a return).

The doco is ambiguous in the circumstance, normally I'd clarify but wanted to get the build passing

I'm happy to look over it, to clarify you thoughts:

• with component: component if exists
• with component: null on parse error
• no component: false on parse error
• no component: array of set values

is this correct?

P

[jrf]

@peterwilsoncc

The doco is ambiguous in the circumstance, normally I'd clarify but wanted to get the build passing

Very true. I've just run some tests and to my surprise on malformed urls + a component parameter parse_url() returns false, so the current behaviour of wp_parse_url() is in line with that.

All the same, it makes for unnecessarily extra complicated variable checking of the return value of wp_parse_url() in the case of a $component parameter being passed.

// Current check would have to be:
$component = wp_parse_url( $url, $component_constant );
if ( isset($component) && false !== $component ) {
	// continue processing
}

// What I suggested would simplify this to:
$component = wp_parse_url( $url, $component_constant );
if ( isset($component) ) {
	// continue processing
}

So to clarify - this was what I suggested:

• with component: component if exists
• with component: null on parse error and if the component does not exist
• no component: false on parse error
• no component: array of set values