-
-
Notifications
You must be signed in to change notification settings - Fork 925
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
Comments
The CGI module we use is unmodified from C Ruby. Perhaps this is a 2.4 method? |
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 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). |
@enebo I think it's pretty safe. There's only a couple functions in here...only took a couple hours to port. |
Actually I just realized this is already on master, but I think it's being wired up wrong. |
Thanks for the quick fix. I just want to point out that |
@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. |
Environment
This is likely to be OS-independent, but:
Expected/Actual Behavior
cgi/escape
, in addition to prepending toCGI::Util
, also extendsCGI
itself: https://github.com/ruby/ruby/blob/ruby_2_3/ext/cgi/escape/escape.c#L104Example of differences:
The text was updated successfully, but these errors were encountered: