Hi Tim, Thanks for your feedback!
> The RFC is not listed in the overview page: https://wiki.php.net/rfc Uh, indeed! I've just fixed it. > 2. > > I agree with Dennis' remark that the `Rfc3986Uri` and `WhatWgUri` > classes must be final. The RFC makes the argument that: > > > Having separate classes for the two standards makes it possible to > > indicate explicit intent at the type level that one specific standard > > is required. > > Developers extending the classes could accidentally violate the > respective standard, which nullifies the benefit of making invalid > states unrepresentable at the type-level. > On the one hand, I also have some concerns about making these classes final or non-final as you probably saw in my last email (the concern came up with a question about an implementation detail: https://github.com/php/php-src/pull/14461#discussion_r1847316607). On the other hand though, if someone overrides an URI implementation, then I assume there's definitely a purpose for doing so (i.e. the child class has additional capabilities, or it can handle additional protocols). If developers cannot achieve this via inheritance, then they will do so otherwise (by using composition, or putting the custom logic in a helper class etc.). It's just not realistic to prevent logical bugs by making classes final. I would rather ask whether it's possible to make the 2 built-in URI implementations having quite some special internal behavior behave consistently with userland classes, even if they are overridden? For now, the answer seems to be yes (especially after hearing Niels' solution in the GitHub thread linked above), but of course new issues may arise later which we don't know about yet. And of course, it's much easier to make a class final first and relax the inheritance rules later, than the other way around... So these are the only reasons why I'd make the classes final, but otherwise it would be useful to be able to extend them. > > This also means that the return type of the “withers” should be `self` > instead of `static`, which also means that the “withers” in the > interface must be `self`. Perhaps this means that they should not exist > on the interface at all. `DateTimeInterface` only provides the getters, > likely for a similar reason. > Using the `self` return type over `static` would be counterproductive in my opinion: it's mostly because static is the correct type semantically, and it can be useful for forward compatibility later if we once want to remove the final modifier. Regarding the analogy with DateTimeInterface, I think this one is wrong: the ext/uri API is completely immutable, while ext/date has the mutable DateTime implementation, so it's not possible to include setters in the interface, otherwise one couldn't know what to expect after modification. I believe the `UriException` class as the base exception should not be > `abstract`. There is no real benefit to it, especially since it doesn't > specify any additional abstract methods. > I have no hard feelings regarding this. If I make it a concrete class, then likely implementations will start to throw it instead of more specific subclasses. That's probably not an issue, people are not usually interested in the exact reason of an exception. Since ext/date also added a generic parent exception (DateError) recently which wasn't abstract, then I'm fine with doing the same with ext/uri. > I'm not sure I like the `Interface` suffix on the `UriInterface` > interface. Just `Uri\Uri` would be equally expressive. > Yes, I was expecting this debate :) To be honest, I never liked interfaces without an "Interface" suffix, and my dislike didn't go away when I had to use such an interface somewhere, because it was difficult for me to find out what the symbol I was typing actually referred to. But apart from my personal experiences, I prefer to stay with "UriInterface" because the 2 most well known internal PHP interfaces also have the same suffix (DateTimeInterface, SessionInterface), and this name definitely conveys that people should not try to instantiate it. I am not sure about the `*User()` and `*Password()` methods existing on > the interface. As the RFC acknowledges, RFC 3986 only specifies a > “userinfo” segment. Should the `*User()` and `*Password()` methods > perhaps be specific to the `WhatWgUri` class? > Really good question, and I hesitated a lot about the same (even in some of my messages to the mailing list). In fact, RFC 3986 has some notion of user/password, because the specification mentions the "user:password" format as deprecated [in favor of passing authentication information in other places]. So I think the `*User()` and `*Password()` methods are legitimately part of the interface. And it's not even without precedent to have them in an interface: PSR-7 made use of the "user" and "password" notions in the `UriInterface::withUserInfo()` method which accepts a `$user` and a `$password` parameter. I know people on this list generally don't like PSR-7, but t would be useful to know why PHP FIG chose to use these two parameters. Due to the reasons above, the question for me is really whether we want to add the `*UserInfo()` methods to the interface or at least to Uri/Rfc3986Uri. Since WhatWg doesn't even mention user info (apart from "userinfo percent-encode set" which refers to something else), I'd prefer not to add the methods in question to Uri/UriInterface. If people insist on it, then I'm fine to add the methods to Uri\Rfc3986Uri though. I disagree with this change and believe that with the current > capabilities of PHP the out-parameter is the correct API design choice, > because then the “failure” case would be returning a falsy value, which > IMO is pretty idiomatic PHP: Yes, I can live with any of the solutions, I'm just not sure which is less bad. :) If only we had out parameters... But wishful thinking aside, I am fine with whatever the majority of people prefer. Probably being able to unify the API of the two implementations is a good argument no one thought about so far for using passing by reference... Regards, Máté
