[PATCH]content_for and fragment caching

Whoops that patch was for 3.0b1. I can probably make a similar one for 2.3.6 without much trouble, but... don't try to apply this to 2.3.6.

Seems a little unfortunate that there is no obvious way to edit or remove my previous reply.

OK maybe I should have let my mind simmer for awhile before I submitted that patch. I just thought of a small problem with the way I implemented the fragment content_for caching, and a good way I could DRY up the code a little bit. Stay tuned for a new patch in the next couple days... I'll also take a look at implementing this for Rails 2.3.6.

Whew! OK I'm making progress. I had a nightmarish merge because I (foolishly) started from the 3.0b1 tag when I was originally working on this, instead of starting from master.

I definitely made some major improvements over my previous attempt. It works great and tests pass, but I think I can do better. I'll sleep on it then post the new patch. No point in rushing it I guess, especially considering I appear to be the only one interested in this patch. ;)

Thanks,
Steve

I'm interested! :)

Take your time, though.

OK I have one failing test left. The problem I'm trying to fight through seems to be that in the testing environment, @controller.view_context always brings back a new object, so trying to assert content_for?(:foo) will of course not work, as it's transitory and will vanish the next time you call @controller.view_context

I guess I'll look at moving my fragment_for test with content_for to the view tests, but right now it's looking fairly confounding. Plus trying to patch Rails is like coding for a moving target, so it's proved an incredible time-sink: write the tests, writing the code to make them pass, rebase, make the tests pass again, rebase, make the tests pass again.... etc. It's like I'm stuck in an infinite loop. Even if I get them to pass and rebase again and everything still looks good and post a patch really quick, it will probably be almost immediately impossible to apply cleanly.

How do most people deal with this?

Also since I (foolishly) started my work on the 3.0b1 tag, it will probably be fairly easy to backport this enhancement to Rails 2.x. I'll work on that next since I don't see my company taking the time and effort to upgrade from 2.3.2 to 3 anytime soon.

I had to start over from scratch with master as my starting point in order to get the patch working properly, so by now I know this area of the Rails code fairly well. ;)

OK I've backported this to 2.3.2. Should I post that patch here or keep it off to avoid confusion? Does anyone other than me want it for 2.3.2?

@steve I reviewed the patch and would like to chat with you. Are you on IRC or GTalk?

OK! I was foolish enough to tell Yehuda that I was "just about done" like four days ago. Then I promptly fell into a super amateurish rabbit-hole. Anyway I finally came out the other end with this new, improved, updated patch for your consideration!

Caching content_for

• Assumes that the cache_store can store arbitrary ruby objects (or at least hashes with defaults defined).
  • IF this is not true, it will be necessary for "string-only" cache_stores to be enhanced to transparently serialize and de-serialize objects when they are written and read.
• For the "standard case" (ie, caching with content_for being unused), this will incur merely an extra if statement for action-caching and an extra block evaluation & new Hash being created and then destroyed in the case of fragment caching.
• Fragment caching is much cleaner than my previous patch
  • content_for keeps an additional Hash while it is evaluating the block to be cached, which is then cached in addition to the content itself. The pointer to the additional Hash is assigned to nil at the end of the block execution.
• Cache-store issues with possible falloff of related keys has been mitigated, relative to my previous patch.
  • Yehuda identified a potential problem with the fact that content_for was being stored separately from the content itself. This could cause the problem where the content itself still exists, so the associated block or action won't be re-executed, but the associated cached content_for could have been pushed out of the cache-store.
  • This has been solved by storing the content and the content_for stuff together in a hash.

Other thoughts

• It would be nice if the tests weren't so implementation specific, but rather tested the essence of the functionality. I tried to make them less brittle but failed.
• To that end, re-writing all the tests in rspec would be (IMHO) a brilliant improvement and pave the way for better tests in the future and more flexibility in implementation. I understand that this would be an enormous undertaking, but I figure that with like 10 people in the community working on different pieces, we could knock it out in a reasonable amount of time. Maybe we can coordinate something at RailsConf...

I don't think this is a high priority for anyone, but just in case people are wondering about the status of this -- here's an update:

I made my tests pass by working around a bug (controller.view_context is a new instance of the view_context every time you call it, so I memoized it in my tests). It seems to me that a simple fix for this would be:

# actionpack/lib/abstract_controller/rendering.rb: line 84
def view_context
  @template ||= view_context_class.new(lookup_context, view_assigns, self)
end

A little bonus would be that if legacy Rails apps used the @template instance variable, this might still work for them (assuming that view_context had been called prior to them trying to use it, of course). I'm guessing that there's some reason this wouldn't be ideal, but I'm not sure what that reason is...

I did a spot check of the cache stores implemented, and I found that they appear to support arbitrary Ruby objects (by utilizing Marshal), so that should be OK.

I am just stopping by to say thanks for working on this and that I really hope this makes it into rails 3. It's fairly important imho.

agreed, looking forward to this making it into rails

OK I cleaned up the patch a bit (removed vestige of old approach, squashed my commits, and removed the "def view_context" stuff from the caching_test.rb). The removal of the "def view_context" stuff means that my patch won't work (and accordingly, 2 tests fail), as it depends on view_context being the same object before, during and after rendering.

To address that, I have also attached a patch that memoizes the view_context in a @template variable (as shown in my last post). Of course, memoizing the view_context causes 2 other tests to fail. Go figure.

@James Hayton -- I honestly haven't tried to apply my patch to the latest Rails3 code base. I'll give it a shot and resolve conflicts/fix issues that come up, then try to reconnect with Yehuda about it...

Yehuda mentioned it may not make it into Rails 3 till a point release or something.

@Steve, have you tested this patch with the Rails3 release? Could you post the latest patch? I'd be happy to help out with this, but i'd rather not start from scratch. Thanks.

@Zach: honestly, I haven't touched this stuff for months. My 9-5 job that pays the bills has been taking all my time & energy recently, and realistically, I don't see myself being able to make time for this till next week at the earliest.

I'll try to get to it sooner, but it doesn't look good. FWIW, Yehuda said we can target 3.1 for this.

@Steve: Thanks for the updates. Would love for this to happen. Thanks for all the work. Totally understand how things go.

Allow me to add my encouragement on getting this in too, @Steve. Just ran into this problem myself and will work around it less elegantly for now.

Joost, James, Zach -- thanks. Your encouragement spurs me onward. :)

What's the latest status for this? I've run into this problem making it impossible for me to cache certain items which I'd really like cached. The problem most of the time is that we use content_for to send javascript code to the bottom of the dom.

Will this be included in 3.0.2? Is it testable/usable today?

+1... also eagerly awaiting this!

Any updates here?

Actually, yes!

I've been working on it all this past week. In short, I think I'm going to separate the fragment caching and the action caching support for content_for into two patches. The reason for this is that the fragment part is pretty much done and working, and while I was able to get action caching working with content_for as well, 149 tests in action_pack broke afterwards(!).

@Daniel -- it's not testable nor usable yet. I'll post a patch soon that can cleanly apply to current-ish HEAD.

Hah! Just got them down to 10 failures. Patch may be forthcoming... :)

[Action pack] tests are green! Feast your eyes on the patch that should apply cleanly to current HEAD.

It's a bit ugly, but it works. I have an idea for refactoring to something a little less disgusting, but for now... this is what I got. Feel free to check it out and give me your suggestions for making it prettier.

What does it mean when it says that this story is on hold?

Since the patch passes all the tests shouldn't it included in the next release of rails? Still waiting eagerly for this.

Getting content_for to work with caches_action might get me to update to rails3 ...
unless there's a backport somewhere.

Can someone give an update on this? Would love to know if it will make it to 3.1? Really really useful feature imho.

@James: I think that making action caching (when you're not caching the template) work with content_for might be a bit too heavy for the Rails team to accept. I'll pull out fragment caching and make it a separate patch since it's much smaller and more lightweight IMO. After that, if no rails core team member will review it and accept it or even reject it... I think that will be the end of this (as far as I'm concerned).

@Steve: I just wish they would at least review it and give an answer one way or the other. I can't believe that there aren't more people clamoring for this. I use action caching extensively with layout => false and this would really help me dry up my views.

For those who need this function now:
I just make a monkey patch for rails 3 based on Steve's great work, and leave the abstract_controller untouched.
The patch is here: https://gist.github.com/891895, all you have to do is to put it in your config/initializers directory.

I'm very curious why abstract_controller.view_context returns a new instance instead of a memorized one like @template, too.
Could anyone point this out?

Thank you so much for this. We really wanted to start using action caching with memcache, it was a no go without this patch.