Implement "Followup improvements for ext/uri" RFC - RFC 3986 URI building - #22173
Implement "Followup improvements for ext/uri" RFC - RFC 3986 URI building#22173kocsismate merged 10 commits into
Conversation
Sorry, something went wrong.
TimWolla
left a comment
There was a problem hiding this comment.
I'm not yet sure how I feel about the manual validation happening during recomposition. I'd like to take a second look later.
Sorry, something went wrong.
|
I fixed a few suggestions, thank you very much! I'll continue tomorrow with the rest |
Sorry, something went wrong.
There was a problem hiding this comment.
Remarks I had were fixed. One suggestion.
I only looked at the C code, I didn't look or think about the API interface or tests. I leave that part to Tim.
Sorry, something went wrong.
| char buf[MAX_LENGTH_OF_LONG + 1]; | ||
| const char *res = zend_print_long_to_buf(buf + sizeof(buf) - 1, port); | ||
|
|
||
| const bool well_formed = uriIsWellFormedPortA(res, res + strlen(res)); |
There was a problem hiding this comment.
Should also work:
| const bool well_formed = uriIsWellFormedPortA(res, res + strlen(res)); | |
| const bool well_formed = uriIsWellFormedPortA(res, buf + sizeof(buf) - 1 - res); |
Sorry, something went wrong.
There was a problem hiding this comment.
Hm, very clever, but the current approach seems much more straightforward for me
Sorry, something went wrong.
TimWolla
left a comment
There was a problem hiding this comment.
I wonder if all success tests should reparse the resulting URL for extra safety. So basically add:
var_dump($uri->equals(new Uri\Rfc3986\Uri($uri->toRawString())));
at the end of every test (it should always be bool(true)).
I looked at all the tests for now. Did not yet take a full look at the implementation.
Sorry, something went wrong.
| const char *p = Z_STRVAL_P(path); | ||
| while (*p != '\0' && *p != '/') { | ||
| if (*p == ':') { | ||
| zend_throw_exception(php_uri_ce_invalid_uri_exception, "The path must not begin with \":\" when the URI doesn't contain a scheme", 0); |
There was a problem hiding this comment.
| zend_throw_exception(php_uri_ce_invalid_uri_exception, "The path must not begin with \":\" when the URI doesn't contain a scheme", 0); | |
| zend_throw_exception(php_uri_ce_invalid_uri_exception, "The path must not begin with \":\" when the URI does not contain a scheme", 0); |
Using contractions is colloquial (same applies to the other error message.
/cc @Girgias for error message suggestions.
Sorry, something went wrong.
[skip ci]
|
Quick question: the RFC for this PR is still listed under "Pending Implementation / Landing". Is there anything still missing ? Or should this RFC be moved into the "Implemented in PHP 8.6" list ? |
Sorry, something went wrong.
|
@kocsismate Thanks for the reply and the answer. Just thinking: maybe annotating in the RFC what the current implementation status is - this was merged in PHP 8.6, this has not been merged yet - might help clarify for non-Core devs what the status is. Just a suggestion. |
Sorry, something went wrong.
RFC: https://wiki.php.net/rfc/uri_followup#uri_building