Add support for Unicode links

[mukrop]

• In function 'external?', URI is now escaped before split.
• This adheres to the recommendation in the URI docs: https://www.rubydoc.info/stdlib/uri/URI.parse
• Resolves #33.

[mukrop]

Sorry it took so long, my past two weeks were also rather busy.

[keithmifsud]

Hi @mukrop ,

Thank for sharing your work :)

The travis checks are failing can you look into why, please?

Also, thank you for the version number :)

[mcrobs]

The problem is that URI escape is deprecated, I suggest to use CGI.escape() instead. They do NOT behave in the same way, but in this particular case it does not matter how the URI is escaped, because this is for comparison only

lib/jekyll-target-blank.rb:196:21: W: Lint/UriEscapeUnescape: URI.escape method is obsolete and should not be used. Instead, use CGI.escape, URI.encode_www_form or URI.encode_www_form_component depending on your specific use case. 459 URI.parse(URI.escape(link)).host != URI.parse(URI.escape(@site_url)).host 460 ^^^^^^^^^^^^^^^^ 461lib/jekyll-target-blank.rb:196:57: W: Lint/UriEscapeUnescape: URI.escape method is obsolete and should not be used. Instead, use CGI.escape, URI.encode_www_form or URI.encode_www_form_component depending on your specific use case. 462 URI.parse(URI.escape(link)).host != URI.parse(URI.escape(@site_url)).host 463 ^^^^^^^^^^^^^^^^^^^^^

[keithmifsud]

The problem is that URI escape is deprecated, I suggest to use CGI.escape() instead. They do NOT behave in the same way, but in this particular case it does not matter how the URI is escaped, because this is for comparison only

lib/jekyll-target-blank.rb:196:21: W: Lint/UriEscapeUnescape: URI.escape method is obsolete and should not be used. Instead, use CGI.escape, URI.encode_www_form or URI.encode_www_form_component depending on your specific use case. 459 URI.parse(URI.escape(link)).host != URI.parse(URI.escape(@site_url)).host 460 ^^^^^^^^^^^^^^^^ 461lib/jekyll-target-blank.rb:196:57: W: Lint/UriEscapeUnescape: URI.escape method is obsolete and should not be used. Instead, use CGI.escape, URI.encode_www_form or URI.encode_www_form_component depending on your specific use case. 462 URI.parse(URI.escape(link)).host != URI.parse(URI.escape(@site_url)).host 463 ^^^^^^^^^^^^^^^^^^^^^

Thank you @mcrobs .. sorry for the cheeky request but is any chance you can update this PR by @mukrop to use the other parser, please?

If not I'll try to get to it asap but I'm rather busy and away from a Ruby IDE at this moment.

[mukrop]

I do apologize for my inactivity, but I'm rather busy now. I'd be glad if you managed to resolve this without me.

[keithmifsud]

I understand and thank you for all your help in this 😄

[wbsmolen]

@mukrop @keithmifsud polite bump :)

[rafaelbiriba]

Any news on this one?

[mukrop]

Hi. I've looked into this after a longer while. In the end, I used the Addressable class for URI parsing as

1. URI is not capable to process non-ascii characters (see https://bugs.ruby-lang.org/issues/12852)
2. URI.escape is deprecated
3. Alternatives such as CGI.escape do a different thing (they escape the :// that separates the protocol and thus breaks the parsing).

The tests now pass but the build fails on not finding .rubocop_todo.yml (which I don't understand and seems unrelated).

[jochenjonc]

@mukrop can we borough a little bit of your time to see if #61 is the fix for #46? Maybe just approving 2 PRs and creating a release could help a lot of us.

[mukrop]

I'm off for the rest of the week but I believe I'll be able to find some time for this next week :-).

[mukrop]

Hi. I've tested the patch, but the issue is not fixed, the error seems the same.

/usr/lib/ruby/2.7.0/uri/rfc3986_parser.rb:21:in `split': URI must be ascii only (URI::InvalidURIError)

[jochenjonc]

Hi. I've tested the patch, but the issue is not fixed, the error seems the same.

/usr/lib/ruby/2.7.0/uri/rfc3986_parser.rb:21:in `split': URI must be ascii only (URI::InvalidURIError)

Could it be that the code or test isn't using the Addressable implementation of URI, but still the standard on of Ruby?

[mukrop]

Aah, sorry @jochenjonc, I misunderstood what I was supposed to check. I did it properly now and, indeed, #61 fixes the thing!

For testing purposes, I've added an extra commit to this PR replicating #61. For the sake of a clean codebase, I presume you want to merge #61 first and then merge this onto it without the replicating commit?

[jochenjonc]

@mukrop you can choose how to merge everything. None of the PRs are mine. 😉

[mukrop]

Oh, and you are not the repository owner either, I thought you were :-).

[jochenjonc]

Funny, I thought you where the repo owner. 🤔

[jochenjonc]

@keithmifsud are you able to merge this PR and create a new release?

[keithmifsud]

Hi @jochenjonc ..I'll merge it asap. I need to remove the Travis CI check first but they seem to have changed the access. I'll get back to you asap.

The PR looks good. Thank you :smile:

[jochenjonc]

@keithmifsud any luck on removing Travis CI check?