Hi Ignace, I just re-read the RFC and I like the updates and precision you've brought > to it here's my review: > For the builders I have nothing more design wise to add this is already > solid. I may nitpick on the *Builder::clear() method name I would have gone > with *Builder::reset() but I presume other developers would go with clear. > Other than that the public API is spot on. >
I also like the reset() method name much more! So I'll update the RFC accordingly. For the Enum, my only concern is that they serve just as flags and their > usage is tightly coupled to the Uri classes. I would add 2 static named > constructors fromUrl and tryFromUrl just for completeness. I believe the > maintenance cost is negligible but the developer DX is improved and allows > for a broader usage of the Enum. > I don't really understand why it would make sense to invert the coupling? (decouple the UriHostType/UrlHostType and the UriType enums from the Uri/Url classes, and in the same time, couple the enums to the Uri/Url classes). IMO it's far more ergonomic to retrieve the URI type/host type from the URI class directly, rather than to instantiate an enum each time? > Last but not least, The Percent encoding feature should be IMHO improved > by moving the encode/decode methods from being static methods on the URI > classes to becoming public API on the Enum. This would indeed imply > renaming the enum from Uri\Rfc3986\UriPercentEncodingMode to > Uri\Rfc3986\UriPercentEncoder with two methods encode/decode. Again it > makes for a more self-contained feature and adds to the DX. Developer will > not have to always statically call the URI classes for encoding/decoding > strings as the Enums and their cases already convey the information > correctly. > Personally - and I may be in the minority - I don't see an issue with having two static methods on Uri/Url. Uri::percentEncode() and Uri::percentDecode() as well as Url::percentEncode() and Url::percentDecode() could indeed be implemented via a dedicated UriPercentEncoder and UrlPercentEncoder class, or even a shared PercentEncoder one, but: - Its methods would still be static - I don't think it's worth to add one or two dedicated classes just for this purpose I also got feedback that these functions could be free-standing in the URI namespace. But I really don't like free standing functions, so I won't go in this direction. IMO even two static methods on the Uri and Url classes are easier to use and find. So if we reject these ideas, then the next candidate is your suggestion of having a UriComponent/UrlComponent enum with an encode() and decode() method ( https://github.com/thephpleague/uri-src/pull/186#issuecomment-4016602880): $uri = new Uri\Rfc3986\Uri("https://example.com/?q=%3A%29";); $query = $uri->getQuery(); // returns "q=%3A%29" echo Uri\Rfc3986\UriComponent::Query->decode($query); // returns "q=:)" But as I mentioned in my comment on the PR, percent-encoding/decoding is not necessarily tied to URI/URL components, for example because the proposal currently contains the Uri\Rfc3986\UriPercentEncodingMode::AllReservedCharacters, Uri\Rfc3986\UriPercentEncodingMode::AllButUnreservedCharacters, or Uri\Rfc3986\UriPercentEncodingMode::PathSegment. Some of the enum cases which don't relate to a component could be removed, but at least the AllReservedCharacters case is important because it provides a direct alternative for rawurlencode() and rawurldecode(). My side-quest is to gradually phase out *urlencode() and *urldecode() functions because their naming is very confusing, and people usually don't know when to use which. And I've just noticed that probably yet another enum case would be needed to provide a direct alternative for urlencode() and urldecode(), because they differ from rawurlencode() and rawurldecode() with regards to how the "~" is handled, besides the " " character. But at this point, I became unsure if it's worth to pursue this goal, because this is not RFC 3986 compliant behavior anymore (and TBH not even Uri\Rfc3986\UriPercentEncodingMode::FormQuery is compliant), so it has nothing to do with the Uri\Rfc3986 namespace. So all in all... As far as I can see, not even a Uri\Rfc3986\UriComponent enum could provide a complete solution for the custom percent-encoding/decoding part of the proposal. If we used a Uri\Rfc3986\UriEncoding enum name instead, then there would be no issue with the various kinds of encoding/decoding modes not referring to URI components, but the naming would probably still not be right, as I wouldn't expect a class name with "ing" suffix to perform percent-encoding/decoding itself. But I'm happy to be corrected by native English speakers :) As I don't have any other ideas, I think I still prefer the static method based approach. Regards, Máté
