This project is archived and is in readonly mode.

#1301 ✓committed
Tom Lea

[PATCH] Named Routes in ActionView Scope Don't Respect AC::Base#default_url_options

Reported by Tom Lea | October 30th, 2008 @ 06:09 PM

As discussed on the mailing list:

I have added a test case [1] in url_helper_test.rb (1st commit), that replicates a scenario we are seeing in production.

This fails [2], while similar test (in the second commit), hitting the controller directly seem to work ok.

This seems to be caused by the ActionController::Routing::Optimisation module.

The 3rd and final commit fixes the issue, does anyone know of a better way to get the test to pass, or are we going to have to just not optimize the *_url named routes (as the patch does)?

Thanks in advance - Tom

[1]


  def
test_named_route_should_show_host_and_path_using_controller_default_url_options
    class << @controller
      def default_url_options(options = nil)
        {:host => 'testtwo.host'}
      end
    end

    with_url_helper_routing do
      get :show_named_route, :kind => 'url'
      assert_equal 'http://testtwo.host/url_helper_with_controller/show_named_route'
, @response.body
    end
  end

[2] Example Failure:


 1) Failure:
test_named_route_should_show_host_and_path_using_controller_default_url_options(UrlHelperWithControllerTest)
    [./test/template/url_helper_test.rb:392:in `test_named_route_should_show_host_and_path_using_controller_default_url_options'
     ./test/template/url_helper_test.rb:417:in `with_url_helper_routing'
     ./test/../lib/action_controller/test_process.rb:514:in `with_routing'
     ./test/template/url_helper_test.rb:412:in `with_url_helper_routing'
     ./test/template/url_helper_test.rb:390:in `test_named_route_should_show_host_and_path_using_controller_default_url_options'
     ./test/../../activesupport/lib/active_support/testing/setup_and_teardown.rb:94:in `__send__'
     ./test/../../activesupport/lib/active_support/testing/setup_and_teardown.rb:94:in `run']:
<"http://testtwo.host/url_helper_with_controller/show_named_route"> expected but was
<"http://test.host/url_helper_with_controller/show_named_route">.

Comments and changes to this ticket

  • Michael Koziarski

    Michael Koziarski October 30th, 2008 @ 06:26 PM

    OK, that fix is way too pessimistic. Turns off the optimisations for everything :)

    Given that the model already does a check on defined?(default_url_options) I'm a little confused why it's not working for you.

    the best bet is probably to either use a debugger, or logging, and see why / if the defined?(... guards are functioning correctly.

  • Tom Lea

    Tom Lea October 31st, 2008 @ 02:26 PM

    Koz: The guard conditions don't fire because default_url_options is not defined on the action view.

    The patch should just disable _url methods and not _path methods... but I agree, this does not feel ideal.

    The only other alternative I can see is to add a default_url_options method to the action view (patch attached). I was initially reluctant to add the method directly to ActionView, as the only reason it is needed is because of the optimizations.

    The named routes are created on the AV::Base at the same time as they are defined on the AC::Base, but AV::Base has no knowledge of default_url_options, before the optimization we would just ask the controller what to do.

    So the third way, would be to ensure that AV::Base#foo_url would delegate to @controller#foo_url. This may be the cleanest option.

    What are your thoughts?

    If the third option is going to be preferred, let me know and I'll try and get a patch together tonight.

  • DHH

    DHH October 31st, 2008 @ 05:43 PM

    • Assigned user set to “Michael Koziarski”
  • Tom Lea

    Tom Lea October 31st, 2008 @ 08:08 PM

    I got bored, so here is a possible implementation of delegating named routes away to the controller.

    Patch attached.

  • Michael Koziarski

    Michael Koziarski November 1st, 2008 @ 11:19 AM

    • Milestone cleared.

    Sorry for the slow responses on this, been a little snowed under.

    So from my understanding, the url_for in ActionView will fallback to the controller's default_url_options. Because of this the faster routes guard conditions don't fire, and the methods return the wrong values.

    Does adding helper_method :default_url_options in your controller fix the issue?

  • Tom Lea

    Tom Lea November 1st, 2008 @ 04:46 PM

    So from my understanding, the url_for in ActionView will fallback to the controller's default_url_options. Because of this the faster routes guard conditions don't fire, and the methods return the wrong values.

    Correct, that seems to be what I am seeing in testing and production.

    Does adding helper_method :default_url_options in your controller fix the issue?

    Indeed it does, but it doesn't feel quite right.

    How would you feel about extending the guard conditions to include defined?(@controller.default_url_options) && !@controller.default_url_options.empty? amongst the rest.

    (once again, patch attached, it seems I just love patches)

  • Michael Koziarski

    Michael Koziarski November 1st, 2008 @ 04:49 PM

    OK, This looks good to me, but could you tidy up those guard conditions. They're 99% the same now so should be defined in one place with the differences getting shifted on the end.

  • Tom Lea

    Tom Lea November 1st, 2008 @ 06:00 PM

    Right, went a little further, built up an array and then &&'d them, felt more readable, all the requirements with a line each.

    Final merged + rebased patch attached.

    Thanks for the help.

  • Michael Koziarski

    Michael Koziarski November 11th, 2008 @ 01:35 PM

    • State changed from “new” to “committed”

    applied ages ago

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>

People watching this ticket

Pages