Skip to content

ext/uri: applied fixers to improve test robustness - #23271

Open
NickSdot wants to merge 1 commit into
php:masterfrom
NickSdot:hardening/ext-uri
Open

ext/uri: applied fixers to improve test robustness#23271
NickSdot wants to merge 1 commit into
php:masterfrom
NickSdot:hardening/ext-uri

Conversation

@NickSdot

@NickSdot NickSdot commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

The one you didn't cherry pick (@ Tim); here without the style only lines. 🫡

Comment thread ext/uri/tests/004.phpt
@@ -9,7 +9,7 @@ var_dump(Uri\Rfc3986\Uri::parse(""));
try {
new Uri\WhatWg\Url("");
} catch (Uri\WhatWg\InvalidUrlException $e) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Will you want to catch Throwable? The current code makes review easier for now, but I guess the intention is to use Throwable ultimately, right?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Generally the PR seeks to just assert what's currently thrown. If you want Throwable I can do it with a commit in here or send a follow up PR - which you prefer?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm fine doing it here, thanks! :)

P.S These are "legacy" tests which should be moved into a more suitable place (one of the existing test subdirectories), and some of the tests are already replaced by newer tests in those subdirectories. The newer tests already use Throwable in the catch. :)

@kocsismate kocsismate left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM especially if you change the catches to Throwable

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