Skip to content

ext/uri: Fixes inconsistent validation ordering - #23823

Merged
kocsismate merged 6 commits into
php:PHP-8.6from
NickSdot:policy/uri-builder-validation-error-order
Oct 7, 2026
Merged

kocsismate merged 6 commits into
php:PHP-8.6from
NickSdot:policy/uri-builder-validation-error-order

Conversation

@NickSdot

Copy link
Copy Markdown
Contributor

Aligns builder and parser error order. Parser is LIFO, which is consistent with Lexbor logs (but imo surprising).

Comment thread ext/uri/uri_parser_whatwg.c Outdated
Comment thread ext/uri/uri_parser_whatwg.c Outdated
@NickSdot

Copy link
Copy Markdown
Contributor Author

Does this need INTERNALS or something?

Comment thread ext/uri/uri_parser_whatwg.c Outdated
@TimWolla

TimWolla commented Oct 1, 2026

Copy link
Copy Markdown
Member

Does this need INTERNALS or something?

Not for static functions, since they are not part of the API / ABI.

@kocsismate

Copy link
Copy Markdown
Member

@TimWolla I realized that the fact that the whatwg errors are LIFO is just a side effect of my unfortunate choice to use lexbor_array_obj_pop() back then to retrieve the logs from lexbor. It's possible to implement this retrieval in FIFO order. Something like:

ZEND_ATTRIBUTE_NONNULL static const char *fill_errors_inner(HashTable *errors)
{
	const char *result = NULL;
	lexbor_plog_t *log = lexbor_parser.log;
	size_t length = lexbor_plog_length(log);

	for (size_t i = 0; i < length; i++) {
		const lexbor_plog_entry_t *lxb_error = lexbor_array_obj_get(&log->list, i);

		const char *reason;
		if (append_validation_error(errors, lxb_error->id, (const char *) lxb_error->data, &reason) && result == NULL) {
			result = reason;
		}
	}

	lexbor_plog_clean(log);

	return result;
}

So the main question is if we are allowed to/want to change the order now or later? If yes, then as far as I can see, this PR should rather pivot to fix this detail.

@TimWolla

TimWolla commented Oct 2, 2026

Copy link
Copy Markdown
Member

So the main question is if we are allowed to/want to change the order now or later?

I don't think we ever guaranteed a specific order: Users are expected to check for ->failure to find out which error caused the parsing to abort. In practice I also suspect that this property is not really inspected in a programmatic fashion (versus just stashing it away in some log).

For PHP 8.6, I'd say: Ask the release managers (by requesting review from the team). For master / 8.7, this can just change with a UPGRADING note.

@NickSdot

NickSdot commented Oct 2, 2026

Copy link
Copy Markdown
Contributor Author

As mentioned in the desc, LIFO surprising. Should be FIFO; so keen to pivot. When there is a decision feel free to @ me.

@kocsismate

Copy link
Copy Markdown
Member

@mbeccati @svpernova09 can you please chime in here? #23823 (comment) We would like to reverse the order of errors passed back/thrown by Uri\WhatWg\UrlBuilder, Uri\WhatWg\Url::__construct(), Uri\WhatWg\Url::parse(), as well as Uri\WhatWg\Url::set*(), since currently these errors are returned in backward order.

Can we still do it? As Tim wrote above, we guaranteed no particular order, and I agree with him that likely there is no much use of these errors besides displaying.

@DanielEScherzer
DanielEScherzer requested a review from a team October 3, 2026 17:13
@mbeccati

mbeccati commented Oct 5, 2026

Copy link
Copy Markdown
Contributor

@kocsismate: we've discussed and we think it's ok to merge.

@kocsismate

Copy link
Copy Markdown
Member

@NickSdot Can you please update this PR to fix the order of errors?

@NickSdot
NickSdot force-pushed the policy/uri-builder-validation-error-order branch from a89a3bb to fbfad78 Compare October 7, 2026 05:15
@NickSdot
NickSdot marked this pull request as draft October 7, 2026 05:16
@NickSdot
NickSdot changed the base branch from master to PHP-8.6 October 7, 2026 05:16
@NickSdot
NickSdot force-pushed the policy/uri-builder-validation-error-order branch from fbfad78 to 0937672 Compare October 7, 2026 06:55
@NickSdot
NickSdot marked this pull request as ready for review October 7, 2026 08:07
@NickSdot

NickSdot commented Oct 7, 2026

Copy link
Copy Markdown
Contributor Author

@NickSdot Can you please update this PR to fix the order of errors?

@kocsismate done and targeted 8.6

@kocsismate kocsismate left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall: looks good, but I had a few nits before merge

Comment thread UPGRADING Outdated
Comment thread UPGRADING Outdated
@NickSdot
NickSdot requested a review from kocsismate October 7, 2026 12:14
@kocsismate
kocsismate merged commit c01c9d0 into php:PHP-8.6 Oct 7, 2026
17 of 18 checks passed
kocsismate added a commit that referenced this pull request Oct 7, 2026
* PHP-8.6:
  ext/uri: Fixes inconsistent validation ordering (#23823)
@NickSdot
NickSdot deleted the policy/uri-builder-validation-error-order branch October 7, 2026 12:56
bukka added a commit to bukka/php-src that referenced this pull request Oct 7, 2026
* master: (109 commits)
  ext/uri: Fixes inconsistent validation ordering (php#23823)
  ext/uri: Address UrlBuilder todo comments
  Fix phpGH-24139: NULL dereference in php_ini.c when expand_filepath() fails
  PHP-8.5 is now for PHP 8.5.13-dev
  Fix phpGH-23352: DOMDocument::adoptNode() stale document references
  Fix merge
  ext/soap: fix use of uninitialized func in do_request() on OOM bailout
  Fix merge
  ext/curl: Use try conversion functions in curl (php#24067)
  ext/standard: Retry getpwnam_r() on ERANGE in php_get_uid_by_name()
  Fix phpGH-20890: Segfault in zval_undefined_cv with non-simple property hook with minimal tracing JIT
  PHP 8.4 is now for PHP 8.4.28-dev
  ext/phar: Fix double-free in webPhar() without PATH_INFO (php#24166)
  [ci skip] Update NEWS for 8.6.0RC4
  std: refactor php_array_find() to only use FCC
  std: add trampoline tests for array_find based user functions
  std: move some array find tests to a dedicated folder
  Fix alternate form flag for %x testing stale signed value
  Fix bzopen() ownership of the stream it wraps
  Preserve bare NUL access with open_basedir on Windows (php#24158)
  ...

# Conflicts:
#	ext/openssl/tests/stream_poll_handle_cast.phpt
#	ext/openssl/xp_ssl.c
#	main/php_streams.h
#	main/streams/plain_wrapper.c
#	main/streams/userspace.c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants