Url builder fixes - #23636
Url builder fixes#23636kocsismate wants to merge 7 commits into
Conversation
Reject literal path, query and fragment delimiters before the hostname setter can silently discard the rest of the input.
Populate validation error details when a code is available, and initialize an empty errors array for other builder failures.
Rename the output argument to softErrors and clear it on error-free success. Preserve it on failure while including earlier soft errors in the exception. This makes the behavior consistent with parsing in the constructor.
Reject credentials and ports when a host becomes empty after normalization instead of silently discarding them. Perform validation once, after setting the host.
Keep empty components distinct from null so serialization retains the trailing question mark or hashmark.
Percent-encode only the space immediately preceding a query or fragment delimiter, retaining validation errors for every parsed space.
Preserve absent hosts for non-special URLs and initialize opaque paths directly. Protect path delimiters, report space errors and encode the space immediately before a query or fragment.
| smart_str opaque_path = {0}; | ||
| for (const char *p = Z_STRVAL_P(path); p < path_end; p++) { | ||
| if (*p == '?') { | ||
| smart_str_appends(&opaque_path, "%3F"); |
There was a problem hiding this comment.
I'm not completely sure about this change. The opaque path state doesn't handle the override state separately, and if it encounters a ? or a # then it switches to another state, see
php-src/ext/lexbor/lexbor/url/url.c
Line 2340 in a0a1167
That's why I encoded them, but an alternative option would be to disallow these characters to be used in an opaque path.
@TimWolla Do you have any idea here?
There was a problem hiding this comment.
Do you have any idea here?
Disallowing them would likely be the safe choice: It would allow to lift the restriction without BC considerations in the future. I also expect opaque paths to come up somewhat rarely.
| zval *error; | ||
| ZEND_HASH_FOREACH_VAL(Z_ARRVAL_P(exception_errors), error) { |
There was a problem hiding this comment.
| zval *error; | |
| ZEND_HASH_FOREACH_VAL(Z_ARRVAL_P(exception_errors), error) { | |
| ZEND_HASH_FOREACH_VAL(Z_ARRVAL_P(exception_errors), zval *error) { |
| lxb_url_t *lexbor_url = lexbor_mraw_calloc(lexbor_parser.mraw, sizeof(*lexbor_url)); | ||
| if (lexbor_url == NULL) { | ||
| zend_throw_exception(php_uri_ce_whatwg_invalid_url_exception, "Memory allocation error", 0); | ||
| php_uri_parser_whatwg_throw_exception("Memory allocation error"); |
There was a problem hiding this comment.
This should likely be php_uri_ce_error, because it is not actionable by the user.
| @@ -0,0 +1,46 @@ | |||
| --TEST-- | |||
| Test Uri\WhatWg\UrlBuilder::build() - error - leaves soft errors unchanged | |||
There was a problem hiding this comment.
In my opinion it is more useful to set $softErrors to an empty array by default, when there are no errors, instead of preserving its value. This has some benefits: The user doesn't need to initialize the variable before the call, and the variable's value is always valid/meaningful after the call.
| $builder->setHost("example.com"); | ||
| $builder->setFragment("a\tb"); | ||
| $softErrors = []; | ||
| $builder->build(softErrors: $softErrors); |
There was a problem hiding this comment.
Nit: print the soft errors from the previous build to check the pre-condition of the test
|
|
||
| failure: | ||
| /* Include errors from earlier components in the exception raised by a later component. */ | ||
| if (zend_hash_num_elements(Z_ARRVAL(errors)) > 0 && EG(exception) |
There was a problem hiding this comment.
Nit: is !EG(exception) possible?
3 participants
Several bugs/incompatibilities were found, each of them are now fixed in a separate commit.