Merge pull request #39060 from alipman88/fix_cache_fragment_digests

Ensure cache fragment keys (i.e. dependency tree digests) include all relevant templates

actionview/CHANGELOG.md

@@ -1,3 +1,12 @@
+*   Ensure cache fragment digests include all relevant template dependencies when
+    fragments are contained in a block passed to the render helper. Remove the
+    virtual_path keyword arguments found in CacheHelper as they no longer possess
+    any function following 1581cab.
+
+    Fixes #38984
+
+    *Aaron Lipman*
+
 *   Deprecate `config.action_view.raise_on_missing_translations` in favor of
     `config.i18n.raise_on_missing_translations`.
 

actionview/lib/action_view/base.rb

@@ -269,13 +269,13 @@ def initialize(lookup_context = nil, assigns = {}, controller = nil, formats = N
       _prepare_context
     end
 
-    def _run(method, template, locals, buffer, &block)
-      _old_output_buffer, _old_virtual_path, _old_template = @output_buffer, @virtual_path, @current_template
-      @current_template = template
+    def _run(method, template, locals, buffer, add_to_stack: true, &block)
+      _old_output_buffer, _old_template = @output_buffer, @current_template
+      @current_template = template if add_to_stack
       @output_buffer = buffer
       send(method, locals, buffer, &block)
     ensure
-      @output_buffer, @virtual_path, @current_template = _old_output_buffer, _old_virtual_path, _old_template
+      @output_buffer, @current_template = _old_output_buffer, _old_template
     end
 
     def compiled_method_container

actionview/lib/action_view/context.rb

@@ -18,7 +18,6 @@ module Context
     def _prepare_context
       @view_flow     = OutputFlow.new
       @output_buffer = nil
-      @virtual_path  = nil
     end
 
     # Encapsulates the interaction with the view flow so it

actionview/lib/action_view/helpers/cache_helper.rb

@@ -165,7 +165,7 @@ module CacheHelper
       # expire the cache.
       def cache(name = {}, options = {}, &block)
         if controller.respond_to?(:perform_caching) && controller.perform_caching
-          name_options = options.slice(:skip_digest, :virtual_path)
+          name_options = options.slice(:skip_digest)
           safe_concat(fragment_for(cache_fragment_name(name, **name_options), options, &block))
         else
           yield
@@ -205,14 +205,11 @@ def cache_unless(condition, name = {}, options = {}, &block)
       # fragments can be manually bypassed. This is useful when cache fragments
       # cannot be manually expired unless you know the exact key which is the
       # case when using memcached.
-      #
-      # The digest will be generated using +virtual_path:+ if it is provided.
-      #
-      def cache_fragment_name(name = {}, skip_digest: nil, virtual_path: nil, digest_path: nil)
+      def cache_fragment_name(name = {}, skip_digest: nil, digest_path: nil)
         if skip_digest
           name
         else
-          fragment_name_with_digest(name, virtual_path, digest_path)
+          fragment_name_with_digest(name, digest_path)
         end
       end
 
@@ -227,14 +224,11 @@ def digest_path_from_template(template) # :nodoc:
       end
 
     private
-      def fragment_name_with_digest(name, virtual_path, digest_path)
-        virtual_path ||= @virtual_path
-
-        if virtual_path || digest_path
-          name = controller.url_for(name).split("://").last if name.is_a?(Hash)
+      def fragment_name_with_digest(name, digest_path)
+        name = controller.url_for(name).split("://").last if name.is_a?(Hash)
 
+        if @current_template&.virtual_path || digest_path
           digest_path ||= digest_path_from_template(@current_template)
-
           [ digest_path, name ]
         else
           name
@@ -243,10 +237,10 @@ def fragment_name_with_digest(name, virtual_path, digest_path)
 
       def fragment_for(name = {}, options = nil, &block)
         if content = read_fragment_for(name, options)
-          @view_renderer.cache_hits[@virtual_path] = :hit if defined?(@view_renderer)
+          @view_renderer.cache_hits[@current_template&.virtual_path] = :hit if defined?(@view_renderer)
           content
         else
-          @view_renderer.cache_hits[@virtual_path] = :miss if defined?(@view_renderer)
+          @view_renderer.cache_hits[@current_template&.virtual_path] = :miss if defined?(@view_renderer)
           write_fragment_for(name, options, &block)
         end
       end

actionview/lib/action_view/helpers/translation_helper.rb

@@ -124,10 +124,10 @@ def localize(object, **options)
         def scope_key_by_partial(key)
           stringified_key = key.to_s
           if stringified_key.start_with?(".")
-            if @virtual_path
+            if @current_template&.virtual_path
               @_scope_key_by_partial_cache ||= {}
-              @_scope_key_by_partial_cache[@virtual_path] ||= @virtual_path.gsub(%r{/_?}, ".")
-              "#{@_scope_key_by_partial_cache[@virtual_path]}#{stringified_key}"
+              @_scope_key_by_partial_cache[@current_template.virtual_path] ||= @current_template.virtual_path.gsub(%r{/_?}, ".")
+              "#{@_scope_key_by_partial_cache[@current_template.virtual_path]}#{stringified_key}"
             else
               raise "Cannot use t(#{key.inspect}) shortcut because path is not available"
             end

actionview/lib/action_view/renderer/partial_renderer.rb

@@ -282,7 +282,7 @@ def render_partial_template(view, locals, template, layout, block)
           identifier: template.identifier,
           layout: layout && layout.virtual_path
         ) do |payload|
-          content = template.render(view, locals) do |*name|
+          content = template.render(view, locals, add_to_stack: !block) do |*name|
             view._layout_for(*name, &block)
           end
 

actionview/lib/action_view/renderer/partial_renderer/collection_caching.rb

@@ -68,7 +68,7 @@ def collection_by_cache_keys(view, template, collection)
       end
 
       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 = view.combined_fragment_cache_key(view.cache_fragment_name(key, digest_path: digest_path))
         key.frozen? ? key.dup : key # #read_multi & #write may require mutability, Dalli 2.6.0.
       end
 

actionview/lib/action_view/template.rb

@@ -176,10 +176,10 @@ 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 = ActionView::OutputBuffer.new, &block)
+    def render(view, locals, buffer = ActionView::OutputBuffer.new, add_to_stack: true, &block)
       instrument_render_template do
         compile!(view)
-        view._run(method_name, self, locals, buffer, &block)
+        view._run(method_name, self, locals, buffer, add_to_stack: add_to_stack, &block)
       end
     rescue => e
       handle_render_error(view, e)

actionview/test/abstract_unit.rb

@@ -59,8 +59,6 @@ def view
   end
 
   def render_erb(string)
-    @virtual_path = nil
-
     template = ActionView::Template.new(
       string.strip,
       "test template",

actionview/test/activerecord/relation_cache_test.rb

@@ -10,7 +10,6 @@ def setup
     view_paths     = ActionController::Base.view_paths
     lookup_context = ActionView::LookupContext.new(view_paths, {}, ["test"])
     @view_renderer = ActionView::Renderer.new(lookup_context)
-    @virtual_path  = "path"
     @current_template = lookup_context.find "test/hello_world"
 
     controller.cache_store = ActiveSupport::Cache::MemoryStore.new

actionview/test/fixtures/test/cache_fragment_inside_render_layout_block_1.html.erb

@@ -0,0 +1 @@
+<%= render layout: "layouts/yield_only" do %><%= cache "foo" do %>CAT<% end %><% end %>

actionview/test/fixtures/test/cache_fragment_inside_render_layout_block_2.html.erb

@@ -0,0 +1 @@
+<%= render layout: "layouts/yield_only" do %><%= cache "foo" do %>DOG<% end %><% end %>

actionview/test/template/render_test.rb

@@ -22,6 +22,7 @@ def combined_fragment_cache_key(key)
 
     controller = TestController.new
     controller.perform_caching = true
+    controller.cache_store = :memory_store
     @view.controller = controller
 
     @controller_view = controller.view_context_class.with_empty_template_cache.new(
@@ -705,6 +706,13 @@ def setup
     assert_equal ActionView::OptimizedFileSystemResolver, view_paths.first.class
     setup_view(view_paths)
   end
+
+  def test_cache_fragments_inside_render_layout_call_with_block
+    cat = @view.render(template: "test/cache_fragment_inside_render_layout_block_1")
+    dog = @view.render(template: "test/cache_fragment_inside_render_layout_block_2")
+
+    assert_not_equal cat, dog
+  end
 end
 
 class LazyViewRenderTest < ActiveSupport::TestCase

actionview/test/template/template_test.rb

@@ -22,7 +22,6 @@ class Context < ActionView::Base
     def initialize(*)
       super
       @output_buffer = "original"
-      @virtual_path = nil
     end
 
     def hello
@@ -35,7 +34,7 @@ def apostrophe
 
     def partial
       ActionView::Template.new(
-        "<%= @virtual_path %>",
+        "<%= @current_template.virtual_path %>",
         "partial",
         ERBHandler,
         virtual_path: "partial",
@@ -115,9 +114,9 @@ def test_restores_buffer
   end
 
   def test_virtual_path
-    @template = new_template("<%= @virtual_path %>" \
+    @template = new_template("<%= @current_template.virtual_path %>" \
                              "<%= partial.render(self, {}) %>" \
-                             "<%= @virtual_path %>")
+                             "<%= @current_template.virtual_path %>")
     assert_equal "hellopartialhello", render
   end