Lighthouse has a new layout. Prefer the old one? Return to the old layout, and switch back any time from the link at the top of each page.

This project is archived and is in readonly mode.

Fix for matches_dynamic_method? on ActionMailer

#1318

ActionMailer::Base.method_missing currently catches any method containing deliver or create instead of just those that begin with deliver or create.

For example

Mailer.deliver_template

has exactly the same result as

Mailer.any_random_string_before_deliver_template

The behaviour changed to be like this in commit 3cf773b187e803e16b8237e5923fa4c1139cde8a

Reported by Joel Chippindale · November 3rd, 2008 @ 09:04 AM

State: resolved
Milestone: 2.x
Assigned to: nobody
Importance: none

Activity

  1. Joel Chippindale
    Joel Chippindale

    The attached patch fixes this and makes Action::Mailer.method_missing just catch methods which begin with create and deliver

    November 3rd, 2008 @ 09:07 AM

  2. Joel Chippindale
    Joel Chippindale
    • Title changed from [PATCH]: Make ActionMailer::Base.method_missing more precise to Make ActionMailer::Base.method_missing more precise

    November 3rd, 2008 @ 09:20 AM

  3. Tom Lea
    Tom Lea

    Well spotted! I have a couple of enhancements to the patch that could be made.

    1. match = matches_dynamic_method?(method_symbol) looks clearly broken as of 3cf773b. This should be fixed separately, possibly in a separate commit, with another failing testcase (ex1 is a fairly ugly way of doing this).
    2. Commit notes should be referring to Prefix not Suffix ;)
    3. Having two else conditions with calls to super in a row does not scan well. How about using (ex2) at the top? It also gets rid of a layer of nesting, which is nice.

    ex1:

    
      def test_should_still_raise_exception_with_expected_message_when_calling_an_undefined_method
        begin
          RespondToMailer.not_a_method
          fail("Expected a NoMethodError.")
        rescue NoMethodError => e
          assert_match(/undefined method.*not_a_method/, e.message)
        rescue Exception => e
          fail("Expected a NoMethodError, got #{e.class}.")
        end
      end
    

    ex2:

    
    return super unless match = matches_dynamic_method?(method_symbol)
    

    November 3rd, 2008 @ 02:04 PM

  4. Joel Chippindale
    Joel Chippindale
    • Title changed from Make ActionMailer::Base.method_missing more precise to Fix for matches_dynamic_method? on ActionMailer

    Thanks for the suggestions Tom.

    1. As you suggested it seems like a good idea to split these two issues.

    I have created a separate ticket #1330 Fix for ActionMailer::Base.method_missing so that it raises NoMethodError whe... for the problem with method_missing not raising an exception correctly when an undefined method is called and included a test case for this (based on ex1 above).

    I removed the first call to super in this (because the line of code is never reached) but did not implement ex2 (above) because I think it is clearer to have an if/else statement in this case.

    1. Oops. Fixed

    A new patch is attached with just the fix for matches_dynamic_method in it

    November 5th, 2008 @ 10:05 AM

  5. James Mead
    James Mead

    +1 Joel, Tom - Thanks for fixing my original commit and sorry for introducing the bug. Cheers, James.

    November 11th, 2008 @ 01:43 PM

  6. Repository
    Repository
    • State changed from new to resolved

    (from [c65075feb6c4ce15582bc08411e6698d782249a7]) Fixed method_missing for ActionMailer so it no longer matches methods where deliver or create are not a suffix [#1318 Fix for matches_dynamic_method? on ActionMailer state:resolved]

    Signed-off-by: Joshua Peek josh@joshpeek.com http://github.com/rails/rails/co...

    November 11th, 2008 @ 03:47 PM