-
Notifications
You must be signed in to change notification settings - Fork 11.8k
[13.x] Use new PHP 8.5 RFC 3986 compliant Uri class #58132
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
37b8c0e to
d726cb5
Compare
|
I think this would be best targetted at Laravel 13. People may also find it unexpected to be forced into RFC 3986, and expect parsing to be more forgiving. |
|
@jnoordsij thanks for the bug report in the puri-olyfill. While I am a proponent advocate of compliance everywhere it is not always the best route. I did take a quick glance at this PR and I believe you need to be aware of some caveats with the PHP native implementations.
This Uri object does not support i18n: So if for whatever reason a host like if you opt to use Since Laravel is a web first framework , this is also a valid option. But again, there are some limitations. While the Url object supports i18n, it always requires absolute URI (aka scheme MUST be present always) this also is not the case for So while I understand the need of using modern PHP I would be cautious during the PR review to take into account these informations. One does not move away from |
|
@nyamsprod thanks for sharing your expertise on the matter. As @GrahamCampbell already rightfully pointed out, the fact that this refactoring does actually go paired with a change in behavior does probably warrant targeting the next major release. I've already been seeing this in tests of the db url parser, which requires some more changes or potentially should be skipped entirely for this reason. So I'll likely follow up by introducing some more incremental changes and some more thoughts/reasoning on wether or not refactoring is warranted on a per-case basis. |
|
Why not simply use |
Including polyfill for prior PHP versions without native support See also https://wiki.php.net/rfc/url_parsing_api
d726cb5 to
3b5aba9
Compare
This, in turn begs the question whether |
|
On second look I'm slightly afraid that this might still be altering behavior in a too prominent manner maybe; e.g. the allowed values for Implementing this all into If anyone else feels like completing this with some more exploring of the pros and cons, feel free to do so. |
Including polyfill for prior PHP versions without native support
See also https://wiki.php.net/rfc/url_parsing_api
Note this should result in generally more safe and consistent code, given the RFC goals. See also https://thephp.foundation/blog/2025/10/10/php-85-uri-extension/.
Implementation notes
masteras it has a small chance of breaking some URL parsing due to now employing a stricter parser. All refactored occurences to me look like they should be complying with such standards, so setting this higher bar seems reasonable.Uri\Whatwg\Urlinstead in some cases; I'm open to feedback on this choice on a per-case basis!Illuminate\Foundation\resources\server.phpuses feature detection rather than polyfilling due to the 'minimal' nature of this file.parse_urlcall in theConfigurationUrlParserclass has been omitted, due to not passing some complex test example after basic refactoring. As this class is designed to parse DB connection urls, which may not necessarily be compliant with stricter specifications, it is questionable whether or not this should be attempted. It is also unclear whether or not the tested example is actually a valid connector url or is also denied when used with real life DB engines. Any undertaken progress on my end can be found at jnoordsij@8c2342b; anyone is free to follow up on this.