ext/uri: Reorganises legacy tests by NickSdot · Pull Request #23286 · php/php-src

NickSdot · 2026-08-14T21:06:16Z

Ref: #23271 (review) @kocsismate
Depends on: #23271 (~~ready here, but marked as draft until 23271 is merged~~)

Coverage

Coverage: ext/uri

Base: a2dc4e9ff260cd47f8b8b7abe7b555f2b9ff3b2f upstream/master
Tree: d0315577c31d6aaab5b0ea764856da4417b13eb4 working tree

+--------+-------+---------+--------------------+-------------------+--------+---------+
|        | Tests | Sources |              Lines |          Branches |   Time |  Memory |
+--------+-------+---------+--------------------+-------------------+--------+---------+
| Base   |   433 |       7 | 1962/2202 (89.10%) | 714/1199 (59.55%) |  8.43s | 28.0 MB |
| Tree   |   451 |       7 | 1962/2202 (89.10%) | 714/1199 (59.55%) |  9.55s | 28.0 MB |
| Change |   +18 |       0 |   +0 / -0 (+0.00%) |  +0 / -0 (+0.00%) | +1.12s | +0.0 MB |
+--------+-------+---------+--------------------+-------------------+--------+---------+

NickSdot · 2026-08-15T08:39:05Z

Two options for these: keep grouped in eg parsing with zend_ prefix or give dedicated zend dir. Thoughts?

I'm fine with putting it into the general directory with something like internal_api_success_parse_url_parsing.php or similar. The important thing here is that the internal API is used by the zend_test function.

kocsismate

partial review yet, I'll continue later

kocsismate · 2026-08-17T20:56:52Z


-$url = new Uri\WhatWg\Url("https://example.com/foo/bar%2Fbaz");
-var_dump($url->getPath());
+var_dump($uri->toRawString());


tests in the parsing directory have the following code for assertions:

var_dump($uri); var_dump($uri->toRawString()); var_dump($uri->toString()); ```

kocsismate · 2026-08-17T20:58:01Z

+
+$uri = Uri\Rfc3986\Uri::parse("192.168/contact.html");
+
+var_dump($uri);


the same assertions are needed as mentioned above

done

I actually think something like assertion_helpers.inc would make sense here.

kocsismate · 2026-08-17T21:01:40Z

much of these cases are already covered, please check them (e.g. there is ext/uri/tests/rfc3986/parsing/path_success_relative_reference.phpt and ext/uri/tests/rfc3986/parsing/path_success_relative_reference_absolute.phpt). so probably only mailto:johndoe@example.com and maybe /// is needed (but let's separate them for better readability)

done for both

NickSdot · 2026-08-19T06:18:20Z

partial review yet, I'll continue later

To avoid miscommunication: I plan to address everything in one go when you continued. In case your second pass didn't yield anything new please update me so I know it's done and we do not wait for each other. :)

kocsismate · 2026-08-19T20:53:31Z

@@ -1,5 +1,5 @@
 --TEST--
-Test reference resolution
+Test Uri\Rfc3986\Uri reference resolution during parsing


the usual format should be used

not sure what you mean

kocsismate · 2026-08-19T20:54:21Z

this is not needed, since it doesn't have a baseUrl

kocsismate · 2026-08-19T20:55:11Z

the rest of the cases should go into separate tests, and the assertion should be the usual one

var_dump($uri); var_dump($uri->toRawString()); var_dump($uri->toString());

Done. Split to:

parse_success_absolute_with_base.phpt parse_success_relative_with_base.phpt parse_success_without_base.phpt

Normalise case was removed in accordance with your comment below.

kocsismate · 2026-08-19T20:57:08Z

I'm not really sure a separate case for normalized URI is needed here, unless it improves coverage somehow 🤔 (but I don't think it does)

Removed. It did not. As mentioned above, these were just 1:1: moved. Goal here was to retain coverage, nothing was added extra.

kocsismate · 2026-08-19T21:11:12Z

+
+$uri = new Uri\Rfc3986\Uri('https://example.com');
+try {
+  $uri->__unserialize([['uri' => 'ftp://example.org'], []]);


this can be a serialization test indeed

It already is an serialisation test?

kocsismate · 2026-08-19T21:18:56Z

To avoid miscommunication: I plan to address everything in one go when you continued. In case your second pass didn't yield anything new please update me so I know it's done and we do not wait for each other. :)

I went through all rfc3986 tests now. There are quite a few recurring problems which even apply to the WHATWG URL tests, so please resolve my comments globally, and then I'll continue with another, full review round.

NickSdot · 2026-08-20T08:17:58Z

To avoid miscommunication: I plan to address everything in one go when you continued. In case your second pass didn't yield anything new please update me so I know it's done and we do not wait for each other. :)

I went through all rfc3986 tests now. There are quite a few recurring problems which even apply to the WHATWG URL tests, so please resolve my comments globally, and then I'll continue with another, full review round.

Except a few with questions that should be done in 43b5f64.
Rebased
Updated the coverage table in the desc. Numbers now slightly different because of Implement "Followup improvements for ext/uri" RFC - WHATWG URL building #22268 -- coverage same.
Unlike others the PR used PHP_EOL for error assertions. Since those are fresh tests I allowed myself to add 80346c6 to unify them (can cherry pick to a separate PR if you prefer). There are some older ones that I didn't want to touch unasked; can do them too if you want -- my OCD would appreciate it.

Aside: before your review I treated the task as mechanical moves and extractions. So the pre-review state was 1:1 to before this PR ; I didn't check for duplications. I get now what you want. Thing is, I am not exactly confident to decide what qualifies as identical, what differs enough to keep it asserted, and what justifies a removal. I also cannot check each move against all 448 tests, my brain will explode. 😅 I did check the cases you mentioned (+whatwg) -- but if the goal really is to have zero duplicates, I cannot claim that's achieved now.

kocsismate

Sorry, it's a very long list of review comments, only a few files are left, but I have to take a break now. Feel free to address my comments already.

kocsismate · 2026-08-30T19:33:53Z

+$uri1 = Uri\Rfc3986\Uri::parse("https://example.com");
+$uri2 = $uri1->withHost("t%3As%2Ft.com");
+
+var_dump($uri1->getRawHost());


The existing tests only assert the modified component of the original URI like this:

var_dump($uri1->getRawHost()); var_dump($uri2->getRawHost()); var_dump($uri2->toRawString()); var_dump($uri2->getHost()); var_dump($uri2->toString());

kocsismate · 2026-08-30T19:38:15Z

+var_dump($uri1->toRawString());
+
+var_dump($uri2->getPort());
+var_dump($uri2->toRawString());


The estabilished format:

Suggested change

var_dump($uri1->toRawString());

var_dump($uri2->getPort());

var_dump($uri2->toRawString());

var_dump($uri1->getPort());

var_dump($uri2->getPort());

var_dump($uri2->toRawString());

kocsismate · 2026-08-30T19:42:34Z

+var_dump($uri1->getPort());
+var_dump($uri1->toString());
+
+var_dump($uri2->getPort());
+var_dump($uri2->toString());


Suggested change

var_dump($uri1->getPort());

var_dump($uri1->toString());

var_dump($uri2->getPort());

var_dump($uri2->toString());

var_dump($uri1->getPort());

var_dump($uri2->getPort());

var_dump($uri2->toRawString());

kocsismate · 2026-08-31T21:32:06Z

maybe it is rather a general test?

kocsismate · 2026-08-31T21:32:38Z

+$url = Uri\WhatWg\Url::parse("file:///E:\Documents and Settings");
+
+var_dump($url);
+var_dump($url->toUnicodeString());


Suggested change

var_dump($url->toUnicodeString());

kocsismate · 2026-08-31T21:34:01Z

+$url = new Uri\WhatWg\Url("https://example.com/foo/bar%2Fbaz");
+
+var_dump($url);
+var_dump($url->toUnicodeString());


Suggested change

var_dump($url->toUnicodeString());

kocsismate · 2026-08-31T21:34:24Z

+var_dump($url);
+var_dump($url->toUnicodeString());
+var_dump($url->toAsciiString());
+var_dump($url->getPath());


Suggested change

var_dump($url->getPath());

kocsismate · 2026-08-31T21:42:48Z

Aside: before your review I treated the task as mechanical moves and extractions. So the pre-review state was 1:1 to before this PR ; I didn't check for duplications. I get now what you want. Thing is, I am not exactly confident to decide what qualifies as identical, what differs enough to keep it asserted, and what justifies a removal. I also cannot check each move against all 448 tests, my brain will explode. 😅 I did check the cases you mentioned (+whatwg) -- but if the goal really is to have zero duplicates, I cannot claim that's achieved now.

Sorry if I got you into more than what you subscribed for. :) If you are still patient then we are not that far away from the finish line. I wanted to avoid keeping bad/unnecessary tests from the legacy folder, that's why I did such a "strict" review. I hope that you understand it :)

NickSdot · 2026-09-01T05:08:42Z

Sorry if I got you into more than what you subscribed for. :) If you are still patient then we are not that far away from the finish line. I wanted to avoid keeping bad/unnecessary tests from the legacy folder, that's why I did such a "strict" review. I hope that you understand it :)

Nah,that's all good. Really just wanted to let you know that I will have to rely on your deeper knowledge of the domain. Will pick up on the comments soon! :)

Co-authored-by: Máté Kocsis <kocsismate90@gmail.com>

github-actions Bot added the Extension: uri label Aug 14, 2026

NickSdot mentioned this pull request Aug 14, 2026

ext/uri: applied fixers to improve test robustness #23271

Merged

NickSdot commented Aug 15, 2026

View reviewed changes

Comment thread ext/uri/tests/rfc3986/parsing/port_error_overflow.phpt

NickSdot commented Aug 15, 2026

View reviewed changes

Comment thread ext/uri/tests/whatwg/parsing/errors_reference_error.phpt

NickSdot commented Aug 15, 2026

View reviewed changes

NickSdot force-pushed the hardening/ext-uri-reorg branch from 74844f2 to d2bfb5c Compare August 15, 2026 14:04

NickSdot marked this pull request as ready for review August 15, 2026 14:10

NickSdot requested review from TimWolla and kocsismate as code owners August 15, 2026 14:10

NickSdot mentioned this pull request Aug 16, 2026

ext/uri: Fixes fragment handler registration #23303

Merged

kocsismate reviewed Aug 17, 2026

View reviewed changes

NickSdot mentioned this pull request Aug 18, 2026

ext/uri: Increases test coverage #23342

Draft

kocsismate reviewed Aug 19, 2026

View reviewed changes

NickSdot added 6 commits August 20, 2026 11:23

ext/uri: removed superseded legacy tests

38c016f

ext/uri: reorganised partially superseded legacy tests

69953be

ext/uri: reorganised remaining legacy tests

af70620

ext/uri: reorganised remaining legacy tests

0df6c2b

ext/uri: restored legacy parsing error cases

4111d0e

review: improved tests

43b5f64

NickSdot force-pushed the hardening/ext-uri-reorg branch from d2bfb5c to 43b5f64 Compare August 20, 2026 07:42

refactor: changed error assertions to use \n

80346c6

NickSdot requested a review from kocsismate August 20, 2026 08:19

fix: added empty last line

d031557

kocsismate reviewed Aug 31, 2026

View reviewed changes

review: wording

9845a95

Co-authored-by: Máté Kocsis <kocsismate90@gmail.com>

NickSdot and others added 4 commits September 1, 2026 12:14

review: wording

6b1ece4

Co-authored-by: Máté Kocsis <kocsismate90@gmail.com>

review: wording

5d0c919

Co-authored-by: Máté Kocsis <kocsismate90@gmail.com>

review: wording

610014b

Co-authored-by: Máté Kocsis <kocsismate90@gmail.com>

review: wording

c2f6384

Co-authored-by: Máté Kocsis <kocsismate90@gmail.com>


		$uri = Uri\Rfc3986\Uri::parse("192.168/contact.html");

		var_dump($uri);

Uh oh!

Conversation

NickSdot commented Aug 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverage

Uh oh!

Uh oh!

NickSdot Aug 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kocsismate left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uh oh!

NickSdot commented Aug 19, 2026

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kocsismate commented Aug 19, 2026

NickSdot commented Aug 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

kocsismate left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kocsismate commented Aug 31, 2026

NickSdot commented Sep 1, 2026

Labels

2 participants

NickSdot commented Aug 14, 2026 •

edited

Loading

NickSdot Aug 15, 2026 •

edited

Loading

NickSdot commented Aug 20, 2026 •

edited

Loading