WordPress.org

Make WordPress Core

Opened 4 months ago

Last modified 4 months ago

#53938 new enhancement

replace core uses of wp_parse_url() with PHP's native parse_url()

Reported by: pbiron Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version:
Component: HTTP API Keywords: needs-patch 2nd-opinion close
Focuses: Cc:

Description (last modified by pbiron)

wp_parse_url() was introduced in #34408 to get around URL parsing failures of PHP's parse_url() in PHP < 5.4.7.

With the minimum supported PHP for core now 5.6.20, wp_parse_url() no longer seems necessary and parse_url() can be used directly. For background on this ticket, see the slack thread.

Change History (11)

#1 @pbiron
4 months ago

@SergeyBiryukov already thought this would be a good idea (see the slack thread mentioned in the ticket description). But before putting a patch together, would like at least one other committer to chime in on whether this would be a good idea.

#2 @SergeyBiryukov
4 months ago

  • Milestone changed from Awaiting Review to 5.9

This ticket was mentioned in Slack in #core by pbiron. View the logs.


4 months ago

#4 @pbiron
4 months ago

  • Description modified (diff)
  • Milestone changed from 5.9 to Awaiting Review
  • Summary changed from replace core uses of wp_parse_url() to replace core uses of wp_parse_url() with PHP's native parse_url()

#6 follow-up: @jrf
4 months ago

  • Keywords close added

The conclusion from the discussion in the previous ticket #48242, as referenced by @johnbillion, still stands:

  1. There is still a PHP cross-version incompatibility (to do with handling of colons vs ports) which is handled by the wp_parse_url() function, which is not handled correctly/consistently in PHP when taking all PHP versions currently supported by WP into account.
  2. And as @dd32 quite rightly states in the previous ticket: the PHP native function has a history of lagging behind in support for new URL formats (like schemeless etc) and fixes are not backported to older PHP versions, so it is likely that there will be new PHP cross-version differences to work around in the future.

All in all, keeping the function in place and recommending for people to use the WP version rather than the PHP version still seems the prudent way forward.

It might be a better idea to do a careful review of the current uses of the PHP native parse_url() function in WP Core (and there are quite a few) to see if those should be switched over/back to using wp_parse_url() instead.

#7 in reply to: ↑ 6 @pbiron
4 months ago

Replying to jrf:

It might be a better idea to do a careful review of the current uses of the PHP native parse_url() function in WP Core (and there are quite a few) to see if those should be switched over/back to using wp_parse_url() instead.

Good thing I didn't rush to put a patch together to replace current uses of wp_parse_url() with parse_url() :-)

Sergey counted 113 uses of parse_url() vs 39 of wp_parse_url().

So, sounds like the 1st course of action is to check whether there are unit tests that cover all current core uses of parse_url() and add unit tests for cases that don't have them. Then, we can decide which uses could/should be replaced with wp_parse_url() without introducing backwards compatibility problems.

I'm really busy right now, but if no one beats me to, I'll start inventorying the unit tests for the current uses of parse_url().

#8 follow-up: @jrf
4 months ago

@pbiron While I would very much welcome more tests, in this case, the tests are only relevant for those calls which may run into the : issue. So I suspect that would be the calls to parse_url() without the $component parameter or when the $component would be any of these: PHP_URL_QUERY, PHP_URL_PORT, but I haven't looked into the colon issue in detail today to bring back the fine details, so the above suggestion should be verified carefully against the PHP core ticket: https://bugs.php.net/bug.php?id=74780

#9 in reply to: ↑ 8 ; follow-up: @SergeyBiryukov
4 months ago

Replying to jrf:

the above suggestion should be verified carefully against the PHP core ticket: https://bugs.php.net/bug.php?id=74780

Just curious: when looking at a PHP core ticket like that, is there a way to tell in which version it was fixed? I don't see a milestone or a similar field neither there nor on GitHub.

#10 in reply to: ↑ 9 ; follow-up: @jrf
4 months ago

Just curious: when looking at a PHP core ticket like that, is there a way to tell in which version it was fixed? I don't see a milestone or a similar field neither there nor on GitHub.

At the bottom of the ticket you can see the git reference of the commit which fixed the issue. In this case: http://git.php.net/?p=php-src.git;a=commit;h=db287b23030a2ac16bd9e2524d7e8bbe1b4a4d9a
If you then click on "commitdiff" you can see that the changelog entry was added for PHP 7.0.22.

You can also take that commit reference and use it in the repo on GitHub: https://github.com/php/php-src/commit/db287b23030a2ac16bd9e2524d7e8bbe1b4a4d9a to see the same commit there (they've now switched completely to GH, but used their own Git server previously).
There you can also see the branch information php-8.1.0beta2 ... php-7.0.22RC1 and if you click on the ... if will fold out and show you more precise details, like that it went into PHP 7.0.22 and 7.1.8 and is included in all versions since 7.2.0.

Does that help ?

#11 in reply to: ↑ 10 @SergeyBiryukov
4 months ago

Replying to jrf:

Does that help ?

Definitely, that's a great explanation :) Thank you!

Note: See TracTickets for help on using tickets.