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

kocsismate

partial review yet, I'll continue later

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-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-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.

kocsismate

I went through all tests and they look fine now with only the following exceptions:

kocsismate · 2026-09-02T18:39:27Z

@@ -1,5 +1,5 @@
 --TEST--
-Test Uri\Rfc3986\Uri getter - uri type - Absolute path reference
+Test Uri\Rfc3986\Uri component retrieval - URI type - absolute-path reference


tbh these tests didn't use "component retrieval", because URI type is not a URI component, so the original wording was better.

Done. Included in next push.

kocsismate · 2026-09-02T18:46:06Z

@@ -1,5 +1,5 @@
 --TEST--
-Test Uri\Rfc3986\Uri component modification - fragment - empty string
+Test Uri\Rfc3986\Uri component modification - fragment - success - empty string


The component modification test names were indeed inconsistent with parsing test names, but maybe using the method name in the first group is better than having 4 groups.

However, I'd prefer not to add more things into this PR because reviewing it already took me 4+ hours, so I would love to finish it soon.

Will make it a follow up.

Follow up here: #23551

kocsismate · 2026-09-02T18:47:43Z

@@ -0,0 +1,22 @@
+--TEST--
+Test Uri\Rfc3986\Uri component modification - host - success - percent-encoded reserved characters


Suggested change

Test Uri\Rfc3986\Uri component modification - host - success - percent-encoded reserved characters

Test Uri\Rfc3986\Uri component modification - host - percent-encoded characters

Only this one or all that have success? Why elsewhere adding success and here not?

I will leave this alone for now and make it part of the follow up. When we remove 'success' it's inconsistent. When we remove 'reserved' it's the same as in host_success_encoded.phpt which is confusing. With the follow up success/error makes more sense again. Could be from:

Test Uri\Rfc3986\Uri component modification - host - success - percent-encoded reserved characters Test Uri\Rfc3986\Uri component modification - host - error - reserved characters

to

Test Uri\Rfc3986\Uri::withHost() - success - percent-encoded reserved characters Test Uri\Rfc3986\Uri::withHost() - error - reserved characters

So I am not going the success remove, nor the reserved here if you are fine with it.

kocsismate · 2026-09-02T19:00:46Z

@@ -0,0 +1,10 @@
+--TEST--
+Test Uri\Rfc3986\Uri parsing - query - emoji code point


FYI: RFC 3986 uses the "character" naming, while WHATWG uses "code point".

Changed in two tests. Included in next push.

kocsismate · 2026-09-02T19:09:18Z

@@ -0,0 +1,25 @@
+--TEST--
+Test Uri\WhatWg\Url parsing - basic - parse() returns null for an emoji relative reference


WHATWG uses relative-URL instead of relative reference

Done in 6 files. Included in next push.

kocsismate · 2026-09-02T19:15:15Z

@@ -0,0 +1,35 @@
+--TEST--
+Test Uri\WhatWg\Url reference resolution - resolve() - relative reference


Suggested change

Test Uri\WhatWg\Url reference resolution - resolve() - relative reference

Test Uri\WhatWg\Url reference resolution - resolve() - relative-URL

Same batch as above.

NickSdot · 2026-09-02T20:04:33Z

I went through all tests and they look fine now with only the following exceptions:

That's done in 3ec9180; expect the pre-existing inconsistency. For those I'll send a follow up as requested above!

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

Comment thread ext/uri/tests/zend/internal_api_success_parse_url_parsing.phpt

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

kocsismate reviewed Sep 2, 2026

View reviewed changes

review: finalised titles

3ec9180

NickSdot mentioned this pull request Sep 3, 2026

ext/uri: Consistency follow up to #23286 #23551

Draft

		@@ -0,0 +1,22 @@
		--TEST--
		Test Uri\Rfc3986\Uri component modification - host - success - percent-encoded reserved characters

		@@ -0,0 +1,10 @@
		--TEST--
		Test Uri\Rfc3986\Uri parsing - query - emoji code point

		@@ -0,0 +1,25 @@
		--TEST--
		Test Uri\WhatWg\Url parsing - basic - parse() returns null for an emoji relative reference

		@@ -0,0 +1,35 @@
		--TEST--
		Test Uri\WhatWg\Url reference resolution - resolve() - relative reference

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!

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!

Uh oh!

Uh oh!

NickSdot commented Aug 19, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kocsismate commented Aug 31, 2026

Uh oh!

NickSdot commented Sep 1, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NickSdot commented Sep 1, 2026

Uh oh!

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

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

NickSdot commented Aug 14, 2026 •

edited

Loading

NickSdot commented Aug 20, 2026 •

edited

Loading