Feature/simple format refactoring #6273

Merged
merged 2 commits into from May 15, 2012

4 participants

@KensoDev

Refactored simple_format into another method called paragraph_split

With the paragraph_split method, you are now able to split paragraphs and wrap those in any tag you may want without confining yourself to the <p> tag.

@smui

👍 +1

@rafaelfranca rafaelfranca and 1 other commented on an outdated diff May 12, 2012
actionpack/lib/action_view/helpers/text_helper.rb
@@ -259,16 +259,47 @@ def word_wrap(text, *args)
# simple_format("<span>I'm allowed!</span> It's true.", {}, :sanitize => false)
# # => "<p><span>I'm allowed!</span> It's true.</p>"
def simple_format(text, html_options={}, options={})
- text = '' if text.nil?
- text = text.dup
- start_tag = tag('p', html_options, true)
- text = sanitize(text) unless options[:sanitize] == false
- text = text.to_str
- text.gsub!(/\r\n?/, "\n") # \r\n and \r -> \n
- text.gsub!(/\n\n+/, "</p>\n\n#{start_tag}") # 2+ newline -> paragraph
- text.gsub!(/([^\n]\n)(?=[^\n])/, '\1<br />') # 1 newline -> br
- text.insert 0, start_tag
- text.html_safe.safe_concat("</p>")
+ paragraphs = paragraph_split(text)
+
+ if paragraphs.empty?
+ "<p></p>".html_safe
@rafaelfranca
Ruby on Rails member

we can use the content_tag helper here

@rafaelfranca
Ruby on Rails member

Also even if the paragraphs are empty we need to pass the html_options

makes sense. applying your comment.

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

@rafaelfranca I updated the pull request with the fix.

@rafaelfranca rafaelfranca and 1 other commented on an outdated diff May 12, 2012
actionpack/lib/action_view/helpers/text_helper.rb
@@ -259,16 +259,47 @@ def word_wrap(text, *args)
# simple_format("<span>I'm allowed!</span> It's true.", {}, :sanitize => false)
# # => "<p><span>I'm allowed!</span> It's true.</p>"
def simple_format(text, html_options={}, options={})
- text = '' if text.nil?
- text = text.dup
- start_tag = tag('p', html_options, true)
- text = sanitize(text) unless options[:sanitize] == false
- text = text.to_str
- text.gsub!(/\r\n?/, "\n") # \r\n and \r -> \n
- text.gsub!(/\n\n+/, "</p>\n\n#{start_tag}") # 2+ newline -> paragraph
- text.gsub!(/([^\n]\n)(?=[^\n])/, '\1<br />') # 1 newline -> br
- text.insert 0, start_tag
- text.html_safe.safe_concat("</p>")
+ paragraphs = paragraph_split(text)
+
+ if paragraphs.empty?
+ content_tag('p', '', html_options).html_safe
@rafaelfranca
Ruby on Rails member

content_tag always returns a html safe string by default. You don't need to call it again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@rafaelfranca
Ruby on Rails member

I added more comments inline.

What do you thing about instead of create another public method, make paragraph_split private and simple_format receives a tag option?

By default the tag will be :p by we can pass another tag.

Eg.:

simple_format(text, {}, :tag => :div)
@KensoDev

@rafaelfranca I had a pull request like that but @tenderlove said its too dangerous.
That's why I added paragraph split as public since you can basically wrap with any tag you want just by iterating over the array

@rafaelfranca
Ruby on Rails member

I read the discussion and since we moved the internals to use always content_tag so I think that now will be safer.

@tenderlove was concerned in that pull request because you are using string concatenation.

@tenderlove am I right?

@KensoDev

If that's acceptable, I have no problem adding a :wrapper_tag option, that was the original trigger for my pull request.

@KensoDev

@rafaelfranca I took the time and tested your assumption and it turns out that rails will trust anything you pass as the name into content_tag.

For example:

>> helper.content_tag("/div><script>alert('ZOMG')</script>")
=> "</div><script>alert('ZOMG')</script>><//div><script>alert('ZOMG')</script>>"

I made a commit here:
KensoDev@ad2a80e

This is a first attempt to attack the issue on securing the tag, I am not sure if this is viable or not, or if we want to go down that path.

If we secure this input, we can make the simple_format function accept any input to the :wrapper_tag option I wanted to add.

What do you think?

cc/ @tenderlove

@DouweM

@KensoDev I might be missing something here, but if the first argument to content_tag is always defined by the developer instead of a potentially malevolent user, why do we need to secure it this way?

@KensoDev

@DouweM The discussion actually originate in a previous pull request where I added the ability to change the wrapper tag that's being generated by simple_format.

One of the comments that @tenderlove said was that this is exposed to attack.

I closed that pull and opened a new one with a rewrite of the underlying code, using the paragraph_split function.

Then @rafaelfranca weighed in saying we CAN change it back since we are using content_tag, but actually this is still a problem in content_tag.

IMHO, this is something that's originated deep in application code, I would never pass user content into that method and tag names are always hard coded in the application code.

@rafaelfranca
Ruby on Rails member

I agree with @DouweM. The developer should not pass user input as the first argument of content_tag. I'm fine with an option that could change the wrapper tag.

I'm calling @NZKoz to the discussion.

@KensoDev

@rafaelfranca I agree with you both :-)

What I want to do is make paragraph_split private and add a :wrapper_tag option to simple_format.

@DouweM

@KensoDev Sounds good!

@rafaelfranca
Ruby on Rails member

Ok. Go ahead. If the possible content_tag problem was a real issue so we should fix the content_tag, not change the intention of this pull request.

👍 to make paragraph_split private and add a :wrapper_tag option.

@rafaelfranca
Ruby on Rails member

👍

@KensoDev

@rafaelfranca Updated the pull.

@DouweM DouweM commented on an outdated diff May 13, 2012
actionpack/lib/action_view/helpers/text_helper.rb
@@ -241,6 +241,7 @@ def word_wrap(text, *args)
#
# ==== Options
# * <tt>:sanitize</tt> - If +false+, does not sanitize +text+.
+ # * <tt>:wrapper_tag</tt> - String representing the tag wrapper, defaults to <tt><p></tt>
@DouweM
DouweM added a note May 13, 2012

"defaults to <p>" incorrectly implies that the value of :wrapper_tag should include the brackets.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@rafaelfranca rafaelfranca and 1 other commented on an outdated diff May 13, 2012
actionpack/lib/action_view/helpers/text_helper.rb
@@ -259,16 +260,17 @@ def word_wrap(text, *args)
# simple_format("<span>I'm allowed!</span> It's true.", {}, :sanitize => false)
# # => "<p><span>I'm allowed!</span> It's true.</p>"
def simple_format(text, html_options={}, options={})
- text = '' if text.nil?
- text = text.dup
- start_tag = tag('p', html_options, true)
- text = sanitize(text) unless options[:sanitize] == false
- text = text.to_str
- text.gsub!(/\r\n?/, "\n") # \r\n and \r -> \n
- text.gsub!(/\n\n+/, "</p>\n\n#{start_tag}") # 2+ newline -> paragraph
- text.gsub!(/([^\n]\n)(?=[^\n])/, '\1<br />') # 1 newline -> br
- text.insert 0, start_tag
- text.html_safe.safe_concat("</p>")
+ wrapper_tag = options.delete(:wrapper_tag) || "p"
@rafaelfranca
Ruby on Rails member
wrapper_tag = options.delete(:wrapper_tag) { :p }
@rafaelfranca
Ruby on Rails member

Also I think that we don't need to delete the key. Only options.fetch(:wrapper_tag, :p) should be fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@rafaelfranca rafaelfranca commented on an outdated diff May 13, 2012
actionpack/lib/action_view/helpers/text_helper.rb
@@ -413,6 +415,14 @@ def set_cycle(name, cycle_object)
@_cycles = Hash.new unless defined?(@_cycles)
@_cycles[name] = cycle_object
end
+
+ def paragraph_split(text)
+ return [] if text.blank?
+
+ text.to_str.gsub(/\r\n?/, "\n").split(/\n\n+/).map! do |t|
+ t.gsub!(/([^\n]\n)(?=[^\n])/, '\1<br />') || t
@rafaelfranca
Ruby on Rails member

I think that the indentation here is wrong

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

@rafaelfranca fixed indentation
@DouweM fixed the default to...

@rafaelfranca
Ruby on Rails member

I think these are the last changes.

Please squash there commits in two: one for the paragraph_split refactoring and other for the :wrapper_tag addition. And force push for this branch.

I'll talk with @NZKoz and @tenderlove about the content_tag and merge it tomorrow.

Thank you for your patience.

@KensoDev

No problem, thanks

@carlosantoniodasilva carlosantoniodasilva commented on an outdated diff May 14, 2012
actionpack/lib/action_view/helpers/text_helper.rb
- start_tag = tag('p', html_options, true)
- text = sanitize(text) unless options[:sanitize] == false
- text = text.to_str
- text.gsub!(/\r\n?/, "\n") # \r\n and \r -> \n
- text.gsub!(/\n\n+/, "</p>\n\n#{start_tag}") # 2+ newline -> paragraph
- text.gsub!(/([^\n]\n)(?=[^\n])/, '\1<br />') # 1 newline -> br
- text.insert 0, start_tag
- text.html_safe.safe_concat("</p>")
+ wrapper_tag = options.delete(:wrapper_tag) || "p"
+ paragraphs = paragraph_split(text)
+
+ if paragraphs.empty?
+ content_tag(wrapper_tag, '', html_options)
+ else
+ paragraphs.map { |paragraph|
+ paragraph = sanitize(paragraph) unless options[:sanitize] == false
@carlosantoniodasilva
Ruby on Rails member

Text was being sanitized only once, before the gsub calls, I think it's ok to keep the same logic instead of sanitizing each block.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@carlosantoniodasilva carlosantoniodasilva commented on an outdated diff May 14, 2012
actionpack/lib/action_view/helpers/text_helper.rb
@@ -259,16 +260,17 @@ def word_wrap(text, *args)
# simple_format("<span>I'm allowed!</span> It's true.", {}, :sanitize => false)
# # => "<p><span>I'm allowed!</span> It's true.</p>"
def simple_format(text, html_options={}, options={})
- text = '' if text.nil?
- text = text.dup
- start_tag = tag('p', html_options, true)
- text = sanitize(text) unless options[:sanitize] == false
- text = text.to_str
- text.gsub!(/\r\n?/, "\n") # \r\n and \r -> \n
- text.gsub!(/\n\n+/, "</p>\n\n#{start_tag}") # 2+ newline -> paragraph
- text.gsub!(/([^\n]\n)(?=[^\n])/, '\1<br />') # 1 newline -> br
- text.insert 0, start_tag
- text.html_safe.safe_concat("</p>")
+ wrapper_tag = options.delete(:wrapper_tag) || "p"
+ paragraphs = paragraph_split(text)
+
+ if paragraphs.empty?
+ content_tag(wrapper_tag, '', html_options)
@carlosantoniodasilva
Ruby on Rails member

Can change to nil instead of empty string:

>> helper.content_tag :p, nil, {}
=> "<p></p>"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@carlosantoniodasilva carlosantoniodasilva commented on an outdated diff May 14, 2012
actionpack/lib/action_view/helpers/text_helper.rb
@@ -413,6 +415,14 @@ def set_cycle(name, cycle_object)
@_cycles = Hash.new unless defined?(@_cycles)
@_cycles[name] = cycle_object
end
+
+ def paragraph_split(text)
@carlosantoniodasilva
Ruby on Rails member

Perhaps we could call it something like split_paragraphs, or split_line_breaks, wdyt?

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

@carlosantoniodasilva Done, changed the name and changed the implementation inside simple_format.

@rafaelfranca implemented the comments, rebased into 2 commits, one with the refactoring and one with the :wrapper_tag option.

@carlosgaldino carlosgaldino commented on the diff May 14, 2012
actionpack/lib/action_view/helpers/text_helper.rb
text = sanitize(text) unless options[:sanitize] == false
- text = text.to_str
- text.gsub!(/\r\n?/, "\n") # \r\n and \r -> \n
- text.gsub!(/\n\n+/, "</p>\n\n#{start_tag}") # 2+ newline -> paragraph
- text.gsub!(/([^\n]\n)(?=[^\n])/, '\1<br />') # 1 newline -> br
- text.insert 0, start_tag
- text.html_safe.safe_concat("</p>")
+ wrapper_tag = options.fetch(:wrapper_tag, :p)
+ paragraphs = split_paragraphs(text)
+
+ if paragraphs.empty?
+ content_tag(wrapper_tag, nil, html_options)
+ else
+ paragraphs.map { |paragraph|
+ content_tag(wrapper_tag, paragraph, html_options, options[:sanitize])
+ }.join("\n\n").html_safe

I think you can use safe_join here. Of course you need to have the array first because is not an Array method.

@rafaelfranca
Ruby on Rails member

In this case is better to use the join.html_safe, because safe_join escapes the separator, and we don't need to escape it since we know what is the separator content.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@rafaelfranca rafaelfranca and 1 other commented on an outdated diff May 14, 2012
actionpack/test/template/text_helper_test.rb
@@ -46,6 +46,30 @@ def test_simple_format_should_not_sanitize_input_when_sanitize_option_is_false
assert_equal "<p><b> test with unsafe string </b><script>code!</script></p>", simple_format("<b> test with unsafe string </b><script>code!</script>", {}, :sanitize => false)
end
+ def test_split_paragraphs_should_split_text_into_paragraphs
+ paragraphs = split_paragraphs("This is the first paagraph\n\nThis is the second paragraph")
@rafaelfranca
Ruby on Rails member

Is not split_paragraphs private? I think we don't need to test private methods.

it used to be public, you want me to remove the specs and resquash?

@rafaelfranca
Ruby on Rails member

Yes. Please remove all the tests to split_paragraphs

Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
KensoDev added some commits May 11, 2012
@KensoDev KensoDev simple_format refactoring
using split_paragraphs method and content_tag instead of string
concatination
09fcac9
@KensoDev KensoDev Added the wrapper tag option to simple_format 4bb14b0
@KensoDev

@rafaelfranca All comments implemented, this can now be pulled in.

Thanks

@rafaelfranca
Ruby on Rails member

Ok. I'll merge this later.

Thank you so much.

@rafaelfranca rafaelfranca was assigned May 15, 2012
@rafaelfranca rafaelfranca merged commit 7994496 into rails:master May 15, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment