New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cgi/escape does not define CGI.escapeHTML and other methods #4513

Closed
jeremyevans opened this Issue Feb 27, 2017 · 8 comments

Comments

Projects
None yet
3 participants
@jeremyevans
Contributor

jeremyevans commented Feb 27, 2017

Environment

This is likely to be OS-independent, but:

$ jruby -v
jruby 9.1.7.0 (2.3.1) 2017-01-11 68056ae OpenJDK 64-Bit Server VM 25.112-b15 on 1.8.0_112-b15 +jit [OpenBSD-x86_64]
$ uname -a
OpenBSD testcurrent.bsa.ca.gov 6.0 GENERIC.MP#149 amd64

Expected/Actual Behavior

cgi/escape, in addition to prepending to CGI::Util, also extends CGI itself: https://github.com/ruby/ruby/blob/ruby_2_3/ext/cgi/escape/escape.c#L104

Example of differences:

$ ruby23 -r cgi/escape -ve "p CGI.escapeHTML('<')"
"&lt;"
$ jruby -r cgi/escape -e "p CGI.escapeHTML('<')"
NoMethodError: undefined method `escapeHTML' for CGI:Class
  method_missing at org/jruby/RubyBasicObject.java:1653
          <main> at -e:1

$ ruby23 -r cgi/escape -e "p CGI.singleton_class.ancestors"
[#<Class:CGI>, CGI::Escape, #<Class:Object>, #<Class:BasicObject>, Class, Module, Object, Kernel, BasicObject]
$ jruby -r cgi/escape -e "p CGI.singleton_class.ancestors"
[#<Class:CGI>, #<Class:Object>, #<Class:BasicObject>, Class, Module, Object, Kernel, BasicObject]
@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Feb 28, 2017

Member

The CGI module we use is unmodified from C Ruby. Perhaps this is a 2.4 method?

Member

headius commented Feb 28, 2017

The CGI module we use is unmodified from C Ruby. Perhaps this is a 2.4 method?

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Feb 28, 2017

Member

Oh nevermind, I see you tried Ruby 2.3. Strange that we don't have this. I think it might be a C ext they added and we never did.

Member

headius commented Feb 28, 2017

Oh nevermind, I see you tried Ruby 2.3. Strange that we don't have this. I think it might be a C ext they added and we never did.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Feb 28, 2017

Member

This was fixed in fd9515b and will be part of our 2.4-compatible release, but it could be included in 9.1.8.0. @enebo?

Member

headius commented Feb 28, 2017

This was fixed in fd9515b and will be part of our 2.4-compatible release, but it could be included in 9.1.8.0. @enebo?

@enebo

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Feb 28, 2017

Member

@headius yeah I think it is a good idea. My only question is will we add some 2.4ism if we cherry-pick this. Maybe it does not matter all that much since escapeHTML will be what people are trying to hit and if we provide anything extra likely no one will be hitting it (and our 2.4 release will be pretty soon).

Member

enebo commented Feb 28, 2017

@headius yeah I think it is a good idea. My only question is will we add some 2.4ism if we cherry-pick this. Maybe it does not matter all that much since escapeHTML will be what people are trying to hit and if we provide anything extra likely no one will be hitting it (and our 2.4 release will be pretty soon).

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Feb 28, 2017

Member

@enebo I think it's pretty safe. There's only a couple functions in here...only took a couple hours to port.

Member

headius commented Feb 28, 2017

@enebo I think it's pretty safe. There's only a couple functions in here...only took a couple hours to port.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Feb 28, 2017

Member

Actually I just realized this is already on master, but I think it's being wired up wrong.

Member

headius commented Feb 28, 2017

Actually I just realized this is already on master, but I think it's being wired up wrong.

@headius headius closed this in 9051c7a Feb 28, 2017

@headius headius added this to the JRuby 9.1.8.0 milestone Feb 28, 2017

@jeremyevans

This comment has been minimized.

Show comment
Hide comment
@jeremyevans

jeremyevans Mar 1, 2017

Contributor

Thanks for the quick fix. I just want to point out that cgi/escape is in ruby 2.3, not just in 2.4 (see link to ruby repo in the original post), so this should be backported to 9.1. Also, note that JRuby 9.1.7.0 already supported cgi/escape, it just didn't extend CGI with CGI::Escape.

Contributor

jeremyevans commented Mar 1, 2017

Thanks for the quick fix. I just want to point out that cgi/escape is in ruby 2.3, not just in 2.4 (see link to ruby repo in the original post), so this should be backported to 9.1. Also, note that JRuby 9.1.7.0 already supported cgi/escape, it just didn't extend CGI with CGI::Escape.

@enebo

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Mar 1, 2017

Member

@jeremyevans if you have the time to verify we landed all the missing bits or at least the bits you were trying to use (in this case we apparently already had them but just had them hooked up wrong).

Sorry for the confusion about 2.3 vs 2.4. I mostly meant if there was any change to cgi/escape in 2.4 and we ported for 2.4 something 2.4ish would come back to 2.3. In any case, we did not have to contemplate the cherry-pick so it is all good.

Member

enebo commented Mar 1, 2017

@jeremyevans if you have the time to verify we landed all the missing bits or at least the bits you were trying to use (in this case we apparently already had them but just had them hooked up wrong).

Sorry for the confusion about 2.3 vs 2.4. I mostly meant if there was any change to cgi/escape in 2.4 and we ported for 2.4 something 2.4ish would come back to 2.3. In any case, we did not have to contemplate the cherry-pick so it is all good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment