[nyamsprod]

A new implementation which is more accurate

[nyamsprod]

@kocsismate @kamil-tekiela @TimWolla the new implementation of the polyfill is more inline with PHP implementation but it has one major difference around exception. All exceptions raised in PHP test suite are triggerrd in the polyfill but they differ in the UrlValidationErrorType being recorded.

In my builder I generate the candidate URL using WHATWG UrlRecord and then offload the actual parsing to the Url constructor so most of the UrlValidationErrorType are the one that would be issued by the Url constructor.

An example is the following taken from the test on php-src:

$builder = new Uri\WhatWg\UrlBuilder();
$builder->setScheme("file");
$builder->setPath("C:/a.txt");
$builder->setPassword("password");

try {
    $builder->build();
} catch (Throwable $e) {
    echo $e::class, ': ', $e->getMessage(), "\n";
}

The expected error is the following

Uri\WhatWg\InvalidUrlException: The specified URL cannot have password

but my implementation will return

Uri\WhatWg\InvalidUrlException: The specified URL is malformed (HostMissing InvalidUrlUnit)

because the generated URL is still exposed to the constructor. I presume this is not the case for your implementation.

The goal is not to debate who's wrong or right but to highlight that a bad URL can trigger different exception depending on how it is feed to the WHATWG URL spec.

[kocsismate]

@nyamsprod Thanks for letting us know about this!

Yes, the main difference must surely be that the ext/uri implementation relies on the setters with a few "tweaks", while the polyfill apparently uses the basic parser. There was quite a long discussion about this in one of the builder fixes a week ago: php/php-src#23815 (comment)

In that thread, I initially wrote that the Builder should behave just like the parser does, but after all, we agreed to rather "copy" the behavior of the setters with our own tweaks (e.g. added base URL support + opaque path support).

And later I realized that my original phrasing was not really accurate (that the Builder should follow the regular parser rules), because:

• it's more ergonomic to implement the Builder based on the setters (we can reuse more code from lexbor), and the setters rely on state overrides when using the basic parser algorithm
• but the basic parser algorithm differs quite much when the state override is given and when it's not given. Even though the differences are logical, the thing is that strictly speaking, it's not feasible to entirely follow the parsing algorithm with the setters.

After all, the current implementation generally follows setter rules with some extra validations (e.g. The specified URL cannot have password is one of them, because the setters would silently ignore these cases), and some of the missing features are retained (like proper support for opaque paths and base URLs).

P.S. There are also a few other nuances with error handling on the internal implementation:

• due to an unfortunate implementation choice during the original ext/uri implementation, ''$errors'' contain the errors in backward order.
• when using the Builder, $errors contain the error instances in backward order per component, because of using the setters with the unfortunate error handling code
• the username/password setter is used after the host setter so that the host is already known when setting the credentials, that's why the errors triggered by username/password will come after the errors triggered by the host.
• the parser removes tabs/newlines etc. as a first step (and a soft error is emitted), while the setters remove the same characters per component -> triggering the same soft errors in a different order
• there's also ext/uri: Fixes inconsistent validation ordering php/php-src#23823 which improve a little bit on the current situation

[NickSdot]

@kocsismate

it's more ergonomic to implement the Builder based on the setters (we can reuse more code from lexbor), and the setters rely on state overrides when using the basic parser algorithm

it's not feasible to entirely follow the parsing algorithm with the setters.

IMHO the builder API should not be forced into either. The specs define parsing and internal setters, not public builder setters. The builder API should be allowed to do their own preprocessing as long as the underlying result follows the spec. Forcing builder setters into either semantics decreases ergonomics. Another example than what's mentioned here: WHATWG setter (builder) will behave different than the RFC setter (builder; which throws on leading #).