Merge pull request #35076 from rails/more-ro-objects

More Read-Only Changes

6 files changed, 77 insertions, 56 deletions

diff --git a/actionview/lib/action_view/lookup_context.rb b/actionview/lib/action_view/lookup_context.rb
index cc262dc2b7..c2f6439e26 100644
--- a/actionview/lib/action_view/lookup_context.rb
+++ b/actionview/lib/action_view/lookup_context.rb
@@ -3,6 +3,7 @@
 require "concurrent/map"
 require "active_support/core_ext/module/remove_method"
 require "active_support/core_ext/module/attribute_accessors"
+require "active_support/deprecation"
 require "action_view/template/resolver"
 
 module ActionView
@@ -132,11 +133,24 @@ module ActionView
       # Adds fallbacks to the view paths. Useful in cases when you are rendering
       # a :file.
       def with_fallbacks
-        view_paths = @view_paths
-        @view_paths = build_view_paths((view_paths.paths + self.class.fallbacks).uniq)
-        yield
-      ensure
-        @view_paths = view_paths
+        view_paths = build_view_paths((@view_paths.paths + self.class.fallbacks).uniq)
+
+        if block_given?
+          ActiveSupport::Deprecation.warn <<~eowarn
+          Calling `with_fallbacks` with a block is deprecated.  Call methods on
+          the lookup context returned by `with_fallbacks` instead.
+          eowarn
+
+          begin
+            _view_paths = @view_paths
+            @view_paths = view_paths
+            yield
+          ensure
+            @view_paths = _view_paths
+          end
+        else
+          ActionView::LookupContext.new(view_paths, @details, @prefixes)
+        end
       end
 
     private
diff --git a/actionview/lib/action_view/renderer/abstract_renderer.rb b/actionview/lib/action_view/renderer/abstract_renderer.rb
index 20b2523cac..ae366ce54a 100644
--- a/actionview/lib/action_view/renderer/abstract_renderer.rb
+++ b/actionview/lib/action_view/renderer/abstract_renderer.rb
@@ -17,7 +17,7 @@ module ActionView
   # that new object is called in turn. This abstracts the setup and rendering
   # into a separate classes for partials and templates.
   class AbstractRenderer #:nodoc:
-    delegate :find_template, :find_file, :template_exists?, :any_templates?, :with_fallbacks, :with_layout_format, :formats, to: :@lookup_context
+    delegate :template_exists?, :any_templates?, :formats, to: :@lookup_context
 
     def initialize(lookup_context)
       @lookup_context = lookup_context
@@ -38,8 +38,6 @@ module ActionView
       end
 
       def instrument(name, **options) # :doc:
-        options[:identifier] ||= (@template && @template.identifier) || @path
-
         ActiveSupport::Notifications.instrument("render_#{name}.action_view", options) do |payload|
           yield payload
         end
diff --git a/actionview/lib/action_view/renderer/partial_renderer.rb b/actionview/lib/action_view/renderer/partial_renderer.rb
index f175c30aa1..478400e016 100644
--- a/actionview/lib/action_view/renderer/partial_renderer.rb
+++ b/actionview/lib/action_view/renderer/partial_renderer.rb
@@ -296,42 +296,43 @@ module ActionView
 
     def render(context, options, block)
       setup(context, options, block)
-      @template = find_partial
+      template = find_partial
 
       @lookup_context.rendered_format ||= begin
-        if @template && @template.formats.present?
-          @template.formats.first
+        if template && template.formats.first
+          template.formats.first
         else
           formats.first
         end
       end
 
       if @collection
-        render_collection
+        render_collection(context, template)
       else
-        render_partial
+        render_partial(context, template)
       end
     end
 
     private
 
-      def render_collection
-        instrument(:collection, count: @collection.size) do |payload|
+      def render_collection(view, template)
+        identifier = (template && template.identifier) || @path
+        instrument(:collection, identifier: identifier, count: @collection.size) do |payload|
           return nil if @collection.blank?
 
           if @options.key?(:spacer_template)
-            spacer = find_template(@options[:spacer_template], @locals.keys).render(@view, @locals)
+            spacer = find_template(@options[:spacer_template], @locals.keys).render(view, @locals)
           end
 
-          cache_collection_render(payload) do
-            @template ? collection_with_template : collection_without_template
+          cache_collection_render(payload, view, template) do
+            template ? collection_with_template(view, template) : collection_without_template(view)
           end.join(spacer).html_safe
         end
       end
 
-      def render_partial
-        instrument(:partial) do |payload|
-          view, locals, block = @view, @locals, @block
+      def render_partial(view, template)
+        instrument(:partial, identifier: template.identifier) do |payload|
+          locals, block = @locals, @block
           object, as = @object, @variable
 
           if !block && (layout = @options[:layout])
@@ -341,12 +342,12 @@ module ActionView
           object = locals[as] if object.nil? # Respect object when object is false
           locals[as] = object if @has_object
 
-          content = @template.render(view, locals) do |*name|
+          content = template.render(view, locals) do |*name|
             view._layout_for(*name, &block)
           end
 
           content = layout.render(view, locals) { content } if layout
-          payload[:cache_hit] = view.view_renderer.cache_hits[@template.virtual_path]
+          payload[:cache_hit] = view.view_renderer.cache_hits[template.virtual_path]
           content
         end
       end
@@ -359,7 +360,6 @@ module ActionView
       # set to that string. Otherwise, the +options[:partial]+ object must
       # respond to +to_partial_path+ in order to setup the path.
       def setup(context, options, block)
-        @view = context
         @options = options
         @block   = block
 
@@ -381,10 +381,10 @@ module ActionView
           @collection = collection_from_object || collection_from_options
 
           if @collection
-            paths = @collection_data = @collection.map { |o| partial_path(o) }
+            paths = @collection_data = @collection.map { |o| partial_path(o, context) }
             @path = paths.uniq.one? ? paths.first : nil
           else
-            @path = partial_path
+            @path = partial_path(@object, context)
           end
         end
 
@@ -423,8 +423,8 @@ module ActionView
         @lookup_context.find_template(path, prefixes, true, locals, @details)
       end
 
-      def collection_with_template
-        view, locals, template = @view, @locals, @template
+      def collection_with_template(view, template)
+        locals = @locals
         as, counter, iteration = @variable, @variable_counter, @variable_iteration
 
         if layout = @options[:layout]
@@ -445,8 +445,8 @@ module ActionView
         end
       end
 
-      def collection_without_template
-        view, locals, collection_data = @view, @locals, @collection_data
+      def collection_without_template(view)
+        locals, collection_data = @locals, @collection_data
         cache = {}
         keys  = @locals.keys
 
@@ -474,7 +474,7 @@ module ActionView
       #
       # If +prefix_partial_path_with_controller_namespace+ is true, then this
       # method will prefix the partial paths with a namespace.
-      def partial_path(object = @object)
+      def partial_path(object, view)
         object = object.to_model if object.respond_to?(:to_model)
 
         path = if object.respond_to?(:to_partial_path)
@@ -483,7 +483,7 @@ module ActionView
           raise ArgumentError.new("'#{object.inspect}' is not an ActiveModel-compatible object. It must implement :to_partial_path.")
         end
 
-        if @view.prefix_partial_path_with_controller_namespace
+        if view.prefix_partial_path_with_controller_namespace
           prefixed_partial_names[path] ||= merge_prefix_into_object_path(@context_prefix, path.dup)
         else
           path
diff --git a/actionview/lib/action_view/renderer/partial_renderer/collection_caching.rb b/actionview/lib/action_view/renderer/partial_renderer/collection_caching.rb
index 5aa6f77902..388f9e5e56 100644
--- a/actionview/lib/action_view/renderer/partial_renderer/collection_caching.rb
+++ b/actionview/lib/action_view/renderer/partial_renderer/collection_caching.rb
@@ -11,13 +11,13 @@ module ActionView
     end
 
     private
-      def cache_collection_render(instrumentation_payload)
+      def cache_collection_render(instrumentation_payload, view, template)
         return yield unless @options[:cached]
 
         # Result is a hash with the key represents the
         # key used for cache lookup and the value is the item
         # on which the partial is being rendered
-        keyed_collection = collection_by_cache_keys
+        keyed_collection = collection_by_cache_keys(view, template)
 
         # Pull all partials from cache
         # Result is a hash, key matches the entry in
@@ -51,23 +51,21 @@ module ActionView
         @options[:cached].respond_to?(:call)
       end
 
-      def collection_by_cache_keys
+      def collection_by_cache_keys(view, template)
         seed = callable_cache_key? ? @options[:cached] : ->(i) { i }
 
+        digest_path = view.digest_path_from_virtual(template.virtual_path)
+
         @collection.each_with_object({}) do |item, hash|
-          hash[expanded_cache_key(seed.call(item))] = item
+          hash[expanded_cache_key(seed.call(item), view, template, digest_path)] = item
         end
       end
 
-      def expanded_cache_key(key)
-        key = @view.combined_fragment_cache_key(@view.cache_fragment_name(key, virtual_path: @template.virtual_path, digest_path: digest_path))
+      def expanded_cache_key(key, view, template, digest_path)
+        key = view.combined_fragment_cache_key(view.cache_fragment_name(key, virtual_path: template.virtual_path, digest_path: digest_path))
         key.frozen? ? key.dup : key # #read_multi & #write may require mutability, Dalli 2.6.0.
       end
 
-      def digest_path
-        @digest_path ||= @view.digest_path_from_virtual(@template.virtual_path)
-      end
-
       # `order_by` is an enumerable object containing keys of the cache,
       # all keys are  passed in whether found already or not.
       #
diff --git a/actionview/lib/action_view/renderer/template_renderer.rb b/actionview/lib/action_view/renderer/template_renderer.rb
index fb5539e0db..c36baeffcd 100644
--- a/actionview/lib/action_view/renderer/template_renderer.rb
+++ b/actionview/lib/action_view/renderer/template_renderer.rb
@@ -12,7 +12,7 @@ module ActionView
 
       @lookup_context.rendered_format ||= (template.formats.first || formats.first)
 
-      render_template(context, template, options[:layout], options[:locals])
+      render_template(context, template, options[:layout], options[:locals] || {})
     end
 
     private
@@ -28,7 +28,7 @@ module ActionView
         elsif options.key?(:html)
           Template::HTML.new(options[:html], formats.first)
         elsif options.key?(:file)
-          with_fallbacks { find_file(options[:file], nil, false, keys, @details) }
+          @lookup_context.with_fallbacks.find_file(options[:file], nil, false, keys, @details)
         elsif options.key?(:inline)
           handler = Template.handler_for_extension(options[:type] || "erb")
           Template.new(options[:inline], "inline template", handler, locals: keys)
@@ -36,7 +36,7 @@ module ActionView
           if options[:template].respond_to?(:render)
             options[:template]
           else
-            find_template(options[:template], options[:prefixes], false, keys, @details)
+            @lookup_context.find_template(options[:template], options[:prefixes], false, keys, @details)
           end
         else
           raise ArgumentError, "You invoked render but did not give any of :partial, :template, :inline, :file, :plain, :html or :body option."
@@ -45,9 +45,7 @@ module ActionView
 
       # Renders the given template. A string representing the layout can be
       # supplied as well.
-      def render_template(view, template, layout_name = nil, locals = nil)
-        locals ||= {}
-
+      def render_template(view, template, layout_name, locals)
         render_with_layout(view, layout_name, locals) do |layout|
           instrument(:template, identifier: template.identifier, layout: layout.try(:virtual_path)) do
             template.render(view, locals) { |*name| view._layout_for(*name) }
@@ -82,9 +80,9 @@ module ActionView
         when String
           begin
             if layout.start_with?("/")
-              with_fallbacks { find_template(layout, nil, false, [], details) }
+              @lookup_context.with_fallbacks.find_template(layout, nil, false, [], details)
             else
-              find_template(layout, nil, false, [], details)
+              @lookup_context.find_template(layout, nil, false, [], details)
             end
           rescue ActionView::MissingTemplate
             all_details = @details.merge(formats: @lookup_context.default_formats)
diff --git a/actionview/test/template/lookup_context_test.rb b/actionview/test/template/lookup_context_test.rb
index 0894925c9f..290f832794 100644
--- a/actionview/test/template/lookup_context_test.rb
+++ b/actionview/test/template/lookup_context_test.rb
@@ -122,19 +122,32 @@ class LookupContextTest < ActiveSupport::TestCase
   test "adds fallbacks to view paths when required" do
     assert_equal 1, @lookup_context.view_paths.size
 
-    @lookup_context.with_fallbacks do
-      assert_equal 3, @lookup_context.view_paths.size
-      assert_includes @lookup_context.view_paths, ActionView::FallbackFileSystemResolver.new("")
-      assert_includes @lookup_context.view_paths, ActionView::FallbackFileSystemResolver.new("/")
+    assert_deprecated do
+      @lookup_context.with_fallbacks do
+        assert_equal 3, @lookup_context.view_paths.size
+        assert_includes @lookup_context.view_paths, ActionView::FallbackFileSystemResolver.new("")
+        assert_includes @lookup_context.view_paths, ActionView::FallbackFileSystemResolver.new("/")
+      end
     end
+
+    @lookup_context = @lookup_context.with_fallbacks
+
+    assert_equal 3, @lookup_context.view_paths.size
+    assert_includes @lookup_context.view_paths, ActionView::FallbackFileSystemResolver.new("")
+    assert_includes @lookup_context.view_paths, ActionView::FallbackFileSystemResolver.new("/")
   end
 
   test "add fallbacks just once in nested fallbacks calls" do
-    @lookup_context.with_fallbacks do
+    assert_deprecated do
       @lookup_context.with_fallbacks do
-        assert_equal 3, @lookup_context.view_paths.size
+        @lookup_context.with_fallbacks do
+          assert_equal 3, @lookup_context.view_paths.size
+        end
       end
     end
+
+    @lookup_context = @lookup_context.with_fallbacks.with_fallbacks
+    assert_equal 3, @lookup_context.view_paths.size
   end
 
   test "generates a new details key for each details hash" do