search

thephpleague / uri-src

URI manipulation Library
https://uri.thephpleague.com
MIT License
30 stars 7 forks source link

Introduce the Encoder class #114

Closed nyamsprod closed 1 year ago

nyamsprod commented 1 year ago

Introduce the Encoder class to normalize encoding/decoding in all packages.

nyamsprod commented 1 year ago

@trowski here's my take on how to properly resolve #109 . I went back and read RFC3986. I centralized all encoding in one file the new Encoder class and also added a KeyValueConverter class to ease everything.

Feedback are welcomed. For instance I am trying to see if I can virtually add the new converter usage without a BC break would allow advance users to setup their own encoding/decofing info for instance.

trowski commented 1 year ago

Reviewing the changes here was on my todo list for today, but I see I'm a little late.

The output from RFC3986 looks good and is as I'd expect with all reserved characters being encoded by default. Thanks for taking care of this issue!

I find the withRfc3986Output and withEncodingOutput methods confusing. Are these suppose to be corresponding input and output values? If so, I'd pass a map withEncodingMap(['%20' => '+']) to make it clear what maps to what.

nyamsprod commented 1 year ago

There was a time where it was a mapper but then the split methods made it easier for validation so I can certainly revisit this part in a fix directly on master as long as we are on the same page for the rest 😉. Thanks the review

trowski commented 1 year ago

Validation IMO is harder with separate methods, but maybe I'm not understanding what you mean. I'm happy to provide a PR for what I have in mind if you'd like.

nyamsprod commented 1 year ago

Fixed in the master https://github.com/thephpleague/uri-src/commit/e7dfac7f7c511eef13a35a8a824237048ae433a2 and code is also simplified 😉

trowski commented 1 year ago

Thanks @nyamsprod, that looks great!