Skip to content

url_encode: use CGI.escapeURIComponent - #23

Merged
k0kubun merged 1 commit into
masterfrom
cgi-escape-url
Oct 25, 2022
Merged

url_encode: use CGI.escapeURIComponent#23
k0kubun merged 1 commit into
masterfrom
cgi-escape-url

Conversation

@byroot

Copy link
Copy Markdown
Member

Ref: ruby/cgi#26

This native implementation is much faster and available in cgi 0.3.3.

@byroot
byroot requested a review from k0kubunOctober 25, 2022 11:09
@byroot

Copy link
Copy Markdown
MemberAuthor

I'm not too sure why 2.5 CI is failing, it's like it's not loading cgi as a gem but using the stdlib version?

I can add a conditional to handle this case. Let me know what you think @k0kubun.

@k0kubun

Copy link
Copy Markdown
Member

it's like it's not loading cgi as a gem but using the stdlib version?

stdlib could be updated only if the Ruby version has it as a default gem instead of a non-gem standard library. CGI has been promoted to a default gem on 2.7 https://github.com/ruby/ruby/blob/v2_7_0/NEWS. Looks like 2.6 is not tested in the workflow (a separate problem), but it wouldn't work in Ruby 2.6 either.

Given that Ruby 2.6 is EOL, I'm okay with requiring Ruby 2.7+ from the next ERB version. Can you do it in this PR to fix the CI?

Ref: ruby/cgi#26
This native implementation is much faster
and available in `cgi 0.3.3`.
@byroot

Copy link
Copy Markdown
MemberAuthor

Done.

@k0kubun
k0kubun merged commit 2d90e9b into masterOct 25, 2022
@k0kubun
k0kubun deleted the cgi-escape-url branch October 25, 2022 16:39
matzbot pushed a commit to ruby/ruby that referenced this pull request Oct 25, 2022
(ruby/erb#23)
Ref: ruby/cgi#26
This native implementation is much faster
and available in `cgi 0.3.3`.
ruby/erb@2d90e9b010
tenderlove pushed a commit to Shopify/ruby that referenced this pull request Oct 27, 2022
(ruby/erb#23)
Ref: ruby/cgi#26
This native implementation is much faster
and available in `cgi 0.3.3`.
ruby/erb@2d90e9b010
@k0kubunk0kubun mentioned this pull request May 13, 2025
Sign up for freeto 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.

2 participants

@byroot@k0kubun