[PATCH] r8542 breaks caching

Grr, pesky formatting. That should have been

first_render[/\.(\w+)$/, 1].to_sym

I ran into the same issue.

Substituting the fragment Catfish mentions with

find_template_extension_for(first_render)[/\.(\w+)$/, 1].to_sym

returns what seems to be what the code wants.

However, I am using HAML, which does not provide a cache_haml_fragment method. Caching worked fine prior to the change via cache_erb_fragment. So I suggest changing the cache helper to call cache_erb_fragment by default. That's what it did before the change, so why break it?

I'm seeing this as well, rendering a partial inside a cache block. At line 35 of cache_helper.rb first_render is "dashboard/_sidebar" in my case, which the regexp can't extract an extension from.

(In [8614]) Fix CacheHelper#cache (closes #10733) [mindforge]

This still breaks for single-extension templates (eg index.erb rather than index.html.erb), where find_template_extension_for(first_render) returns nil

I've attached a preliminary patch that adds a few functional caching tests. At present, caching is only tested in a very unit-testy way, never for testing that a controller will correctly cache something. There's only a couple of tests there at the moment, but they give test coverage for things that are completely untested otherwise - eg cache_helper.rb.

The patch also includes two cache_helper fixes - handler_for_extension should be handler_class_for_extension, and I've made the '.' in the template extension optional, so it will correctly deal with both 'html.erb' and just 'erb'

Finally, there's a failing test case in there that displays my cache problems. Trying to cache an erb partial that's rendered from an rjs template will fail, since find_template_extension_for(first_render) returns nil. I suspect this is an ActionController bug - shouldn't it be using the full path (complete with an extension) when calling @template.render_file in AC's base.rb:1100 ?

@template.render_file does not have to use the full_path. It normally is able to correctly infer the extension/format of the file without it.

I think what's happening is that when the the RJS template goes for rendering, the variables that allows it to assume '.js.rjs' as part of the filename is set to what the ERB partial needed. So, find_template_extension_for(first_render) nils out since the parent file wasn't ERB.

render_template knows what the correct extension is for the file is. Rather than incorrectly infer from first_render, the cache helper can use what render_template already knew. That's what this patch does.

Looks good to me. +1

+1. Patch fixes test_fragment_caching_in_rjs_partials in caching_test.rb.

allows caching from RJS

Fixed the diff to use relative paths to actionpack. No other change.

I think making cache_erb_fragment default defeated the purpose of cache_TEMPLATETYPE_fragment hook.

Right now the reason why HAML works is that they alias _hamlout.buffer to _erbout. Rails no longer hard codes _erbout, plus a low level variable trickery is not an API anyway.

Just to favor HAML into not making the right change, we are now making errors from templates not supporting cache less obvious. With a callback you get a message "template type does not support cache_something_fragment method", but with a variable you get a complaint about _erbout. One is actionable other is just obscure.

Seems reasonable to let HAML break and do the fix at their side.

Update of tcoppock's patch against r8787

I've updated tcoppock's patch against current trunk and verified that it works correctly. +1 from me, FTW. The patch also fixes a CI failure.

(In [8791]) Fix missing extension for caching. Closes #10733 [Catfish, tcoppock, mpalmer]

FWIW, recent changes in Haml's source work fine with this change.