Skip to content

Conversation

kocsismate
Copy link
Member

No description provided.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ^ character is in the percent-encode set of the path (https://url.spec.whatwg.org/#path-percent-encode-set), but somehow it's not percent-encoded.

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

@TimWolla TimWolla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

<?php

$uri1 = Uri\Rfc3986\Uri::parse("https://example.com");
$uri2 = $uri1->withHost("[2001:db8:3333:4444:5555:6666:7777:8888]");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
$uri2 = $uri1->withHost("[2001:db8:3333:4444:5555:6666:7777:8888]");
$uri2 = $uri1->withHost("[2001:0db8:3333:4444:5555:6666:7777:8888]");

Perhaps that to showcase whether or not IPv6 addressed will be normalized?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P.S. I had to realize (again) that IPv6 addresses are not shortened during normalization. During normalization, only the casing of hexadecimal characters is corrected (they are either lowercased or uppercases).

But using an address that could be shortened is a good idea for sure.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is just testing that type checks work. I think it can be removed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test was originally added by you when you fixed a bug. Do you think it can really be removed?

@kocsismate
Copy link
Member Author

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
Copy link
Member Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants