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

Cached collections only work if there is one template #35253

Merged
merged 1 commit into from
Feb 13, 2019

Conversation

tenderlove
Copy link
Member

@tenderlove tenderlove commented Feb 13, 2019

I don't know this is the right fix necessarily.

When rendering a collection, each object in the collection can respond to to_partial_path. The template renderer will render the partial returned by that method. However, each object in the collection could return a different template. That means there isn't one template associated with the collection. Our collection template cache code depends on the collection using just one template. If you try to set cached: true on a collection that doesn't have just one template associated with it, it will raise an exception.

The patch in this PR fixes the exception, but doesn't actually cache the templates. Obviously, this use case isn't supported today, and it seems like nobody has noticed.

  1. I'm not sure how or if we should support caching collections like this
  2. If we decide not to support this, should it raise an exception?

@rails-bot rails-bot bot added the actionview label Feb 13, 2019
@rafaelfranca
Copy link
Member

I can't think in a way we can cache collections like this so I feel like raising an exception on this case is better than silently not caching it. At least the users can make the decision to use the same template if cache is important or remove the cache call that was not doing anything.

@tenderlove
Copy link
Member Author

I can't think in a way we can cache collections like this so I feel like raising an exception on this case is better than silently not caching it. At least the users can make the decision to use the same template if cache is important or remove the cache call that was not doing anything.

Cool. That was my feeling too, but I wanted to hear other opinions. I'll update this to raise a NotImplementedError.

Cached collections only work if there is one template.  If there are
more than one templates, the caching mechanism doesn't have a key.
@tenderlove tenderlove force-pushed the cached-collections-must-have-a-template branch from 6d37aee to bfcdd46 Compare February 13, 2019 18:35
@tenderlove tenderlove merged commit af6ade0 into master Feb 13, 2019
@tenderlove tenderlove deleted the cached-collections-must-have-a-template branch February 13, 2019 19:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants