-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Add support for Uri\Rfc3986\Uri withers #19636
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
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would still like to see #19636 (comment) changed, but I'm not seeing any bugs anymore.
ext/uri/uri_parser_rfc3986.c
Outdated
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uh, you are right 😅 it's absolutely useless indeed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks
No description provided.