Adjust caching to use ActionView::Base#cache #67

Merged
merged 5 commits into from Oct 4, 2012

Conversation

Projects
None yet
3 participants
Contributor

rgarver commented Sep 25, 2012

In an attempt to leverage some of the Rails primitives better I took a stab at refactoring the #cache! method to use the ActionView caching system instead of the Rails caching system directly. The hope is that this will enable support for cache_digests. Based on my initial tests it already honors the controller.perform_caching which makes sense for this. Any feed back on code here would be appreciated since I needed to get a little nasty with the output buffer to make it work.

Contributor

rgarver commented Sep 26, 2012

Updated to leverage the great work by @rolftimmermans for 0.8.0. Still needed to do some monkey work to get things in the right formats, but the new helper that he introduced makes things much cleaner.

Contributor

rolftimmermans commented Sep 26, 2012

Completely agree with the enhancement, but I can't help wondering if it's possible to achieve without the serialization/deserialization wizardry. Isn't it possible to reuse the internal API that is used in ActionView::Helpers::CacheHelper#cache? We really just need to call fragment_name_with_digest and controller.read/write_fragment, right?

Contributor

rgarver commented Sep 26, 2012

The primary goal for going this route was to get cache_digest support. I wanted to avoid touching things too deep.

rails/cache_digests overrides ActionView::Helpers::CacheHelper#fragment_for which is used in ActionView::Helpers::CacheHelper#cache. We can work around it but we would then need to build cache_digests support custom. This approach keeps everything working the same even when the caching system is being modified in some way.

rails 4 is supposed to build cache_digests in so this might be moot.

Owner

dhh commented Sep 26, 2012

We can just make it not private. The power of the full stack :)

On Sep 26, 2012, at 6:23 PM, Rolf Timmermans wrote:

Completely agree with the enhancement, but I can't help wondering if it's possible to achieve without the serialization/deserialization wizardry. Isn't it possible to reuse the internal API that is used in ActionView::Helpers::CacheHelper#cache? We really just need to call fragment_name_with_digest, right? Shame it's private.


Reply to this email directly or view it on GitHub.

David Heinemeier Hansson

Contributor

rolftimmermans commented Sep 27, 2012

It's probably necessary to duplicate some logic because we're storing objects, not strings. So controller.read/write_fragment won't work for Jbuilder, but perhaps we can at least reuse fragment_name_with_digest if it is present. That would introduce cache digest support for Rails master. Let me know if you want help, @rgarver.

Contributor

rgarver commented Sep 27, 2012

Got it. That looks simple enough. I assume we'll want to patch rails to make that public. We'd probably also want to patch cache_digests to agree on the api since it inlines the fragment_name_with_digest function in to fragment_for.

Owner

dhh commented Sep 27, 2012

I can get both patches in if you guys do the work there.

On Sep 27, 2012, at 9:59 AM, Ryan Garver wrote:

Got it. That looks simple enough. I assume we'll want to patch rails to make that public. We'd probably also want to patch cache_digests to agree on the api since it inlines the fragment_name_with_digest function in to fragment_for.


Reply to this email directly or view it on GitHub.

David Heinemeier Hansson

Contributor

rgarver commented Sep 27, 2012

I've submitted patches. The cache_digests one might need to be brought inline with rails master a bit more, but I'm not sure what the motivations were that drove the divergence in the first place. Plus this isn't the place for that discussion.

See:
rails/cache_digests#5
rails/rails#7769

Owner

dhh commented Oct 4, 2012

The Rails one is the authoritative version. Please do submit PRs to bring them in line.

On Sep 27, 2012, at 6:05 PM, Ryan Garver wrote:

I've submitted patches. The cache_digests one might need to be brought inline with rails master a bit more, but I'm not sure what the motivations were that drove the divergence in the first place. Plus this isn't the place for that discussion.

See:
rails/cache_digests#5
rails/rails#7769


Reply to this email directly or view it on GitHub.

lib/jbuilder_template.rb
_scope { yield self }
end
_merge(value)
end
+
+ protected
+ def _cache_key(key)
@dhh

dhh Oct 4, 2012

Owner

Indent methods after the protected call.

lib/jbuilder_template.rb
- cache_key = ::ActiveSupport::Cache.expand_cache_key(key.is_a?(::Hash) ? url_for(key).split("://").last : key, :jbuilder)
- value = ::Rails.cache.fetch(cache_key, options) do
+ options[:force] = true unless @context.controller.perform_caching
+ value = ::Rails.cache.fetch(_cache_key(key), options) do
_scope { yield self }
end
_merge(value)
@dhh

dhh Oct 4, 2012

Owner

Carrier return above the _merge call.

Contributor

rgarver commented Oct 4, 2012

Code style updated

dhh added a commit that referenced this pull request Oct 4, 2012

Merge pull request #67 from rgarver/feature/support-cache-digests
Adjust caching to use ActionView::Base#cache

@dhh dhh merged commit e0d9acc into rails:master Oct 4, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment