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

Use the lookup_context to find the correct template path #42437

Merged

Conversation

@HParker
Copy link
Contributor

@HParker HParker commented Jun 9, 2021

This replaces the controller/action method of finding a path with the lookup_context which should always find the same thing as the render method finds.

Closes: #42417

@HParker HParker force-pushed the digest-find-parent-controller-template branch from 9d2f41a to 3044fd7 Jun 9, 2021
@rails-bot rails-bot bot added the actionpack label Jun 9, 2021
This replaces the controller/action method of finding a path with the lookup_context which should always find the same thing as the render method finds.
@HParker HParker force-pushed the digest-find-parent-controller-template branch from 3044fd7 to 26259b0 Jun 9, 2021
zzak
zzak approved these changes Jun 10, 2021
@zzak zzak added the ready label Jun 10, 2021
@@ -44,7 +44,7 @@ def determine_template_etag(options)
# template digest from the ETag.
def pick_template_for_etag(options)
unless options[:template] == false
options[:template] || "#{controller_path}/#{action_name}"
options[:template] || lookup_context.find_all(action_name, _prefixes).first&.virtual_path
Copy link
Contributor

@lfalcao lfalcao Jun 10, 2021

Why not lookup_context.find(action_name, _prefixes).virtual_path ? 🤔

Copy link
Contributor Author

@HParker HParker Jun 10, 2021

find will raise an error if it doesn't find the template. I don't think we want to raise here since we didn't raise before. It fails a lot of actionpack tests that don't have templates but ask about the etag.

Maybe it would be worth switching to find? should we raise if it can't find the template you want as part of your etag? That would be new behavior, but might help people find problems with their cache keys? I am unsure.

Copy link
Member

@zzak zzak Jun 10, 2021

@HParker I would avoid trying to solve problems people aren't experiencing, this PR already adds enough value IMO ❤️

@rafaelfranca rafaelfranca merged commit ec69356 into rails:main Jun 23, 2021
4 checks passed
rafaelfranca added a commit that referenced this issue Jun 23, 2021
…template

Use the lookup_context to find the correct template path
@HParker HParker deleted the digest-find-parent-controller-template branch Jun 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants