Adding new Rfc3986 polyfills

[nyamsprod]

No description provided.

[nyamsprod]

@kocsismate, @TimWolla and @nicolas-grekas as always I tried implementing the UriBuilder at least the RFC3986 version of it.

Here's an usage example:

(new Uri\Rfc3986\UriBuilder())
    ->scheme("https")
    ->userInfo("user:pass")
    ->host("example.com")
    ->port(8080)
    ->pathSegments(["foo", "bar"])
    ->query("a=1&b=2")
    ->fragment("section1")
    ->build()
    ->toString();
// returns https://user:pass@example.com:8080/foo/bar?a=1&b=2#section1

I have intentionally remove the set prefixes:

My first thoughts are as follow:

• It is not stated in the RFC but I hope it is an immutable object, having a mutable builder is IMHO not a great design.
• I added the withPathSegments here simply called pathSegments from what I can tell, we do not need to specify if the path is absolute or not. The way Uri::withPath already works handle the issue. So I used the same logic in the build method, hence I am now wondering if this is in fact a real issue. If one absolutely wants a leading slash he/she could do so by doing withPathSegments(['', 'yes'])
• Also the withPathSegments is not part of the UriBuilder RFC as fas as I can see, if the withQueryParams is part of the public API I fail to see why withPathSegments is not 🤷🏾
• I intentionally added some validation on the with to throw when the input is already not valid.
• • I intentionally left out the withQueryParams because when I checked the tests in the implementation it raised more questions than answer and I will right a follow up email in the internal mailing list when I get more time.

Last but not least I wonder why we could not have on the top namespace something like the following:

Uri\Builder;

Builder::buildUrl(?Uri\WhatWg\Url $baseUrl = null): Uri\WhatWg\Url;
Builder::buildUrI(?Uri\Rfc3986\Uri $baseUri = null): Uri\Rfc3986\Uri;

We would have one builder with two building methods one for each type of URI. Since component are the same. It would probably push validation inside the build* methods but it would
reduced the number of builder which sahres a lot of common methods ?

[kocsismate]

Just a very wuick note for now: the implementation is outdated and incomplete quite much, so please refer to the rfc for now which is much more complete.

But I agree some questions have to be answered related to the queryparams, especially percent encoding-wise. The rfc should describe all the info needed.

[nyamsprod]

@kocsismate the current implementation is based on the RFC but I did not include the queryParams class. Otherwise everything else is based solely on the RFC.

[kocsismate]

@nyamsprod sorry, my bad. I thought you were referring to my implementation when you wrote "I checked the test in the implementation" 🙂

[kocsismate]

It is not stated in the RFC but I hope it is an immutable object, having a mutable builder is IMHO not a great design.

What are your problems with a mutable builder? Is it its statefulness that prevents it to be dependency injected? I chose it to be mutable because URIs are already immutable, so an immutable builder wouldn't solve the performance penalty caused by the withers.

To be fair, Ilija told me that it may be possible to minimize the perf penalty by checking if the return value is used: if not, then there's no need to clone the object (I think this is what he said). If this is possible, then I may be OK with an immutable builder class.

I added the withPathSegments here simply called pathSegments from what I can tell, we do not need to specify if the path is absolute or not. The way Uri::withPath already works handle the issue. So I used the same logic in the build method, hence I am now wondering if this is in fact a real issue. If one absolutely wants a leading slash he/she could do so by doing withPathSegments(['', 'yes'])

Also the withPathSegments is not part of the UriBuilder RFC as fas as I can see, if the withQueryParams is part of the public API I fail to see why withPathSegments is not 🤷🏾

It's just an oversight :) I'll add it to the proposal next time I update it.

I intentionally added some validation on the with to throw when the input is already not valid.

Yes, it's fine. I think the most tricky cases are the following (without checking uriparser's behavior, just from the top of my head):

• userinfo is set, but host doesn't: this is what I have already mentioned in code review
• scheme is not set, then path validation is tricky: in the setters/withers, validating according to the segment syntax is needed, but the build() method should also reject the path if the first segment contains a ":" character (path-noscheme)
• in case of relative references, there's a similar situation, build() should validate according to either path-absolute or path-rootless

I intentionally left out the withQueryParams because when I checked the tests in the implementation it raised more questions than answer and I will right a follow up email in the internal mailing list when I get more time.

I'm looking forward to your evaluation.

P.S. I hate that both WHATWG URL, both uriparser use application/x-www-form-urlencoded for percent-encoding/decoding, this makes interoperability with the query component basically non-working. BTW do you have any experience with this topic? What I can see is that something will need to be changed for RFC 3986. Let's see for example:

$uri = new Uri\Rfc3986\Uri("https://example.com?%61bc");     // query is "abc"

$params = $uri->getQueryParams();                            // $params now contains [ ["%61bc" => null] ]

echo $params->toString();                                    // %2561bc

Double-encoding...

At least WHATWG's Uri\WhatWg\UrlQueryParams::parse()solves the same issue by automatically percent-decoding the input. So probably the same should be done by UriQueryParams. Although, the best would be to (additionally) make it possible to parse according to regular RFC 3996 percent-encoding/decoding rules...

[kocsismate]

BTW I've just seen your Query::fromRFC1738() method when I was checking https://uri.thephpleague.com/components/7.0/query/#countable-and-iteratoraggregate. I like this approach very much. Let me think if I could follow the same behavior :)

[nyamsprod]

@kocsismate I have updated the implementation and added more checks during URI building

• if userinfo exists but no host -> exception
• if port exists but no host -> exception
• if no scheme and no authority and colon present in first segment -> exception

and the builder is made mutable, for now, I will hold on commenting on the Query method to let you brainstorm and perhaps come up with an improved design 😉 🤞🏾

Also I added SensitiveParameter to setUserInfo just in case. I believe it is missing in the RFC text.

[kocsismate]

Also I added SensitiveParameter to setUserInfo just in case. I believe it is missing in the RFC text.

Yes, thanks!

The more I think the more I believe your Query design is the right choice: having a unified class both for RFC 3986 and WHATWG with different input (from*() methods) and output options (to*() methods). As far as I saw, the main (and probably only) differences between the implementations of uriparser and lexbor exactly lies in the parsing and output of query params, but lexbor implements all the methods that I would need. So I think the only thing needed is to implement RFC 3986 parsing for lexbor's URLSearchParams implementation, as well as RFC 3986 based recomposition + percent-encoding ^^

[nyamsprod]

So I think the only thing needed is to implement RFC 3986 parsing for lexbor's URLSearchParams implementation, as well as RFC 3986 based recomposition + percent-encoding ^^

The "funny part" 😄

I will leave the PR open while you are working on your Query implementation and/or RFC and see if I can add the other part and methods but I Believe the Query is the main difficult part as well as deciding if we really need to pass the complete object to the URI objects or force the developer to choose what he wants as shown in this pseudo code:

Uri::withQuery($queryParams->toRfc3968()); // the developer decides
Url::withQuery($queryParams->toRfc3968()); // the developer decides wrongly or not ? it depends
//or
Uri::withQueryParams($queryParams); // the object chooses on behalf of/ or restrict the developer
`` 

[nyamsprod]

@kocsismate I have refactored my PR. It contains only things that can be polyfill so any extra method added to the Uri\WhatWg\Url or Uri\Rfc3986\Uri classes are out of scope.

I was wondering if we should not also include the following methods:

Uri\Rfc3986\UriHostType::fromUri(Uri $uri): self;
Uri\Rfc3986\UriType::fromUri(Uri $uri): self;
Uri\WhatWg\UrlHostType::fromUrl(Uri\WhatWg\Url $url): self;

To emphasise that the logic is no coupled to the URI object ? And the bonus side effect it that is that they can be introduced via the polyfill 😉