EscapeUtils.escape_html_as_html_safe #30

Merged
merged 17 commits into from Feb 28, 2013

Conversation

Projects
None yet
4 participants
Collaborator

tmm1 commented Feb 27, 2013

Returns a custom subclass of String and optionally sets @html_safe=true in the new instance.

Owner

brianmario commented Feb 27, 2013

I kinda like the class ivar thing we talked about earlier where we can do the type check once up front. I'd feel a lot safer using the klass value then.

Collaborator

tmm1 commented Feb 27, 2013

Yea, maybe something like

EscapeUtils.html_string_class = ActiveSupport::SafeBuffer

That method can just raise if you try to use a non-class or non-subclass of String.

@brianmario brianmario Move html_string_class into a global
This is so we can perform some checks on the value passed in ahead
of time
774d9f0

Use rb_obj_is_kind_of? More elegant check.

Owner

brianmario replied Feb 28, 2013

The type of this object is a T_CLASS though, not an instance of another class right?

Collaborator

vmg replied Feb 28, 2013

You're damn right. Disregard that I suck. :p

I'd drop this. No need to cache an rb_intern call that we're only doing once.

I'd drop this. Do an attribute_accessor on Rubyland. Prettier.

And rb_cvar_set(self, rb_intern("@@string_class"), val);

This is very arbitrary... I'm not too sure on how to work around it, but looks inelegant. Do we always need to set the ivar for Rails' SafeString?

Owner

brianmario replied Feb 28, 2013

Unfortunately yeah, for the case of using ActiveSupport::SafeBuffer or whatever, it's assumed that @html_safe is already set to true.

@tmm1 tmm1 and 1 other commented on an outdated diff Feb 28, 2013

ext/escape_utils/escape_utils.c
+ int secure = g_html_secure;
+ gh_buf buf = GH_BUF_INIT;
+
+ Check_Type(str, T_STRING);
+ check_utf8_encoding(str);
+
+ if (houdini_escape_html0(&buf, (const uint8_t *)RSTRING_PTR(str), RSTRING_LEN(str), secure)) {
+ result = eu_new_str(buf.ptr, buf.size);
+ gh_buf_free(&buf);
+ } else {
+ result = rb_str_dup(str);
+ }
+
+ RBASIC(result)->klass = rb_html_string_class;
+ if (RTEST(set_ivar))
+ rb_ivar_set(result, ID_at_html_safe, Qtrue);
@tmm1

tmm1 Feb 28, 2013

Collaborator

Let's just get rid of the optional argument and always set @html_safe=true.

@brianmario

brianmario Feb 28, 2013

Owner

I'm down with that. After all, it is actually html safe when we're done with it 🎉

Collaborator

tmm1 commented Feb 28, 2013

Added a fallback path for rbx. Although, it looks like the build is still failing because of rb_cvar_set. Can't we use a simple ivar instead?

Owner

brianmario commented Feb 28, 2013

Yeah I guess we could just set a regular ivar on the class itself?

@brianmario brianmario added a commit that referenced this pull request Feb 28, 2013

@brianmario brianmario Merge pull request #30 from brianmario/as_html_safe
EscapeUtils.escape_html_as_html_safe
c8130d4

@brianmario brianmario merged commit c8130d4 into master Feb 28, 2013

1 check passed

default The Travis build passed
Details

brianmario deleted the as_html_safe branch Feb 28, 2013

Hello. I encountered an issue concerning escape_util's CGI monkeypatch and the cocoon gem. The issue involves converting an ActiveSupport::SafeBuffer to a string in order to escape it properly. Please see nathanvda/cocoon#191. Would setting something like EscapeUtils.html_string_class = ActiveSupport::SafeBuffer resolve this issue? Thanks for the help!

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