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/tests

Base: 2d7131e1bf1da03eb133c50aee37c8377ecc1723 upstream/master
Tree: 2148cd5c088f1ddddc716ebfb079fd9af0334e36 working tree

+--------+-----------+---------+--------------------+-------------------+--------+---------+
|        |     Tests | Sources |              Lines |          Branches |   Time |  Memory |
+--------+-----------+---------+--------------------+-------------------+--------+---------+
| Base   |       484 |       7 | 2035/2281 (89.22%) | 733/1234 (59.40%) |  9.96s | 28.0 MB |
| Tree   |       519 |       7 | 2035/2281 (89.22%) | 733/1234 (59.40%) | 10.39s | 27.8 MB |
| Change | +73 / -38 |       0 |   +0 / -0 (+0.00%) |  +0 / -0 (+0.00%) | +0.43s | -0.2 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.

Moved them to ext/uri/tests/zend and kept your proposed prefix. Only having the suffix does not make it clear what it is; having a zend directory convention in extensions helps contributors that are new to the code base. Ok with it?

kocsismate

partial review yet, I'll continue later

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>

NickSdot · 2026-09-01T14:33:38Z

 } catch (Throwable $e) {
-    echo $e::class, ": ", $e->getMessage(), PHP_EOL;
+    echo $e::class, ': ', $e->getMessage(), "\n";
+    var_dump($e->errors[0]->type === Uri\WhatWg\UrlValidationErrorType::PortOutOfRange);


Added this because the other test has it too.

NickSdot · 2026-09-01T14:36:36Z

+<?php
+
+$errors = [];
+$url = Uri\WhatWg\Url::parse("🐘", null, $errors);


Not exactly the same as the one with emoji in host.

NickSdot · 2026-09-01T15:10:38Z

Should be done @kocsismate.

each test should now does one thing
applied each feedback to rfc and whatwg
had Codex run over it twice to look for duplicates

Notes:

The last two commits are for unrelated tests. While we are on it, we can unify all titles and style. They added some more files. If you prefer them cherry-picked to a new PR please let me know.
whatwg/builder/host_error_null_special.phpt and whatwg/builder/basic_error_with_base.phpt do not actually test what they say; probably related to Implement "Followup improvements for ext/uri" RFC - URL building with base URL #23526, and you might want to address it there.

Failure unrelated.

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_error_typed_property_reference.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 force-pushed the hardening/ext-uri-reorg branch from d2bfb5c to 43b5f64 Compare August 20, 2026 07:42

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

kocsismate reviewed Aug 31, 2026

View reviewed changes

NickSdot and others added 9 commits September 1, 2026 21:48

ext/uri: removed superseded legacy tests

128157a

ext/uri: reorganised partially superseded legacy tests

435fb0c

ext/uri: reorganised remaining legacy tests

5af9f17

ext/uri: reorganised remaining legacy tests

f373bcd

ext/uri: restored legacy parsing error cases

cd1712b

review: improved tests

d227278

refactor: changed error assertions to use \n

fbcca76

fix: added empty last line

a1fcf51

review: nits

24f46da

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

NickSdot added 20 commits September 1, 2026 21:48

review: moved zend tests

ca8c97c

review: standardised assertions

599053a

review: simplified host parsing

9e21966

review: separated multibyte cases

ef182a8

review: reorganised path and normalisation tests

2417f8f

review: separated resolve cases

27287ea

review: restored bug tests

87a7a2f

review: standardised clone

c582755

review: simplified re-inits

ac06cfb

review: standardised encoded comments

b7a7b9c

review: naming

7c431a5

review: removed duplicates

f13ba52

review: standardised constructor parsing

d1451e0

review: moved memory to general

129a395

review: standardised warnings

96dff28

review: unified titles

d8e2371

review: dropped unicode assertions

c2e7896

review: asserted error type

7b3d58f

review: unified titles (unrelated tests)

ecc6fe9

review: formatting (unrelated tests)

72c11be

NickSdot force-pushed the hardening/ext-uri-reorg branch from c2f6384 to 72c11be Compare September 1, 2026 13:59

NickSdot commented Sep 1, 2026

View reviewed changes

review: updated wrong title

2148cd5

NickSdot requested a review from kocsismate September 1, 2026 15:10

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!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kocsismate 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

NickSdot commented Aug 19, 2026

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!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kocsismate commented Aug 19, 2026

Uh oh!

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

Uh oh!

Uh oh!

kocsismate left a comment

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!

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!

Choose a reason for hiding this comment

NickSdot commented Aug 14, 2026 •

edited

Loading

NickSdot Aug 15, 2026 •

edited

Loading

NickSdot commented Aug 20, 2026 •

edited

Loading