Add support for Uri\Rfc3986\Uri withers#19636
Conversation
|
It's probably best to commit the uriparser changes separately and base the PHP changes off on top of that. |
Yes, sure! This PR in its current form is more like intended to test the setters. I'll create a separate one once the new uriparser release is done. |
TimWolla
left a comment
TimWolla
left a comment
There was a problem hiding this comment.
I would still like to see #19636 (comment) changed, but I'm not seeing any bugs anymore.
| if (result != URI_SUCCESS) { | ||
| zend_throw_exception(uri_invalid_uri_exception_ce, "The specified path is malformed", 0); | ||
| return FAILURE; | ||
| } | ||
|
|
||
| reset_normalized_uri_after_writing(internal_uri); | ||
|
|
||
| return SUCCESS; |
There was a problem hiding this comment.
This could theoretically be adjusted to a switch() for consistency with the port, host, and userinfo setters (applies to all setters).
|
Already requesting RM review, since I don't expect any more significant changes. |
DanielEScherzer
left a comment
DanielEScherzer
left a comment
There was a problem hiding this comment.
RM review - looks good, approved
Also did a minor technical review, some unused variables and parameters that could be removed
ext/uri/tests/026.phpt
Outdated
| } | ||
|
|
||
| try { | ||
| $uri3->withHost("t:s/t.com"); // t:s/t.com |
There was a problem hiding this comment.
Contrary to the use of the comment above, this comment seems kinda useless: it throws me off because now I'm staring at what the difference is between the string and the comment but I don't see it ;)
There was a problem hiding this comment.
Uh, you are right 😅 it's absolutely useless indeed
4 participants
No description provided.