Reorganize ext/uri tests - withers #19970

kocsismate · 2025-09-26T06:44:20Z

No description provided.

ext/uri/tests/whatwg/modification/host_error_forbidden_host_codepoint_opaque2.phpt

ext/uri/tests/whatwg/modification/path_success_auto_encoded.phpt

TimWolla

I went through all RFC 3986 tests. Looking pretty good overall, some nits. I also think it would be helpful to print the entire resulting URL in addition to printing the output of the getters, this makes it easier to double-check how exactly the resulting URL will look, particularly with regard to the difference between null and "" (e.g. whether there is a ? for the query or not).

ext/uri/tests/rfc3986/modification/fragment_success_encoded.phpt

ext/uri/tests/rfc3986/modification/host_success_ipv6.phpt

ext/uri/tests/rfc3986/modification/path_error_reserved.phpt

ext/uri/tests/rfc3986/modification/port_error_overflow.phpt

ext/uri/tests/rfc3986/modification/roundtrip_special_case3.phpt

ext/uri/tests/rfc3986/modification/scheme_error_empty.phpt

ext/uri/tests/rfc3986/modification/scheme_error_reserved.phpt

ext/uri/tests/rfc3986/modification/userinfo_success_existing.phpt

ext/uri/tests/rfc3986/modification/userinfo_success_new.phpt

kocsismate · 2025-09-28T12:51:55Z

I also think it would be helpful to print the entire resulting URL in addition to printing the output of the getters, this makes it easier to double-check how exactly the resulting URL will look, particularly with regard to the difference between null and "" (e.g. whether there is a ? for the query or not).

Yes, I was also thinking about something similar (at least in some edge cases), but I didn't want to make the output too "crowded". My other concern is that most states can be tested via regular parsing 🤔 (we don't necessary have to use withers for them)

kocsismate · 2025-09-29T19:54:18Z

@TimWolla What do you think about verifying URI recomposition separately? Or do you think we should always test the recomposed URI after URI modification?

TimWolla · 2025-10-04T09:46:04Z

Or do you think we should always test the recomposed URI after URI modification?

Always, because this also helps the human reader understand the test better.

TimWolla · 2025-10-04T09:47:15Z

but I didn't want to make the output too "crowded".

If the tests follow a common structure / order, that's easy to check and scan. You could also insert an additional newline between the various output “sections”.

nielsdos · 2025-10-04T23:07:06Z

Unless I'm really needed here I'm removing my review request.
The honest reason is that I'm not really looking forward to reviewing 2K lines of tests in my spare time 😅

TimWolla

Taken another look at the RFC 3986 tests now. Generally LGTM. Will still need to look at the WHATWG tests.

ext/uri/tests/rfc3986/modification/host_success_encoded.phpt

ext/uri/tests/rfc3986/modification/fragment_success_encoded.phpt

ext/uri/tests/rfc3986/modification/path_success_encoded.phpt

ext/uri/tests/rfc3986/modification/query_success_context_sensitive_reserved.phpt

ext/uri/tests/rfc3986/modification/query_success_encoded.phpt

ext/uri/tests/rfc3986/modification/scheme_success_basic.phpt

TimWolla · 2025-10-12T20:12:16Z

ext/uri/tests/whatwg/modification/fragment_success_empty.phpt

Can you also add a test with ->withFragment("#")?

This revealed a bug :)

For now, I pushed the fixes with the same commit, but I plan to separately apply it before this PR is merged.

ext/uri/tests/whatwg/modification/host_error_forbidden_host_codepoint_opaque2.phpt

ext/uri/tests/whatwg/modification/password_success_existing.phpt

ext/uri/tests/whatwg/modification/password_success_unset_existing.phpt

TimWolla · 2025-10-12T20:15:27Z

ext/uri/tests/whatwg/modification/password_success_unset_non_existent.phpt

@@ -0,0 +1,19 @@
+--TEST--
+Test Uri\WhatWg\Url component modification - username - unsetting non-existent


This description does not match the test and does not match the filename.

TimWolla · 2025-10-12T20:16:42Z

ext/uri/tests/whatwg/modification/port_error_overflow.phpt

Can be removed, this is just testing the signature (similarly to the one in Rfc3986 that was already removed).

TimWolla · 2025-10-12T20:17:44Z

ext/uri/tests/whatwg/modification/query_success_context_sensitive_reserved.phpt

Typo in filename

ext/uri/tests/whatwg/modification/query_success_empty.phpt

ext/uri/tests/whatwg/modification/username_success_existing.phpt

ext/uri/tests/whatwg/modification/username_success_unset_non_existent1.phpt

nielsdos · 2025-10-14T20:30:42Z

ext/uri/uri_parser_whatwg.c

 	const lxb_url_t *lexbor_uri = uri;

-	if (lexbor_uri->fragment.length) {
+	if (lexbor_uri->fragment.data != NULL) {


FWIW, with this changed now, a follow-up PR (probs for master) may change the ZVAL_STRINGL to ZVAL_STRINGL_FAST to avoid allocating empty strings.

kocsismate requested review from TimWolla and nielsdos September 26, 2025 06:44

github-actions bot added the Extension: uri label Sep 26, 2025

kocsismate commented Sep 26, 2025

View reviewed changes

ext/uri/tests/whatwg/modification/host_error_forbidden_host_codepoint_opaque2.phpt Outdated Show resolved Hide resolved

ext/uri/tests/whatwg/modification/path_success_auto_encoded.phpt Show resolved Hide resolved

kocsismate force-pushed the ext-uri-tests1 branch from 00e380e to dd5e549 Compare September 26, 2025 06:52

kocsismate changed the base branch from master to PHP-8.5 September 26, 2025 07:40

Reorganize ext/uri tests - withers

059b1a1

kocsismate force-pushed the ext-uri-tests1 branch from dd5e549 to 059b1a1 Compare September 27, 2025 11:31

TimWolla reviewed Sep 27, 2025

View reviewed changes

Address review comments

d32421f

kocsismate added 2 commits October 4, 2025 15:38

Display full URI

26e17e4

Remove unnecessary test

98f5282

nielsdos removed their request for review October 4, 2025 23:06

TimWolla reviewed Oct 5, 2025

View reviewed changes

Address review comments

48072a6

TimWolla reviewed Oct 12, 2025

View reviewed changes

Add more tests, fix bugs, address code review

3fcba8e

nielsdos reviewed Oct 14, 2025

View reviewed changes

This was referenced Oct 17, 2025

ext/uri: Fix the distinction between an empty and a missing query/fragment for WHATWG URLs #20208

Open

Add NoDiscard attribute for Uri::with*() and Url::with*() methods #20209

Closed

		@@ -0,0 +1,19 @@
		--TEST--
		Test Uri\WhatWg\Url component modification - username - unsetting non-existent

Reorganize ext/uri tests - withers #19970

Are you sure you want to change the base?

Reorganize ext/uri tests - withers #19970

Uh oh!

Conversation

kocsismate commented Sep 26, 2025

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

kocsismate commented Sep 28, 2025

Uh oh!

kocsismate commented Sep 29, 2025

Uh oh!

TimWolla commented Oct 4, 2025

Uh oh!

TimWolla commented Oct 4, 2025

Uh oh!

nielsdos commented Oct 4, 2025

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!

TimWolla Oct 12, 2025

Choose a reason for hiding this comment

Uh oh!

kocsismate Oct 14, 2025

Choose a reason for hiding this comment

Uh oh!

kocsismate Oct 14, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TimWolla Oct 12, 2025

Choose a reason for hiding this comment

Uh oh!

TimWolla Oct 12, 2025

Choose a reason for hiding this comment

Uh oh!

TimWolla Oct 12, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nielsdos Oct 14, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants