This project is archived and is in readonly mode.

#1318 ✓resolved
Joel Chippindale

Fix for matches_dynamic_method? on ActionMailer

Reported by Joel Chippindale | November 3rd, 2008 @ 09:04 AM | in 2.x

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

Comments and changes to this ticket

  • Joel Chippindale

    Joel Chippindale November 3rd, 2008 @ 09:07 AM

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

  • Joel Chippindale

    Joel Chippindale November 3rd, 2008 @ 09:20 AM

    • Title changed from “[PATCH]: Make ActionMailer::Base.method_missing more precise” to “Make ActionMailer::Base.method_missing more precise”
  • Tom Lea

    Tom Lea November 3rd, 2008 @ 02:04 PM

    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)
    
  • Joel Chippindale

    Joel Chippindale November 5th, 2008 @ 10:05 AM

    • 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 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

  • James Mead

    James Mead November 11th, 2008 @ 01:43 PM

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

  • Repository

    Repository November 11th, 2008 @ 03:47 PM

    • 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 state:resolved]

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

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

<h2 style="font-size: 14px">Tickets have moved to Github</h2>

The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>

Referenced by

Pages