Implement "Followup improvements for ext/uri" RFC - URL building with base URL

[kocsismate]

RFC: https://wiki.php.net/rfc/uri_followup#uri_building

Add support for passing a non-null $baseUrl parameter for Uri\WhatWg\UrlBuilder::build().

This PR is not complete yet (tests are missing).

[kocsismate]

Overall, this code below is pretty much hacky, works coincidentally, and to be honest I wish it wouldn't exist. 😅 On the other hand, it gets the job done! So thank you @arnaud-lb for the suggestion, your idea works indeed :) I would have never thought about it.

Due to the above mentioned implementation difficulties though, IMO we should prioritize if the feature is worth more or the sanity of our code ^^

[NickSdot]

@kocsismate is the following expected?

### Rfc3986
$builder = new Uri\Rfc3986\UriBuilder();
$builder->setPort(123);

try {
    var_dump($builder->build(new Uri\Rfc3986\Uri("https://example.com"))->toAsciiString());
} catch (Throwable $e) {
    echo $e::class, ': ', $e->getMessage(), "\n";
}

// Uri\InvalidUriException: Cannot set a port without having a host


### WhatWg
$builder = new Uri\WhatWg\UrlBuilder();
$builder->setPort(123);

try {
    var_dump($builder->build(new Uri\WhatWg\Url("https://example.com"))->toAsciiString());
} catch (Throwable $e) {
    echo $e::class, ': ', $e->getMessage(), "\n";
}

// string(24) "https://example.com:123/"

I would expect RFC3986 to behave like WHATWG here. While trying to come up with tests for here and some more for #23342 I got stuck one way or another. The follow up RFC does not really spell out what the merging behaviour should be. The discussion and comments point towards resolution, but that also does not seem to be how everything consistently works?

For me it seems like WHATWG behaves sanely in the example above and RFC3986 does not, but then there are situations where WHATWG seems wrong too: A) see above; B) setting a fragment drops the query; C) fragments are appended instead of replaced; D) setting a username without a host results in https://example.comnew/a; and more.

Possible that I am having a dumb day, but I cannot really make sense of what the expected behaviour is. Could you please clarify how both RFC3986 and WHATWG are supposed to merge with a base URI/URL? Must the builder components form a valid reference on their own before resolving or can the base provide missing parts like the host?

Would love to add some tests to lock in the findings, but I really could use a hint first.

[kocsismate]

I would expect RFC3986 to behave like WHATWG here. While trying to come up with tests for here and some more for #23342 I got stuck one way or another. The follow up RFC does not really spell out what the merging behaviour should be. The discussion and comments point towards resolution, but that also does not seem to be how everything consistently works?

Ah no, it should be the other way around, and this was also due to a small ordering issue in the builder's code.

The merging behavior should be the very same what is done during parsing. E.g.

$builder = new Uri\WhatWg\UrlBuilder();
$builder->set*(...);
...
$url1 = $builder->build($baseUrl);

$url2 = new Uri\WhatWg\Url("...", $baseUrl);

For each $baseUrl, one of the two must be true:

• $url1 and $url2 are both invalid with the same reason
• $url1 and $url2 are equivalent

(Given that the resulting $url1 after the set*() calls would yield the same URL string as $url2)

The same goes for RFC 3986.

A) #23526 (comment);

Answered above.

B) setting a fragment drops the query;

Can you please elaborate upon this?

C) fragments are appended instead of replaced

Ah, this is an "annoying" behavior of the WHATWG URL spec (because our use-case is not really supported by the spec). That's why I had to add e.g. lxb_url_path_set_null(lexbor_url); and similar calls, but I forgot to do it with the query and the fragment.

D) setting a username without a host results in https://example.comnew/a; and more.

This is the same category as A, and it's fixed with the code reordering.

[NickSdot]

Ah no, it should be the other way around, and this was also due to a small ordering issue in the builder's code.

The merging behavior should be the very same what is done during parsing. E.g.

$builder = new Uri\WhatWg\UrlBuilder();
$builder->set*(...);
...
$url1 = $builder->build($baseUrl);

$url2 = new Uri\WhatWg\Url("...", $baseUrl);

For each $baseUrl, one of the two must be true:

• $url1 and $url2 are both invalid with the same reason
• $url1 and $url2 are equivalent

I see why it is. But it also makes it very restrictive and rather surprising for "normal" usage. Like, not being able to append a port is kinda unfortunate. But fair enough.

Having some way to have "component overlays" would be neat. Like, how I expected it - just to replace/append/remove from a given URL. Probably that's something for the future, maybe UrlBuilder::fromUrl(). Different topic...

Can you please elaborate upon this?

I believe it was:

$base = https://example.com/a?old"
$builder->setFragment("new");
$builder->build($base); // https://example.com/a#new

But I will need to double check tomorrow.

[kocsismate]

I believe it was:
$base = https://example.com/a?old"
$builder->setFragment("new");
$builder->build($base); // https://example.com/a#new
But I will need to double check tomorrow.

Oh right, thanks for noticing it again. There was an existing test for this case with incorrect expectation. Now, it should be fixed 🤔 I hope there aren't more edge cases which I missed.

[nyamsprod]

@kocsismate I was able to fix a couple of discrepancies in my polyfill and now I have the same results as yours for the Uri\Rfc3986\UriBuilder. My polyfill was more lenient than your implementation around path resolution during URI building.

For Uri\Whatwg\UrlBuilder it's another story I have 2 main questions/remarks.

$builder = new Uri\WhatWg\UrlBuilder();
$builder->setScheme("foo");

$url = $builder->build();

var_dump($url->toAsciiString());
var_dump($url);
var_dump($url->equals(new Uri\WhatWg\Url($url->toAsciiString())));

Currently the test expects foo:// but my implementation returns foo: the reason my implementation does so is because no authority component (host, port, userinfo) was set whereas in your implementation you seems to default to the empty string host. Which seems unexpected.

My implementations always populates the $errors argument when calling the build method. The tests seems not to cover such generator to see if the resulting error code are the ones expected or if they are indeed popupated.

That's all... for now 😉

[kocsismate]

Thanks, Ignace, for the tests! I can confirm that this case is a real issue. And I found a couple of other edge cases (e.g. empty string for query/fragment.

Regarding, the $errors parameter: I should have used $softErrors just like for Uri\WhatWg\Url::__construct(). I'm going to fix these with a followup PR.

[kocsismate]

Submitted #23636 to fix the incompatibilites with parsing + some other bugs.

[ndossche]

Small bug:

<?php
$u = (new Uri\WhatWg\UrlBuilder())->setPath("D:/c")->build(new Uri\WhatWg\Url("file:///C:/a/b"));

var_dump($u->toAsciiString()); // string(12) "file:///D:/c"
var_dump($u->withPath("/q")->toAsciiString()); // string(12) "file:///D:/c", but expected "file:///q"

Seems to be because php_uri_parser_whatwg_resolve_reference_from_zval doesn't clear the opaque flag.

[ndossche]

It seems that the term "query" and "query string" are used mixed in this file.