Fix underscore can't work with html safe string

[janx]

In #underscore gsub! is used to modify string, when the string is a SafeBuffer instance an exception will be raised. It cause errors when you use radio_button with translated string:

ActionView::Template::Error (Cannot modify SafeBuffer in place):
    25:     <%= f.radio_button :signup_type, t('site.account_signup_close_label') %>

[pixeltrix]

What's the backtrace for your error? Trying to track down where the SafeBuffer is created and what's doing the underscore.

[janx]

It turns out the SafeBuffer is returned by a modified (in our project) version of I18n.t. Please close this pull request, sorry for the noise :-)

[janx]

This fix may still be necessary because today I found I18n.t itself can return SafeBuffer - if your key is ended with _html.

[pixeltrix]

If your string really does contain HTML then you shouldn't be calling things like underscore, camelize, etc. since those may alter the HTML code. If somewhere in the Rails code those methods are being called (e.g humanize on a string in the label form helper) then those method need to be fixed not underscore itself.

[janx]

I don't think whether the passed in argument is HTML should be underscore's concern. It should work for all string-like objects. If you check all methods in activesupport/lib/active_support/inflector/methods.rb, you'll find underscore is the only one use gsub! and incompatible with SafeBuffer.

[pixeltrix]

The reason for the _html feature in I18n is specifically so that HTML code can be localized - if there's no HTML don't use _html for a key. You're correct in saying that underscore shouldn't have to worry about whether the argument is HTML or not and that's the root of the problem - should SafeBuffer be like a String or not, i.e. should we use inheritance or composition.

With the benefit of hindsight it'd probably been better if was clear that SafeBuffer is not a string - once you've modified it with anything like gsub then you have to treat it as dirty and escape the whole string again which is likely to lead to unexpected double escaping bugs. The most you should be able to do with a SafeBuffer is concat together with another String (in which case the String is escaped) or with another SafeBuffer (in which case no extra escaping is performed). I'd probably go so far as to add a warning similar to the warning raised when a mass-assignment variable is ignored when a SafeBuffer is converted to a string.

I think the method name of html_safe has partly led to the confusion over how SafeBuffer is meant to be used - people have been using to say "this string doesn't contain anything dangerous" where what it really means is "this string may contain HTML code that's already been escaped, don't escape it again". This has led to people using it pre-emptively in their helpers - the only time you should be calling html_safe in your helpers is if you're building chunks of HTML. Even then you probably don't need to call html_safe if you're using tag, content_tag, etc.

[janx]

I agree. But I'm afraid people will eventually stumble upon here. SafeBuffer is used more and more in applications, plugins and rails itself, sometimes application programmer can't control the thing he get is a String or SafeBuffer.