From c2afa055614f15edfbd2f4c97f9254425286fc6e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Britto?= Date: Mon, 16 Dec 2013 00:51:35 -0200 Subject: Improve ERB dependency detection. The current implementation can't handle some special cases of oddly-formatted Ruby. Now we are able to detect them: * Multi-line arguments on the `render` call * Strings containing quotes, e.g. `"something's wrong"` * Multiple kinds of identifiers - instance variables, class variables and globals * Method chains as arguments for the `render` call Also, this fix reduces the rate of "false positives" which showed up when we had calls/access to identifiers containing `render`, like `surrender` and `rendering`. --- actionview/lib/action_view/dependency_tracker.rb | 74 +++++++++++++---- .../test/template/dependency_tracker_test.rb | 92 +++++++++++++++++++++- 2 files changed, 145 insertions(+), 21 deletions(-) diff --git a/actionview/lib/action_view/dependency_tracker.rb b/actionview/lib/action_view/dependency_tracker.rb index b2e8334077..748fa81221 100644 --- a/actionview/lib/action_view/dependency_tracker.rb +++ b/actionview/lib/action_view/dependency_tracker.rb @@ -1,7 +1,7 @@ require 'thread_safe' module ActionView - class DependencyTracker + class DependencyTracker # :nodoc: @trackers = ThreadSafe::Cache.new def self.find_dependencies(name, template) @@ -23,9 +23,36 @@ module ActionView @trackers.delete(handler) end - class ERBTracker + class ERBTracker # :nodoc: EXPLICIT_DEPENDENCY = /# Template Dependency: (\S+)/ + # A valid ruby identifier - suitable for class, method and specially variable names + IDENTIFIER = / + [[:alpha:]_] # at least one uppercase letter, lowercase letter or underscore + [[:word:]]* # followed by optional letters, numbers or underscores + /x + + # Any kind of variable name. e.g. @instance, @@class, $global or local. + # Possibly following a method call chain + VARIABLE_OR_METHOD_CHAIN = / + (?:\$|@{1,2})? # optional global, instance or class variable indicator + (?:#{IDENTIFIER}\.)* # followed by an optional chain of zero-argument method calls + (?#{IDENTIFIER}) # and a final valid identifier, captured as DYNAMIC + /x + + # A simple string literal. e.g. "School's out!" + STRING = / + (?['"]) # an opening quote + (?.*?) # with anything inside, captured as STATIC + \k # and a matching closing quote + /x + + # Part of any hash containing the :partial key + PARTIAL_HASH_KEY = / + (?:\bpartial:|:partial\s*=>) # partial key in either old or new style hash syntax + \s* # followed by optional spaces + /x + # Matches: # render partial: "comments/comment", collection: commentable.comments # render "comments/comments" @@ -36,11 +63,13 @@ module ActionView # render(topics) => render("topics/topic") # render(message.topics) => render("topics/topic") RENDER_DEPENDENCY = / - render\s* # render, followed by optional whitespace - \(? # start an optional parenthesis for the render call - (partial:|:partial\s+=>)?\s* # naming the partial, used with collection -- 1st capture - ([@a-z"'][@\w\/\."']+) # the template name itself -- 2nd capture - /x + \brender\b # render, the whole word + \s*\(?\s* # optional opening paren surrounded by spaces + (?: + (?:.*?#{PARTIAL_HASH_KEY})? # optional hash, up to the partial key declaration + (?:#{STRING}|#{VARIABLE_OR_METHOD_CHAIN}) # finally, the dependency name of interest + ) + /xm def self.call(name, template) new(name, template).dependencies @@ -68,19 +97,30 @@ module ActionView end def render_dependencies - source.scan(RENDER_DEPENDENCY). - collect(&:second).uniq. + render_dependencies = [] - # render(@topic) => render("topics/topic") - # render(topics) => render("topics/topic") - # render(message.topics) => render("topics/topic") - collect { |name| name.sub(/\A@?([a-z_]+\.)*([a-z_]+)\z/) { "#{$2.pluralize}/#{$2.singularize}" } }. + source.scan(RENDER_DEPENDENCY) do + add_dynamic_dependency(render_dependencies, Regexp.last_match[:dynamic]) + add_static_dependency(render_dependencies, Regexp.last_match[:static]) + end - # render("headline") => render("message/headline") - collect { |name| name.include?("/") ? name : "#{directory}/#{name}" }. + render_dependencies.uniq + end + + def add_dynamic_dependency(dependencies, dependency) + if dependency + dependencies << "#{dependency.pluralize}/#{dependency.singularize}" + end + end - # replace quotes from string renders - collect { |name| name.gsub(/["']/, "") } + def add_static_dependency(dependencies, dependency) + if dependency + if dependency.include?('/') + dependencies << dependency + else + dependencies << "#{directory}/#{dependency}" + end + end end def explicit_dependencies diff --git a/actionview/test/template/dependency_tracker_test.rb b/actionview/test/template/dependency_tracker_test.rb index 7a9b4b26ac..6540394c14 100644 --- a/actionview/test/template/dependency_tracker_test.rb +++ b/actionview/test/template/dependency_tracker_test.rb @@ -1,3 +1,5 @@ +# encoding: utf-8 + require 'abstract_unit' require 'action_view/dependency_tracker' @@ -52,23 +54,105 @@ class ERBTrackerTest < Minitest::Test def test_dependency_of_erb_template_with_number_in_filename template = FakeTemplate.new("<%# render 'messages/message123' %>", :erb) - tracker = make_tracker('messages/_message123', template) + tracker = make_tracker("messages/_message123", template) assert_equal ["messages/message123"], tracker.dependencies end def test_finds_dependency_in_correct_directory template = FakeTemplate.new("<%# render(message.topic) %>", :erb) - tracker = make_tracker('messages/_message', template) + tracker = make_tracker("messages/_message", template) assert_equal ["topics/topic"], tracker.dependencies end def test_finds_dependency_in_correct_directory_with_underscore template = FakeTemplate.new("<%# render(message_type.messages) %>", :erb) - tracker = make_tracker('message_types/_message_type', template) + tracker = make_tracker("message_types/_message_type", template) assert_equal ["messages/message"], tracker.dependencies end -end + def test_dependency_of_erb_template_with_no_spaces_after_render + template = FakeTemplate.new("<%# render'messages/message' %>", :erb) + tracker = make_tracker("messages/_message", template) + + assert_equal ["messages/message"], tracker.dependencies + end + + def test_finds_no_dependency_when_render_begins_the_name_of_an_identifier + template = FakeTemplate.new("<%# rendering 'it useless' %>", :erb) + tracker = make_tracker("resources/_resource", template) + + assert_equal [], tracker.dependencies + end + + def test_finds_no_dependency_when_render_ends_the_name_of_another_method + template = FakeTemplate.new("<%# surrender 'to reason' %>", :erb) + tracker = make_tracker("resources/_resource", template) + + assert_equal [], tracker.dependencies + end + + def test_finds_dependency_on_multiline_render_calls + template = FakeTemplate.new("<%# + render :object => @all_posts, + :partial => 'posts' %>", :erb) + + tracker = make_tracker("some/_little_posts", template) + + assert_equal ["some/posts"], tracker.dependencies + end + + def test_finds_multiple_unrelated_odd_dependencies + template = FakeTemplate.new(" + <%# render('shared/header', title: 'Title') %> +

Section title

+ <%# render@section %> + ", :erb) + + tracker = make_tracker("multiple/_dependencies", template) + + assert_equal ["shared/header", "sections/section"], tracker.dependencies + end + + def test_finds_dependencies_for_all_kinds_of_identifiers + template = FakeTemplate.new(" + <%# render $globals %> + <%# render @instance_variables %> + <%# render @@class_variables %> + ", :erb) + + tracker = make_tracker("identifiers/_all", template) + + assert_equal [ + "globals/global", + "instance_variables/instance_variable", + "class_variables/class_variable" + ], tracker.dependencies + end + + def test_finds_dependencies_on_method_chains + template = FakeTemplate.new("<%# render @parent.child.grandchildren %>", :erb) + tracker = make_tracker("method/_chains", template) + + assert_equal ["grandchildren/grandchild"], tracker.dependencies + end + + def test_finds_dependencies_with_special_characters + template = FakeTemplate.new("<%# render @pokémon, partial: 'ピカチュウ' %>", :erb) + tracker = make_tracker("special/_characters", template) + + assert_equal ["special/ピカチュウ"], tracker.dependencies + end + + def test_finds_dependencies_with_quotes_within + template = FakeTemplate.new(" + <%# render \"single/quote's\" %> + <%# render 'double/quote\"s' %> + ", :erb) + tracker = make_tracker("quotes/_single_and_double", template) + + assert_equal ["single/quote's", 'double/quote"s'], tracker.dependencies + end +end -- cgit v1.2.3 From ccbba3ff50a7c2d6523f55f7821aabdb89fc5d45 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Britto?= Date: Sat, 21 Dec 2013 22:14:07 -0200 Subject: Avoid scanning multiple render calls as a single match. Each chunk of text coming after `render` is now handled individually as a possible list of arguments. --- actionview/lib/action_view/dependency_tracker.rb | 36 ++++++++++++---------- .../test/template/dependency_tracker_test.rb | 30 +++++++++++++++--- 2 files changed, 45 insertions(+), 21 deletions(-) diff --git a/actionview/lib/action_view/dependency_tracker.rb b/actionview/lib/action_view/dependency_tracker.rb index 748fa81221..0ccf2515c5 100644 --- a/actionview/lib/action_view/dependency_tracker.rb +++ b/actionview/lib/action_view/dependency_tracker.rb @@ -54,21 +54,20 @@ module ActionView /x # Matches: - # render partial: "comments/comment", collection: commentable.comments - # render "comments/comments" - # render 'comments/comments' - # render('comments/comments') + # partial: "comments/comment", collection: @all_comments => "comments/comment" + # (object: @single_comment, partial: "comments/comment") => "comments/comment" # - # render(@topic) => render("topics/topic") - # render(topics) => render("topics/topic") - # render(message.topics) => render("topics/topic") - RENDER_DEPENDENCY = / - \brender\b # render, the whole word - \s*\(?\s* # optional opening paren surrounded by spaces - (?: - (?:.*?#{PARTIAL_HASH_KEY})? # optional hash, up to the partial key declaration - (?:#{STRING}|#{VARIABLE_OR_METHOD_CHAIN}) # finally, the dependency name of interest - ) + # "comments/comments" + # 'comments/comments' + # ('comments/comments') + # + # (@topic) => "topics/topic" + # topics => "topics/topic" + # (message.topics) => "topics/topic" + RENDER_ARGUMENTS = /\A + (?:\s*\(?\s*) # optional opening paren surrounded by spaces + (?:.*?#{PARTIAL_HASH_KEY})? # optional hash, up to the partial key declaration + (?:#{STRING}|#{VARIABLE_OR_METHOD_CHAIN}) # finally, the dependency name of interest /xm def self.call(name, template) @@ -98,10 +97,13 @@ module ActionView def render_dependencies render_dependencies = [] + render_calls = source.split(/\brender\b/).drop(1) - source.scan(RENDER_DEPENDENCY) do - add_dynamic_dependency(render_dependencies, Regexp.last_match[:dynamic]) - add_static_dependency(render_dependencies, Regexp.last_match[:static]) + render_calls.each do |arguments| + arguments.scan(RENDER_ARGUMENTS) do + add_dynamic_dependency(render_dependencies, Regexp.last_match[:dynamic]) + add_static_dependency(render_dependencies, Regexp.last_match[:static]) + end end render_dependencies.uniq diff --git a/actionview/test/template/dependency_tracker_test.rb b/actionview/test/template/dependency_tracker_test.rb index 6540394c14..df3a0602d1 100644 --- a/actionview/test/template/dependency_tracker_test.rb +++ b/actionview/test/template/dependency_tracker_test.rb @@ -147,12 +147,34 @@ class ERBTrackerTest < Minitest::Test end def test_finds_dependencies_with_quotes_within - template = FakeTemplate.new(" - <%# render \"single/quote's\" %> - <%# render 'double/quote\"s' %> - ", :erb) + template = FakeTemplate.new(%{ + <%# render "single/quote's" %> + <%# render 'double/quote"s' %> + }, :erb) + tracker = make_tracker("quotes/_single_and_double", template) assert_equal ["single/quote's", 'double/quote"s'], tracker.dependencies end + + def test_finds_dependencies_with_extra_spaces + template = FakeTemplate.new(%{ + <%= render "header" %> + <%= render partial: "form" %> + <%= render @message %> + <%= render ( @message.events ) %> + <%= render :collection => @message.comments, + :partial => "comments/comment" %> + }, :erb) + + tracker = make_tracker("spaces/_extra", template) + + assert_equal [ + "spaces/header", + "spaces/form", + "messages/message", + "events/event", + "comments/comment" + ], tracker.dependencies + end end -- cgit v1.2.3 From e987dcd78f6fa21d4a6c3a10f2347c80145bac40 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Britto?= Date: Thu, 9 Jan 2014 20:47:14 -0200 Subject: Update changelog --- actionview/CHANGELOG.md | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/actionview/CHANGELOG.md b/actionview/CHANGELOG.md index a57da72d8c..19877ca8cb 100644 --- a/actionview/CHANGELOG.md +++ b/actionview/CHANGELOG.md @@ -1,3 +1,10 @@ +* Improved ERB dependency detection. New argument types and formattings for the `render` + calls can be matched. + + Fixes #13074 and #13116 + + *João Britto* + * Use `display:none` instead of `display:inline` for hidden fields Fixes #6403 -- cgit v1.2.3