Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes an issue where cache sweepers with only after filters would have no controller object #1798

Merged
merged 2 commits into from
Jun 22, 2011

Conversation

jeroenj
Copy link

@jeroenj jeroenj commented Jun 21, 2011

It would raise undefined method controller_name for nil while trying to reach controllers where the sweeper was defined.

I do not have tests for this yet...

…e no controller object

It would raise undefined method controller_name for nil
@spastorino
Copy link
Contributor

Everything should be tested ;), we will wait for tests in order to review it.

@jeroenj
Copy link
Author

jeroenj commented Jun 22, 2011

It appears that the cache_sweeper isn't tested, or am I overlooking something?

@dmathieu
Copy link
Contributor

@jeroenj
Copy link
Author

jeroenj commented Jun 22, 2011

Right. Ignore my previous comment... :)

@jeroenj
Copy link
Author

jeroenj commented Jun 22, 2011

Since I'm not sure if this is the best test, I didn't commit it yet but here it is:

def test_after_method_of_sweeper_should_always_return_nil
  sweeper = ActionController::Caching::Sweeper.send(:new)
  assert_nil sweeper.after(TestController.new)
end

If you run the test snippet above, and you aren't using the fix in my previous commit, you'll get the following exception:

NoMethodError: undefined method `controller_name' for nil:NilClass

You can reproduce this by only using an after filter in a cache sweeper. If you don't define a before filter, it will never set self.controller in the sweeper, which causes it to crash in the private callback method in the Sweeper class.

@dmathieu
Copy link
Contributor

It seems fine to me.

spastorino added a commit that referenced this pull request Jun 22, 2011
Fixes an issue where cache sweepers with only after filters would have no controller object
@spastorino spastorino merged commit 6b3342d into rails:master Jun 22, 2011
@spastorino
Copy link
Contributor

I guess we should add this to 3-1-stable too, can you provide a backport?

@jeroenj
Copy link
Author

jeroenj commented Jun 23, 2011

I encountered this issue in Rails 3.0.x actually. As far as I can see it's compatible with rails-3.0, rails-3-1 and master.

@spastorino
Copy link
Contributor

provide a backport to 3-1-stable so, thanks :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants