Implement "Followup improvements for ext/uri" RFC - RFC 3986 URI building#22173
Implement "Followup improvements for ext/uri" RFC - RFC 3986 URI building#22173kocsismate wants to merge 6 commits into
Conversation
TimWolla
left a comment
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.
| @@ -1,5 +1,5 @@ | |||
| --TEST-- | |||
| Test Uri\Rfc3986\Uri component modification - host - too small number | |||
| Test Uri\Rfc3986\Uri component modification - port - too small number | |||
There was a problem hiding this comment.
I'll fix this separately
| typedef zend_result (*php_uri_property_handler_read)(void *uri, php_uri_component_read_mode read_mode, zval *retval); | ||
|
|
||
| typedef zend_result (*php_uri_property_handler_write)(void *uri, zval *value, zval *errors); | ||
| typedef zend_result (*php_uri_property_handler_write)(void *uri, const zval *value, zval *errors); |
There was a problem hiding this comment.
I realized that it was possible to make value a const. I can implement this in a separate PR if you think it's a good idea.
| return uriIsWellFormedFragmentA(p, p + len); | ||
| } | ||
|
|
||
| ZEND_ATTRIBUTE_NONNULL_ARGS(2,3,4,5,6,7,8) php_uri_parser_rfc3986_uris *php_uri_parser_rfc3986_build_from_zval( |
There was a problem hiding this comment.
how about this method? I think it's more efficient than the previous implementation, because the recomposition part was eliminated. It was also possible to get rid of two custom validation of the "global state" below. The remaining two are needed because uriparser doesn't fail in these cases (
php-src/ext/uri/uriparser/src/UriSetPath.c
Line 279 in 8fd69e1
|
I fixed a few suggestions, thank you very much! I'll continue tomorrow with the rest |
| static zend_result php_uri_parser_rfc3986_add_base_url( | ||
| UriUriA *tmp, const UriUriA *uri, const php_uri_parser_rfc3986_uris *uriparser_base_urls, const bool silent | ||
| ) { | ||
| const int result = uriAddBaseUriExMmA(tmp, uri, &uriparser_base_urls->uri, URI_RESOLVE_STRICTLY, mm); |
There was a problem hiding this comment.
CLion started to nag me about adding the const modifier for scalar values like this. As far as I know, the compilers usually find out themselves without the modifier if a value is modified or not, so in practice, its only benefit is maybe some extra explicitness. WDYT?
There was a problem hiding this comment.
The const modifier only helps you, the programmer, marking local as const won't influence the quality of the resulting machine code.
I don't think it's terribly important to mark them as const. If it's not a burden for you or goes automatically feel free to add it of course.
4 participants
RFC: https://wiki.php.net/rfc/uri_followup#uri_building