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

Make ActionMailer #cache helper a no-op, not an exception #19941

Merged
merged 1 commit into from Apr 28, 2015

Conversation

@javan
Copy link
Member

@javan javan commented Apr 28, 2015

ActionView's cache helper checks controller.perform_caching which is only defined in ActionController so views of an AbstractController (like ActionMailer's) will raise an exception:

$ ruby test/mail_helper_test.rb
Run options: --seed 21612

# Running:

.E.....

Finished in 0.217102s, 32.2429 runs/s, 55.2736 assertions/s.

  1) Error:
MailerHelperTest#test_use_cache:
ActionView::Template::Error: undefined method `perform_caching' for #<HelperMailer:0x007fa2cfa2cd20>
    /Users/javan/Code/rails/actionview/lib/action_view/helpers/cache_helper.rb:137:in `cache'
    inline template:1:in `_inline_template___584781172439821615_70168607863040'

(also noted in #17657)

This change fixes the exception, allowing controller views to be more easily shared with mailers. It doesn't implement mailer caching (a meatier potential change).

@@ -134,7 +134,7 @@ module CacheHelper
#
# <%= render @notifications, cache: false %>
def cache(name = {}, options = nil, &block)
if controller.perform_caching
if controller.try(:perform_caching)

This comment has been minimized.

@rafaelfranca

rafaelfranca Apr 28, 2015
Member

We avoid to use try inside the framework and since we are expecting controller to always be defined can you change to a respond_to? check?

This comment has been minimized.

@dhh

dhh Apr 28, 2015
Member

#try can also be used for it's secondary effect of "receiver is there but may or may not respond to the message". Seems like legit usage here.

This comment has been minimized.

@rafaelfranca

rafaelfranca Apr 28, 2015
Member

Yeah, but if for some case receiver is not there (and it should always be) we will not catch the problem.

This comment has been minimized.

@dhh

dhh Apr 28, 2015
Member

We will also not catch it with the new code. Nil#respond_to? is there as
well.

On Tue, Apr 28, 2015 at 9:58 PM, Rafael Mendonça França <
notifications@github.com> wrote:

In actionview/lib/action_view/helpers/cache_helper.rb
#19941 (comment):

@@ -134,7 +134,7 @@ module CacheHelper
#
# <%= render @notifications, cache: false %>
def cache(name = {}, options = nil, &block)

  •    if controller.perform_caching
    
  •    if controller.try(:perform_caching)
    

Yeah, but if for some case receiver is not there (and it should always be)
we will not catch the problem.


Reply to this email directly or view it on GitHub
https://github.com/rails/rails/pull/19941/files#r29281815.

This comment has been minimized.

@rafaelfranca

rafaelfranca Apr 28, 2015
Member

duh! You are completely right. I must be crazy. And after searching the entire codebase I saw some usage of try, so I think there is no need to avoid to use it.

@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Apr 28, 2015

Cool! Thank you so much for the pull request. I did a minor comment about our code conventions.

@javan javan force-pushed the javan:actionmailer-cache-noop branch from c3d1fe1 to 20f6f64 Apr 28, 2015
rafaelfranca added a commit that referenced this pull request Apr 28, 2015
Make ActionMailer #cache helper a no-op, not an exception
@rafaelfranca rafaelfranca merged commit f4b8b58 into rails:master Apr 28, 2015
@javan
Copy link
Member Author

@javan javan commented Apr 28, 2015

Updated with respond_to?, rebased, squashed, etc. Thanks. :)

@dhh
Copy link
Member

@dhh dhh commented Apr 28, 2015

I think the new version reads worse, to be honest. But no biggie.

@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Apr 28, 2015

Yeah, I think try reads better, but inside the framework the usage of try in places where we always expected the receiver to be there already give us problem. It is a matter of be more restrictive about what we are expecting.

rafaelfranca added a commit that referenced this pull request Apr 28, 2015
Make ActionMailer #cache helper a no-op, not an exception
rafaelfranca added a commit that referenced this pull request Apr 28, 2015
Make ActionMailer #cache helper a no-op, not an exception
@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Apr 28, 2015

I'm considering this a bug fix so backported as e7a265c and cddb156

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.

None yet

3 participants