Skip to content

Commit

Permalink
For performance reasons, you can no longer call html_safe! on Strings…
Browse files Browse the repository at this point in the history
…. 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 authored and Yehuda Katz committed Feb 1, 2010
1 parent 1c83fd2 commit 4cbb9db
Show file tree
Hide file tree
Showing 31 changed files with 178 additions and 195 deletions.
4 changes: 3 additions & 1 deletion actionpack/lib/action_view.rb
Expand Up @@ -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"
2 changes: 1 addition & 1 deletion actionpack/lib/action_view/base.rb
Expand Up @@ -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
Expand Down
49 changes: 0 additions & 49 deletions actionpack/lib/action_view/erb/util.rb

This file was deleted.

11 changes: 5 additions & 6 deletions actionpack/lib/action_view/helpers/active_model_helper.rb
Expand Up @@ -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

Expand Down Expand Up @@ -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.
Expand Down
6 changes: 3 additions & 3 deletions actionpack/lib/action_view/helpers/asset_tag_helper.rb
Expand Up @@ -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

This comment has been minimized.

Copy link
@german

german Oct 18, 2010

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

Expand Down Expand Up @@ -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

Expand Down Expand Up @@ -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)
Expand Down
14 changes: 7 additions & 7 deletions actionpack/lib/action_view/helpers/date_helper.rb
Expand Up @@ -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

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down
4 changes: 2 additions & 2 deletions actionpack/lib/action_view/helpers/debug_helper.rb
Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions actionpack/lib/action_view/helpers/form_helper.rb
Expand Up @@ -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:
Expand Down Expand Up @@ -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 = {})
Expand Down
2 changes: 1 addition & 1 deletion actionpack/lib/action_view/helpers/form_options_helper.rb
Expand Up @@ -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
Expand Down
6 changes: 3 additions & 3 deletions actionpack/lib/action_view/helpers/form_tag_helper.rb
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion actionpack/lib/action_view/helpers/number_helper.rb
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion actionpack/lib/action_view/helpers/prototype_helper.rb
Expand Up @@ -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>
Expand Down
2 changes: 1 addition & 1 deletion actionpack/lib/action_view/helpers/raw_output_helper.rb
Expand Up @@ -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
Expand Down
12 changes: 2 additions & 10 deletions actionpack/lib/action_view/helpers/sanitize_helper.rb
Expand Up @@ -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.
Expand All @@ -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.
Expand Down
8 changes: 4 additions & 4 deletions actionpack/lib/action_view/helpers/tag_helper.rb
Expand Up @@ -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
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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)
Expand All @@ -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
Expand Down
10 changes: 5 additions & 5 deletions actionpack/lib/action_view/helpers/text_helper.rb
Expand Up @@ -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>.
Expand Down
2 changes: 1 addition & 1 deletion actionpack/lib/action_view/helpers/translation_helper.rb
Expand Up @@ -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')
Expand Down
8 changes: 4 additions & 4 deletions actionpack/lib/action_view/helpers/url_helper.rb
Expand Up @@ -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
Expand Down Expand Up @@ -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] || {}
Expand All @@ -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

Expand Down Expand Up @@ -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


Expand Down

5 comments on commit 4cbb9db

@TomK32
Copy link
Contributor

@TomK32 TomK32 commented on 4cbb9db Feb 1, 2010

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine, breaks HAML (edge) :/

@marcolz
Copy link

@marcolz marcolz commented on 4cbb9db Feb 1, 2010

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, here too.

@martinrehfeld
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@notlaforge
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@martinrehfeld
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.