[PATCH] page caching shouldn't cache redirects

Perhaps only 200 OK should be cached. Please update with unit tests demonstrating the expected behavior.

I couldn't get to sleep because I can't stop thinking about this. How do you write a unit test for part of ActionController? I'd think you need a functional test. But then I think, how do you write a functional test of this without fixtures, routing, and at least one controller? I'm going to ask around the #rubyonrails irc channel but I might end up just having to write a small app and put it up on an svn for you guys to check out. I welcome your suggestions if you have any. I'm sure I'm missing something obvious since it's 3 AM here and I've been lying in bed for an hour thinking about this.

So sleepy. :)

Well, I plowed through the Action Pack unit tests. [I figured out that functional tests are really unit tests. Heh.] It seems the page caching was never really tested as far as actually creating files and writing data. The tests in /vendor/rails/actionpack/test/controller/caching_test.rb seem to merely test url rewriting. I'm not sure, though.

Anyhow... The testing environment doesn't define RAILS_ROOT which cause problems in the actual page caching. But that's another problem. [For some other day.] I just want to prove that my change works. I wrote a rather long and completely awesome unit test [included] which needs to be put into the /vendor/rails/actionpack/test/controller directory like all the other action pack tests. I've got a good bit of comments in the code so that should explain what's going on.

1. I had to manual set ActionController::Base.page_cache_directory because of the RAILS_ROOT problem.

2. I redefined ActionController::Caching::Pages.caching_allowed thusly:

def caching_allowed
  !request.post? && response.headers['Status'] && response.headers['Status'].to_i == 200
end

Everything runs awesome in the tests and now I have an even greater mastery of testing. I hadn't thought about all the redefining and things that this task made me think about. I'm pretty damned proud of this test suite.

I'm reopening the ticket and resetting the priority to high because this is [to me] a very serious need to not cache pages that are redirected. I mean no disrespect reopening what you guys closed so please don't take it that way.

Hope y'all are having a great day. It's time for me to get that sleep I've been missing.

Oh, yeah. I've also redone the patch to reflect the fact that you said that only 200 should be cached.

Unit tests for my patch's changes

I'd also suggest the part !request.post? be changed to request.get?

With the way the patch is now, 200 OK responses to DELETE and POST requests will be cached too, which is probably not what you would want. With the recent push for REST and Rails integration, this is a distinct possibility.

Replying to dkubb:

I'd also suggest the part !request.post? be changed to request.get? With the way the patch is now, 200 OK responses to DELETE and POST requests will be cached too, which is probably not what you would want. With the recent push for REST and Rails integration, this is a distinct possibility.

I completely agree. Updated that patch to "request.get?".

Updated again to cache only HTTP GET requests

(In [5755]) Only cache GET requests with a 200 OK response. Closes #6514, #6743.

(In [5756]) Merge [5755] from trunk. Closes #6514, closes #6743.