This project is archived and is in readonly mode.
[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
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 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 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 October 31st, 2008 @ 05:43 PM
- Assigned user set to “Michael Koziarski”
-
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 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 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 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 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 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>
DHH
Michael Koziarski
Tom Lea