ext-uri when to throw ValueError vs InvalidUriException

[nyamsprod]

Description

The following code:

test

Resulted in this output:

Currently the tests returns a `ValueError`

But I expected this output instead:

Uri\WhatWg\InvalidUrlException: The specified URI is malformed; Invalid URL (DomainInvalidCodePoint)

To me both are acceptable, I would lean toward the second options which is what the WHATWG URL standard would expect but I understand the current output. I just want to know if there is a clear guideline when to throw a ValueError vs InvalidUrlException because from a DX usage it may mean that withers can only throw ValueError ? 🤔

PHP Version

using php master branch (but this is irrelevant)

Operating System

No response

[devnexen]

it seems to me the parameter is of type Z_PARAM_PATH_STR thus with the null byte in the middle of your test case, it already takes precedence though I guess (as not even reaching the lexbor parser).

[nyamsprod]

OK so following your reasoning, the null byte character will never reach the lexbor parser so InvalidUrlException can never be thrown.

[devnexen]

IMHO, those are not bugs at all.

[nyamsprod]

I am not saying it is a bug I just need to understand when one is thrown vs the other to improve my polyfill. Currently the polyfill never throw the ValueError and everything is validated by the WHATWG parser which means I need to add a specific fix for the null byte in order to match PHP implementation.

[devnexen]

yes so to rephrase it, it is expected outcome in my view. Hope it helps.

[nyamsprod]

@devnexen I have added the following

!str_contains($uri, "\0") || throw new ValueError('Argument #1 ($uri) must not contain any null bytes');

prior to any validation by the userland URI parser.

I hope this is the only gatekeeper to look for 🤔

[devnexen]

I ll let @TimWolla respond on this particular point ;)

[TimWolla]

I hope this is the only gatekeeper to look for 🤔

Yes. That's Z_PARAM_PATH_STR vs Z_PARAM_STR internally.

As for this issue, I'd be inclined to just pass along NUL bytes as-is to the parsers for consistent error handling, but I didn't yet have time to check if all the methods are fully NUL-safe (and I would need to check with Mate as well).

[nyamsprod]

@TimWolla thanks for the comment. So if you end up passing it along I will be able to remove my current "patch" then. Let me know if this is the case.

[devnexen]

@TimWolla I think your point is valid, might be a tougher decision in another hand. i.e. most of other php extensions treat nul bytes from input prevently. But again, making it a thinner wrapper around lexbor is a valid POV.