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 · 2025-10-24T11:51:26Z

I got rid of the Uri\WhatWg\Url::getQuery() and Uri\WhatWg\Url::getFragment() related changes so that this PR is at least uncontroversial again.

TimWolla

I trust that you added all the extra tests that I suggested. All files in this PR are now marked as "Viewed" for me and I didn't see any further issues after the latest changes / don't have further comments (or just a minor typo in a test title). Thus this should be good to go and clearly improves the test coverage and organization. If necessary we can just make some follow-up to fix issues we notice afterwards.

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

* PHP-8.5: Reorganize ext/uri tests - withers (#19970)

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

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

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

TimWolla reviewed Oct 5, 2025

View reviewed changes

TimWolla reviewed Oct 12, 2025

View reviewed changes

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

kocsismate added 7 commits October 24, 2025 13:44

Reorganize ext/uri tests - withers

f44a12d

Address review comments

2eac512

Display full URI

877319e

Remove unnecessary test

4600f84

Address review comments

42ec15e

Add more tests, fix bugs, address code review

dc83c44

Get rid of fixing bug

3fef42e

kocsismate force-pushed the ext-uri-tests1 branch from 3fcba8e to 3fef42e Compare October 24, 2025 11:49

TimWolla approved these changes Oct 24, 2025

View reviewed changes

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

Fix typo

847eb3c

kocsismate merged commit 27bc7c0 into php:PHP-8.5 Oct 25, 2025
9 of 10 checks passed

kocsismate deleted the ext-uri-tests1 branch October 25, 2025 12:46

kocsismate added a commit that referenced this pull request Oct 25, 2025

Merge branch 'PHP-8.5'

0cdf7be

* PHP-8.5: Reorganize ext/uri tests - withers (#19970)

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

Uh oh!

Reorganize ext/uri tests - withers #19970

Reorganize ext/uri tests - withers #19970

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!

kocsismate commented Oct 24, 2025

Uh oh!

TimWolla left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone