Merge pull request #49782 from Shopify/fix-strict-locals

Ignore implicit locals if not declared by templates with strict locals

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*

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)

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

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.