Skip to content

Conversation

johha
Copy link
Contributor

@johha johha commented Aug 9, 2022

Replace URI.encode with URI.encode_www_form_component to make gem compatible to Ruby version to 3.x.

The integration tests passed successfully however there might be some specials case as previously only certain characters ('/[^!*\'()\;?:@#&%=+$,{}[]<>" '`) where encoded. Therefore a thorough review is needed before merging this PR.
Related to #156.

Update Ruby version to 3.0.

Co-authored-by: Johannes Haass <[email protected]>
@johha
Copy link
Contributor Author

johha commented Aug 9, 2022

The PR does not seem to fix the problem, there are still errors.

@philippthun
Copy link
Contributor

I've tested different "encoders" and compared them with URI.encode(some_string, '/[^!*\'()\;?:@#&%=+$,{}[]<>`" '). For testing purposes I used a string containing all ascii characters plus space: "!"#$%&'()*+,-./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\]^_`abcdefghijklmnopqrstuvwxyz{|}~ ".

I've encoded this test string with the currently used implementation, with URI.encode_www_form_component, with CGI.escape and with Addressable::URI.encode_component.

The exact same result can be achieved by replacing URI.encode(some_string, '/[^!*\'()\;?:@#&%=+$,{}[]<>`" ') with Addressable::URI.encode_component(some_string, Addressable::URI::CharacterClasses::UNRESERVED + '|').

Question: Is there a reason why the pipe symbol (|) is handled differently compared to other symbols? Or was it just forgotten in the list of characters passed to URI.encode?

@jiangytcn
Copy link

The PR does not seem to fix the problem, there are still errors.

@johha @philippthun still having errors now ?

@philippthun philippthun mentioned this pull request Aug 11, 2022
@philippthun
Copy link
Contributor

@jiangytcn We've opened a new PR (#158) with which we could successfully run our deploy pipeline on AliCloud (still only a development environment with a limited set of components).

We can close this PR and move on to the new one.

It would still be good to get an answer for my question raised above about the pipe symbol. I guess we could replace...

Addressable::URI.encode_component(some_string, Addressable::URI::CharacterClasses::UNRESERVED + '|')

... with...

Addressable::URI.encode_component(some_string, Addressable::URI::CharacterClasses::UNRESERVED)

..., or is there a reason for the special handling of |?

@jiangytcn
Copy link

jiangytcn commented Aug 11, 2022

@jiangytcn We've opened a new PR (#158) with which we could successfully run our deploy pipeline on AliCloud (still only a development environment with a limited set of components).

We can close this PR and move on to the new one.

It would still be good to get an answer for my question raised above about the pipe symbol. I guess we could replace...

Addressable::URI.encode_component(some_string, Addressable::URI::CharacterClasses::UNRESERVED + '|')

... with...

Addressable::URI.encode_component(some_string, Addressable::URI::CharacterClasses::UNRESERVED)

..., or is there a reason for the special handling of |?

I would ask @xiaozhu36 to answer that, do you know the reasons of the | char ?

@stephanme
Copy link

I suggest to close this PR. The change is included in #158.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants