Permalink
Browse files

For performance reasons, you can no longer call html_safe! on Strings…

…. Instead, all Strings are always not html_safe?. Instead, you can get a SafeBuffer from a String by calling #html_safe, which will SafeBuffer.new(self).

  * Additionally, instead of doing concat("</form>".html_safe), you can do
    safe_concat("</form>"), which will skip both the flag set, and the flag
    check.
  * For the first pass, I converted virtually all #html_safe!s to #html_safe,
    and the tests pass. A further optimization would be to try to use
    #safe_concat as much as possible, reducing the performance impact if
    we know up front that a String is safe.
  • Loading branch information...
Yehuda Katz Yehuda Katz
Yehuda Katz authored and Yehuda Katz committed Feb 1, 2010
1 parent 1c83fd2 commit 4cbb9db0a5ff65b0a626a5b043331abefd89e717
Showing with 178 additions and 195 deletions.
  1. +3 −1 actionpack/lib/action_view.rb
  2. +1 −1 actionpack/lib/action_view/base.rb
  3. +0 −49 actionpack/lib/action_view/erb/util.rb
  4. +5 −6 actionpack/lib/action_view/helpers/active_model_helper.rb
  5. +3 −3 actionpack/lib/action_view/helpers/asset_tag_helper.rb
  6. +7 −7 actionpack/lib/action_view/helpers/date_helper.rb
  7. +2 −2 actionpack/lib/action_view/helpers/debug_helper.rb
  8. +2 −2 actionpack/lib/action_view/helpers/form_helper.rb
  9. +1 −1 actionpack/lib/action_view/helpers/form_options_helper.rb
  10. +3 −3 actionpack/lib/action_view/helpers/form_tag_helper.rb
  11. +1 −1 actionpack/lib/action_view/helpers/number_helper.rb
  12. +1 −1 actionpack/lib/action_view/helpers/prototype_helper.rb
  13. +1 −1 actionpack/lib/action_view/helpers/raw_output_helper.rb
  14. +2 −10 actionpack/lib/action_view/helpers/sanitize_helper.rb
  15. +4 −4 actionpack/lib/action_view/helpers/tag_helper.rb
  16. +5 −5 actionpack/lib/action_view/helpers/text_helper.rb
  17. +1 −1 actionpack/lib/action_view/helpers/translation_helper.rb
  18. +4 −4 actionpack/lib/action_view/helpers/url_helper.rb
  19. +1 −1 actionpack/lib/action_view/render/partials.rb
  20. +0 −27 actionpack/lib/action_view/safe_buffer.rb
  21. +1 −1 actionpack/lib/action_view/test_case.rb
  22. +2 −2 actionpack/test/controller/caching_test.rb
  23. +1 −1 actionpack/test/controller/output_escaping_test.rb
  24. +1 −1 actionpack/test/template/erb_util_test.rb
  25. +1 −1 actionpack/test/template/form_helper_test.rb
  26. +3 −3 actionpack/test/template/form_tag_helper_test.rb
  27. +1 −1 actionpack/test/template/safe_buffer_test.rb
  28. +1 −1 actionpack/test/template/test_case_test.rb
  29. +1 −0 activesupport/lib/active_support.rb
  30. +85 −21 activesupport/lib/active_support/core_ext/string/output_safety.rb
  31. +34 −33 activesupport/test/core_ext/string_ext_test.rb
@@ -56,7 +56,9 @@ module ActionView
autoload :TestCase, 'action_view/test_case'
end
-require 'action_view/erb/util'
+require 'active_support/core_ext/string/output_safety'
require 'action_view/base'
+ActionView::SafeBuffer = ActiveSupport::SafeBuffer
+
I18n.load_path << "#{File.dirname(__FILE__)}/action_view/locale/en.yml"
@@ -284,7 +284,7 @@ def initialize(view_paths = [], assigns_for_first_render = {}, controller = nil,
@helpers = self.class.helpers || Module.new
@_controller = controller
- @_content_for = Hash.new {|h,k| h[k] = ActionView::SafeBuffer.new }
+ @_content_for = Hash.new {|h,k| h[k] = ActiveSupport::SafeBuffer.new }
@_virtual_path = nil
self.view_paths = view_paths
end
@@ -1,49 +0,0 @@
-require 'erb'
-
-class ERB
- module Util
- HTML_ESCAPE = { '&' => '&amp;', '>' => '&gt;', '<' => '&lt;', '"' => '&quot;' }
- JSON_ESCAPE = { '&' => '\u0026', '>' => '\u003E', '<' => '\u003C' }
-
- # A utility method for escaping HTML tag characters.
- # This method is also aliased as <tt>h</tt>.
- #
- # In your ERb templates, use this method to escape any unsafe content. For example:
- # <%=h @person.name %>
- #
- # ==== Example:
- # puts html_escape("is a > 0 & a < 10?")
- # # => is a &gt; 0 &amp; a &lt; 10?
- def html_escape(s)
- s = s.to_s
- if s.html_safe?
- s
- else
- s.gsub(/[&"><]/) { |special| HTML_ESCAPE[special] }.html_safe!
- end
- end
-
- undef :h
- alias h html_escape
-
- module_function :html_escape
- module_function :h
-
- # A utility method for escaping HTML entities in JSON strings.
- # This method is also aliased as <tt>j</tt>.
- #
- # In your ERb templates, use this method to escape any HTML entities:
- # <%=j @person.to_json %>
- #
- # ==== Example:
- # puts json_escape("is a > 0 & a < 10?")
- # # => is a \u003E 0 \u0026 a \u003C 10?
- def json_escape(s)
- s.to_s.gsub(/[&"><]/) { |special| JSON_ESCAPE[special] }
- end
-
- alias j json_escape
- module_function :j
- module_function :json_escape
- end
-end
@@ -6,7 +6,7 @@
module ActionView
class Base
- @@field_error_proc = Proc.new{ |html_tag, instance| "<div class=\"fieldWithErrors\">#{html_tag}</div>".html_safe! }
+ @@field_error_proc = Proc.new{ |html_tag, instance| "<div class=\"fieldWithErrors\">#{html_tag}</div>".html_safe }
cattr_accessor :field_error_proc
end
@@ -86,12 +86,11 @@ def form(record_name, options = {})
submit_value = options[:submit_value] || options[:action].gsub(/[^\w]/, '').capitalize
contents = form_tag({:action => action}, :method =>(options[:method] || 'post'), :enctype => options[:multipart] ? 'multipart/form-data': nil)
- contents << hidden_field(record_name, :id) unless record.new_record?
- contents << all_input_tags(record, record_name, options)
+ contents.safe_concat hidden_field(record_name, :id) unless record.new_record?
+ contents.safe_concat all_input_tags(record, record_name, options)
yield contents if block_given?
- contents << submit_tag(submit_value)
- contents << '</form>'
- contents.html_safe!
+ contents.safe_concat submit_tag(submit_value)
+ contents.safe_concat('</form>')
end
# Returns a string containing the error message attached to the +method+ on the +object+ if one exists.
@@ -293,7 +293,7 @@ def javascript_include_tag(*sources)
else
sources = expand_javascript_sources(sources, recursive)
ensure_javascript_sources!(sources) if cache
- sources.collect { |source| javascript_src_tag(source, options) }.join("\n").html_safe!
+ sources.collect { |source| javascript_src_tag(source, options) }.join("\n").html_safe
@german

german Oct 18, 2010

Contributor

Sorry, why there should be 'html_safe' on javascript_src_tag?

Because when I write:

<%= javascript_include_tag "prototype-base-extensions", "prototype-date-extensions", "datepicker" %>

I constantly get this in my HTML:

<script src="/javascripts/prototype-base-extensions.js?1286905348" type="text/javascript"></script>
<script src="/javascripts/prototype-date-extensions.js?1286905348" type="text/javascript"></script>
<script src="/javascripts/datepicker.js?1286905348" type="text/javascript"></script>

so I should do something like this:

<%= raw(javascript_include_tag "prototype-base-extensions", "prototype-date-extensions", "datepicker") %>

I use rails3.0.0 / ruby 1.8.7 (2010-04-19 patchlevel 253) [i686-linux], MBARI 0x8770, Ruby Enterprise Edition 2010.02

Am I the only one who encountered this issue too?

end
end
@@ -444,7 +444,7 @@ def stylesheet_link_tag(*sources)
else
sources = expand_stylesheet_sources(sources, recursive)
ensure_stylesheet_sources!(sources) if cache
- sources.collect { |source| stylesheet_tag(source, options) }.join("\n").html_safe!
+ sources.collect { |source| stylesheet_tag(source, options) }.join("\n").html_safe
end
end
@@ -588,7 +588,7 @@ def video_tag(sources, options = {})
if sources.is_a?(Array)
content_tag("video", options) do
- sources.map { |source| tag("source", :src => source) }.join.html_safe!
+ sources.map { |source| tag("source", :src => source) }.join.html_safe
end
else
options[:src] = path_to_video(sources)
@@ -616,7 +616,7 @@ def select_datetime
build_selects_from_types(order)
else
- "#{select_date}#{@options[:datetime_separator]}#{select_time}".html_safe!
+ "#{select_date}#{@options[:datetime_separator]}#{select_time}".html_safe
end
end
@@ -835,7 +835,7 @@ def build_select(type, select_options_as_html)
select_html << prompt_option_tag(type, @options[:prompt]) + "\n" if @options[:prompt]
select_html << select_options_as_html.to_s
- (content_tag(:select, select_html, select_options) + "\n").html_safe!
+ (content_tag(:select, select_html, select_options) + "\n").html_safe
end
# Builds a prompt option tag with supplied options or from default options
@@ -865,7 +865,7 @@ def build_hidden(type, value)
:id => input_id_from_type(type),
:name => input_name_from_type(type),
:value => value
- }) + "\n").html_safe!
+ }) + "\n").html_safe
end
# Returns the name attribute for the input tag
@@ -896,7 +896,7 @@ def build_selects_from_types(order)
separator = separator(type) unless type == order.first # don't add on last field
select.insert(0, separator.to_s + send("select_#{type}").to_s)
end
- select.html_safe!
+ select.html_safe
end
# Returns the separator for a given datetime component
@@ -916,15 +916,15 @@ def separator(type)
class InstanceTag #:nodoc:
def to_date_select_tag(options = {}, html_options = {})
- datetime_selector(options, html_options).select_date.html_safe!
+ datetime_selector(options, html_options).select_date.html_safe
end
def to_time_select_tag(options = {}, html_options = {})
- datetime_selector(options, html_options).select_time.html_safe!
+ datetime_selector(options, html_options).select_time.html_safe
end
def to_datetime_select_tag(options = {}, html_options = {})
- datetime_selector(options, html_options).select_datetime.html_safe!
+ datetime_selector(options, html_options).select_datetime.html_safe
end
private
@@ -27,10 +27,10 @@ module DebugHelper
def debug(object)
begin
Marshal::dump(object)
- "<pre class='debug_dump'>#{h(object.to_yaml).gsub(" ", "&nbsp; ")}</pre>".html_safe!
+ "<pre class='debug_dump'>#{h(object.to_yaml).gsub(" ", "&nbsp; ")}</pre>".html_safe
rescue Exception => e # errors from Marshal or YAML
# Object couldn't be dumped, perhaps because of singleton methods -- this is the fallback
- "<code class='debug_dump'>#{h(object.inspect)}</code>".html_safe!
+ "<code class='debug_dump'>#{h(object.inspect)}</code>".html_safe
end
end
end
@@ -311,7 +311,7 @@ def form_for(record_or_name_or_array, *args, &proc)
concat(form_tag(options.delete(:url) || {}, options.delete(:html) || {}))
fields_for(object_name, *(args << options), &proc)
- concat('</form>'.html_safe!)
+ safe_concat('</form>')
end
def apply_form_for_options!(object_or_array, options) #:nodoc:
@@ -879,7 +879,7 @@ def to_check_box_tag(options = {}, checked_value = "1", unchecked_value = "0")
end
hidden = tag("input", "name" => options["name"], "type" => "hidden", "value" => options['disabled'] && checked ? checked_value : unchecked_value)
checkbox = tag("input", options)
- (hidden + checkbox).html_safe!
+ (hidden + checkbox).html_safe
end
def to_boolean_select_tag(options = {})
@@ -296,7 +296,7 @@ def options_for_select(container, selected = nil)
options << %(<option value="#{html_escape(value.to_s)}"#{selected_attribute}#{disabled_attribute}>#{html_escape(text.to_s)}</option>)
end
- options_for_select.join("\n").html_safe!
+ options_for_select.join("\n").html_safe
end
# Returns a string of option tags that have been compiled by iterating over the +collection+ and assigning the
@@ -446,7 +446,7 @@ def field_set_tag(legend = nil, options = nil, &block)
concat(tag(:fieldset, options, true))
concat(content_tag(:legend, legend)) unless legend.blank?
concat(content)
- concat("</fieldset>".html_safe!)
+ safe_concat("</fieldset>")
end
private
@@ -474,14 +474,14 @@ def extra_tags_for_form(html_options)
def form_tag_html(html_options)
extra_tags = extra_tags_for_form(html_options)
- (tag(:form, html_options, true) + extra_tags).html_safe!
+ (tag(:form, html_options, true) + extra_tags).html_safe
end
def form_tag_in_block(html_options, &block)
content = capture(&block)
concat(form_tag_html(html_options))
concat(content)
- concat("</form>".html_safe!)
+ safe_concat("</form>")
end
def token_tag
@@ -92,7 +92,7 @@ def number_to_currency(number, options = {})
:precision => precision,
:delimiter => delimiter,
:separator => separator)
- ).gsub(/%u/, unit).html_safe!
+ ).gsub(/%u/, unit).html_safe
rescue
number
end
@@ -610,7 +610,7 @@ def method_missing(method, *arguments)
# page.hide 'spinner'
# end
def update_page(&block)
- JavaScriptGenerator.new(@template, &block).to_s.html_safe!
+ JavaScriptGenerator.new(@template, &block).to_s.html_safe
end
# Works like update_page but wraps the generated JavaScript in a <script>
@@ -2,7 +2,7 @@ module ActionView #:nodoc:
module Helpers #:nodoc:
module RawOutputHelper
def raw(stringish)
- stringish.to_s.html_safe!
+ stringish.to_s.html_safe
end
end
end
@@ -50,11 +50,7 @@ module SanitizeHelper
# confuse browsers.
#
def sanitize(html, options = {})
- returning self.class.white_list_sanitizer.sanitize(html, options) do |sanitized|
- if sanitized
- sanitized.html_safe!
- end
- end
+ self.class.white_list_sanitizer.sanitize(html, options).try(:html_safe)
end
# Sanitizes a block of CSS code. Used by +sanitize+ when it comes across a style attribute.
@@ -77,11 +73,7 @@ def sanitize_css(style)
# strip_tags("<div id='top-bar'>Welcome to my website!</div>")
# # => Welcome to my website!
def strip_tags(html)
- returning self.class.full_sanitizer.sanitize(html) do |sanitized|
- if sanitized
- sanitized.html_safe!
- end
- end
+ self.class.full_sanitizer.sanitize(html).try(:html_safe)
end
# Strips all link tags from +text+ leaving just the link text.
@@ -41,7 +41,7 @@ module TagHelper
# tag("img", { :src => "open &amp; shut.png" }, false, false)
# # => <img src="open &amp; shut.png" />
def tag(name, options = nil, open = false, escape = true)
- "<#{name}#{tag_options(options, escape) if options}#{open ? ">" : " />"}".html_safe!
+ "<#{name}#{tag_options(options, escape) if options}#{open ? ">" : " />"}".html_safe
end
# Returns an HTML block tag of type +name+ surrounding the +content+. Add
@@ -94,7 +94,7 @@ def content_tag(name, content_or_options_with_block = nil, options = nil, escape
# cdata_section(File.read("hello_world.txt"))
# # => <![CDATA[<hello from a text file]]>
def cdata_section(content)
- "<![CDATA[#{content}]]>".html_safe!
+ "<![CDATA[#{content}]]>".html_safe
end
# Returns an escaped version of +html+ without affecting existing escaped entities.
@@ -128,7 +128,7 @@ def block_called_from_erb?(block)
def content_tag_string(name, content, options, escape = true)
tag_options = tag_options(options, escape) if options
- "<#{name}#{tag_options}>#{content}</#{name}>".html_safe!
+ "<#{name}#{tag_options}>#{content}</#{name}>".html_safe
end
def tag_options(options, escape = true)
@@ -143,7 +143,7 @@ def tag_options(options, escape = true)
attrs << %(#{key}="#{final_value}")
end
end
- " #{attrs.sort * ' '}".html_safe! unless attrs.empty?
+ " #{attrs.sort * ' '}".html_safe unless attrs.empty?
end
end
end
@@ -24,14 +24,14 @@ module TextHelper
# end
# # will either display "Logged in!" or a login link
# %>
- def concat(string, unused_binding = nil)
- if unused_binding
- ActiveSupport::Deprecation.warn("The binding argument of #concat is no longer needed. Please remove it from your views and helpers.", caller)
- end
-
+ def concat(string)
output_buffer << string
end
+ def safe_concat(string)
+ output_buffer.safe_concat(string)
+ end
+
# Truncates a given +text+ after a given <tt>:length</tt> if +text+ is longer than <tt>:length</tt>
# (defaults to 30). The last characters will be replaced with the <tt>:omission</tt> (defaults to "...")
# for a total length not exceeding <tt>:length</tt>.
@@ -12,7 +12,7 @@ module TranslationHelper
# prepend the key with a period, nothing is converted.
def translate(key, options = {})
options[:raise] = true
- I18n.translate(scope_key_by_partial(key), options).html_safe!
+ I18n.translate(scope_key_by_partial(key), options).html_safe
rescue I18n::MissingTranslationData => e
keys = I18n.send(:normalize_translation_keys, e.locale, e.key, e.options[:scope])
content_tag('span', keys.join(', '), :class => 'translation_missing')
@@ -98,7 +98,7 @@ def url_for(options = {})
polymorphic_path(options)
end
- escape ? escape_once(url).html_safe! : url
+ escape ? escape_once(url).html_safe : url
end
# Creates a link tag of the given +name+ using a URL created by the set
@@ -208,7 +208,7 @@ def link_to(*args, &block)
if block_given?
options = args.first || {}
html_options = args.second
- concat(link_to(capture(&block), options, html_options).html_safe!)
+ safe_concat(link_to(capture(&block), options, html_options))
else
name = args[0]
options = args[1] || {}
@@ -226,7 +226,7 @@ def link_to(*args, &block)
end
href_attr = "href=\"#{url}\"" unless href
- "<a #{href_attr}#{tag_options}>#{ERB::Util.h(name || url)}</a>".html_safe!
+ "<a #{href_attr}#{tag_options}>#{ERB::Util.h(name || url)}</a>".html_safe
end
end
@@ -312,7 +312,7 @@ def button_to(name, options = {}, html_options = {})
html_options.merge!("type" => "submit", "value" => name)
("<form method=\"#{form_method}\" action=\"#{escape_once url}\" #{"data-remote=\"true\"" if remote} class=\"button-to\"><div>" +
- method_tag + tag("input", html_options) + request_token_tag + "</div></form>").html_safe!
+ method_tag + tag("input", html_options) + request_token_tag + "</div></form>").html_safe
end
Oops, something went wrong.

5 comments on commit 4cbb9db

Contributor

TomK32 replied Feb 1, 2010

Fine, breaks HAML (edge) :/

Yep, here too.

Contributor

martinrehfeld replied Feb 5, 2010

Jesus, another yak queuing up for being shaved :-(

Contributor

notlaforge replied Feb 5, 2010

This is fantastic! Making all String be html_safe? by default caused operations like + and << be dog slow, even on String literals.

Contributor

martinrehfeld replied Feb 5, 2010

As it turns out the Haml guys have already done their homework and 2.2.18 is fine with the new model. Definitely a nice improvement in speed!

Please sign in to comment.