Implement "Followup improvements for ext/uri" RFC - RFC 3986 URI building by kocsismate · Pull Request #22173 · php/php-src

kocsismate · 2026-05-28T18:21:37Z

RFC: https://wiki.php.net/rfc/uri_followup#uri_building

TimWolla

I'm not yet sure how I feel about the manual validation happening during recomposition. I'd like to take a second look later.

kocsismate · 2026-05-31T21:20:49Z

+	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(


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

UriBool success = URI_FUNC(FixPathNoScheme)(destUri, memory);

): https://github.com/php/php-src/blob/27bc7c0e1294eb0ce87f071acd42f43f01877d61/ext/uri/tests/rfc3986/modification/roundtrip_special_case2.phpt#L18-L17

kocsismate · 2026-05-31T21:58:03Z

I fixed a few suggestions, thank you very much! I'll continue tomorrow with the rest

kocsismate · 2026-06-07T09:25:52Z

+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);


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?

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.

ndossche

Remarks I had were fixed. One suggestion.
I only looked at the C code, I didn't look or think about the API interface or tests. I leave that part to Tim.

ndossche · 2026-06-14T10:42:18Z

+	char buf[MAX_LENGTH_OF_LONG + 1];
+	const char *res = zend_print_long_to_buf(buf + sizeof(buf) - 1, port);
+
+	const bool well_formed = uriIsWellFormedPortA(res, res + strlen(res));


Should also work:

Suggested change

const bool well_formed = uriIsWellFormedPortA(res, res + strlen(res));

const bool well_formed = uriIsWellFormedPortA(res, buf + sizeof(buf) - 1 - res);

Hm, very clever, but the current approach seems much more straightforward for me

…ding RFC: https://wiki.php.net/rfc/uri_followup#uri_building

TimWolla

I wonder if all success tests should reparse the resulting URL for extra safety. So basically add:

var_dump($uri->equals(new Uri\Rfc3986\Uri($uri->toRawString())));

at the end of every test (it should always be bool(true)).

I looked at all the tests for now. Did not yet take a full look at the implementation.

TimWolla · 2026-06-22T17:56:05Z

Given that the test description explicitly mentions “falls back to a registered name”, should the test also assert on getHostType()?

TimWolla · 2026-06-22T18:00:16Z

+<?php
+
+$builder = new Uri\Rfc3986\UriBuilder();
+$builder->setUserInfo(null);


Suggested change

$builder->setUserInfo(null);

$builder->setUserInfo('something');

$builder->setUserInfo(null);

I think all the null tests should first set the value to something else, otherwise we are just testing that we're successfully not doing anything - since null already effectively is the default value.

TimWolla · 2026-06-22T18:03:10Z

+  NULL
+  ["fragment"]=>
+  NULL
+}


missing newline at eof

TimWolla · 2026-06-22T18:04:38Z

+			const char *p = Z_STRVAL_P(path);
+			while (*p != '\0' && *p != '/') {
+				if (*p == ':') {
+					zend_throw_exception(php_uri_ce_invalid_uri_exception, "The path must not begin with \":\" when the URI doesn't contain a scheme", 0);


Suggested change

zend_throw_exception(php_uri_ce_invalid_uri_exception, "The path must not begin with \":\" when the URI doesn't contain a scheme", 0);

zend_throw_exception(php_uri_ce_invalid_uri_exception, "The path must not begin with \":\" when the URI does not contain a scheme", 0);

Using contractions is colloquial (same applies to the other error message.

/cc @Girgias for error message suggestions.

kocsismate requested a review from ndossche May 28, 2026 18:21

kocsismate requested a review from TimWolla as a code owner May 28, 2026 18:21

kocsismate added the RFC label May 28, 2026

github-actions Bot added ABI break Extension: uri labels May 28, 2026

TimWolla reviewed May 28, 2026

View reviewed changes

Comment thread ext/uri/php_uri.c Outdated

Comment thread ext/uri/php_uri.c Outdated

Comment thread ext/uri/uri_parser_rfc3986.c Outdated

Comment thread ext/uri/uri_parser_rfc3986.c Outdated

Comment thread ext/uri/uri_parser_rfc3986.c Outdated

kocsismate force-pushed the uri-followup3 branch from 79d8d24 to ae4c905 Compare May 28, 2026 21:00

kocsismate commented May 31, 2026

View reviewed changes

ndossche requested changes May 31, 2026

View reviewed changes

Comment thread ext/uri/php_uri.c Outdated

Comment thread ext/uri/php_uri.c

Comment thread ext/uri/php_uri.c Outdated

ndossche requested changes May 31, 2026

View reviewed changes

Comment thread ext/uri/uri_parser_whatwg.c Outdated

kocsismate force-pushed the uri-followup3 branch from 18f1287 to 67fa4c9 Compare May 31, 2026 21:57

kocsismate force-pushed the uri-followup3 branch from 67fa4c9 to c5f6817 Compare June 1, 2026 20:25

TimWolla self-requested a review June 1, 2026 20:34

ndossche requested changes Jun 1, 2026

View reviewed changes

Comment thread ext/uri/php_uri.c

kocsismate commented Jun 7, 2026

View reviewed changes

kocsismate force-pushed the uri-followup3 branch from a0954b9 to 4ccd652 Compare June 7, 2026 09:33

ndossche approved these changes Jun 8, 2026

View reviewed changes

Comment thread ext/uri/uri_parser_rfc3986.c Outdated

kocsismate mentioned this pull request Jun 10, 2026

Mark the value param with const modifier in the ext/uri write handler #22274

Merged

kocsismate force-pushed the uri-followup3 branch from 4ccd652 to 3048323 Compare June 13, 2026 21:27

ndossche approved these changes Jun 14, 2026

View reviewed changes

kocsismate added 8 commits June 14, 2026 14:15

Implement "Followup improvements for ext/uri" RFC - RFC 3986 URI buil…

dcd1b92

…ding RFC: https://wiki.php.net/rfc/uri_followup#uri_building

Review fixes + optimize property reads + fix leak

cc9c15a

Refactor

f45c5db

Some code review fixes

1dabc44

Fix property handling

0eea3ac

Refactor component validation during URI building

1f3f561

Microoptimize port validation

7929820

Fix unintended static removal

0bf64ff

kocsismate force-pushed the uri-followup3 branch from a3ec042 to 0bf64ff Compare June 14, 2026 12:18

TimWolla reviewed Jun 22, 2026

View reviewed changes

	const bool well_formed = uriIsWellFormedPortA(res, res + strlen(res));
	const bool well_formed = uriIsWellFormedPortA(res, buf + sizeof(buf) - 1 - res);

	$builder->setUserInfo(null);
	$builder->setUserInfo('something');
	$builder->setUserInfo(null);

	zend_throw_exception(php_uri_ce_invalid_uri_exception, "The path must not begin with \":\" when the URI doesn't contain a scheme", 0);
	zend_throw_exception(php_uri_ce_invalid_uri_exception, "The path must not begin with \":\" when the URI does not contain a scheme", 0);

Conversation

kocsismate commented May 28, 2026

Uh oh!

TimWolla left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kocsismate commented May 31, 2026

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ndossche left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TimWolla left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ndossche left a comment •

edited

Loading