From ba1285c5ed1412ed926e0362aa9de9a92194d58b Mon Sep 17 00:00:00 2001 From: Jean Boussier Date: Wed, 25 Oct 2023 18:00:06 +0200 Subject: [PATCH] Merge pull request #49782 from Shopify/fix-strict-locals Ignore implicit locals if not declared by templates with strict locals --- actionview/CHANGELOG.md | 8 +++++ .../renderer/collection_renderer.rb | 2 +- actionview/lib/action_view/template.rb | 36 +++++++++++++------ actionview/test/template/template_test.rb | 19 ++++++++-- 4 files changed, 51 insertions(+), 14 deletions(-) diff --git a/actionview/CHANGELOG.md b/actionview/CHANGELOG.md index 169b2862bbfae..2da5ec5558ea5 100644 --- a/actionview/CHANGELOG.md +++ b/actionview/CHANGELOG.md @@ -1,3 +1,11 @@ +* Automatically discard the implicit locals injected by collection rendering for template that can't accept them + + When rendering a collection, two implicit variables are injected, which breaks templates with strict locals. + + Now they are only passed if the template will actually accept them. + + *Yasha Krasnou*, *Jean Boussier* + * Fix `@rails/ujs` calling `start()` an extra time when using bundlers *Hartley McGuire*, *Ryunosuke Sato* diff --git a/actionview/lib/action_view/renderer/collection_renderer.rb b/actionview/lib/action_view/renderer/collection_renderer.rb index 7eba19edb92c6..64e3f8668d30a 100644 --- a/actionview/lib/action_view/renderer/collection_renderer.rb +++ b/actionview/lib/action_view/renderer/collection_renderer.rb @@ -194,7 +194,7 @@ def collection_with_template(view, template, layout, collection) _template = (cache[path] ||= (template || find_template(path, @locals.keys + [as, counter, iteration]))) - content = _template.render(view, locals) + content = _template.render(view, locals, implicit_locals: [counter, iteration]) content = layout.render(view, locals) { content } if layout partial_iteration.iterate! build_rendered_template(content, _template) diff --git a/actionview/lib/action_view/template.rb b/actionview/lib/action_view/template.rb index 189e73c23991e..da0c8e7a4e8bc 100644 --- a/actionview/lib/action_view/template.rb +++ b/actionview/lib/action_view/template.rb @@ -201,6 +201,7 @@ def initialize(source, identifier, handler, locals:, format: nil, variant: nil, @variant = variant @compile_mutex = Mutex.new @strict_locals = NONE + @strict_local_keys = nil @type = nil end @@ -244,9 +245,15 @@ def supports_streaming? # This method is instrumented as "!render_template.action_view". Notice that # we use a bang in this instrumentation because you don't want to # consume this in production. This is only slow if it's being listened to. - def render(view, locals, buffer = nil, add_to_stack: true, &block) + def render(view, locals, buffer = nil, implicit_locals: [], add_to_stack: true, &block) instrument_render_template do compile!(view) + + if strict_locals? && @strict_local_keys && !implicit_locals.empty? + locals_to_ignore = implicit_locals - @strict_local_keys + locals.except!(*locals_to_ignore) + end + if buffer view._run(method_name, self, locals, buffer, add_to_stack: add_to_stack, has_strict_locals: strict_locals?, &block) nil @@ -474,23 +481,30 @@ def compile(mod) return unless strict_locals? + parameters = mod.instance_method(method_name).parameters - [[:req, :output_buffer]] # Check compiled method parameters to ensure that only kwargs # were provided as strict locals, preventing `locals: (foo, *foo)` etc # and allowing `locals: (foo:)`. - non_kwarg_parameters = - (mod.instance_method(method_name).parameters - [[:req, :output_buffer]]). - select { |parameter| ![:keyreq, :key, :keyrest, :nokey].include?(parameter[0]) } + non_kwarg_parameters = parameters.select do |parameter| + ![:keyreq, :key, :keyrest, :nokey].include?(parameter[0]) + end - return unless non_kwarg_parameters.any? + unless non_kwarg_parameters.empty? + mod.undef_method(method_name) - mod.undef_method(method_name) + raise ArgumentError.new( + "#{non_kwarg_parameters.map { |_, name| "`#{name}`" }.to_sentence} set as non-keyword " \ + "#{'argument'.pluralize(non_kwarg_parameters.length)} for #{short_identifier}. " \ + "Locals can only be set as keyword arguments." + ) + end - raise ArgumentError.new( - "#{non_kwarg_parameters.map { |_, name| "`#{name}`" }.to_sentence} set as non-keyword " \ - "#{'argument'.pluralize(non_kwarg_parameters.length)} for #{short_identifier}. " \ - "Locals can only be set as keyword arguments." - ) + unless parameters.any? { |type, _| type == :keyrest } + parameters.map!(&:first) + parameters.sort! + @strict_local_keys = parameters.freeze + end end def offset diff --git a/actionview/test/template/template_test.rb b/actionview/test/template/template_test.rb index 296b5931509b4..529a123c98922 100644 --- a/actionview/test/template/template_test.rb +++ b/actionview/test/template/template_test.rb @@ -62,8 +62,8 @@ def new_template(body = "<%= hello %>", details = {}) ActionView::Template.new(body.dup, "hello template", details.delete(:handler) || ERBHandler, **{ virtual_path: "hello" }.merge!(details)) end - def render(locals = {}) - @template.render(@context, locals) + def render(implicit_locals: [], **locals) + @template.render(@context, locals, implicit_locals: implicit_locals) end def setup @@ -200,6 +200,21 @@ def test_extra_locals_raises_error assert_match(/unknown local: :foo/, error.message) end + def test_rails_injected_locals_does_not_raise_error_if_not_passed + @template = new_template("<%# locals: (message:) -%>") + render(message: "Hi", message_counter: 1, message_iteration: 1, implicit_locals: %i[message_counter message_iteration]) + end + + def test_rails_injected_locals_can_be_specified + @template = new_template("<%# locals: (message: 'Hello') -%>\n<%= message %>") + assert_equal "Hello", render(message: "Hello", implicit_locals: %i[message]) + end + + def test_rails_injected_locals_can_be_specified_as_kwargs + @template = new_template("<%# locals: (message: 'Hello', **kwargs) -%>\n<%= kwargs[:message_counter] %>-<%= kwargs[:message_iteration] %>") + assert_equal "1-2", render(message: "Hello", message_counter: 1, message_iteration: 2, implicit_locals: %i[message_counter message_iteration]) + end + # TODO: This is currently handled inside ERB. The case of explicitly # lying about encodings via the normal Rails API should be handled # inside Rails.