route url defaults take precedence over controller params/url_options #7144

Closed
the8472 opened this Issue Jul 24, 2012 · 18 comments

Comments

Projects
None yet
@the8472

the8472 commented Jul 24, 2012

Using 3.2.6

#routes.rb
match "/test(/:foo)" => "my_controller#index", :defaults => {:foo => "my_default"}, :as => :test_index
match ":id/test(/:foo)" => "my_controller#show", :defaults => {:foo => "my_default"}, :as => :test_show
# my_controller.rb

# this shouldn't even be necessary since url_for takes its context from the controller anyway
def url_options
   super.merge(:foo => params[:foo])
end

load /test/something-thats-not-default

-# index.html.haml

=link_to "link text", test_show_path(some_object)

Resulting link goes to 1/test/ instead of 1/test/something-thats-not-default

This works on the other hand, but seems verbose as i have to drag along parameters everywhere:

 =link_to "link text", test_show_path(some_object, :foo => params[:foo])
@kytrinyx

This comment has been minimized.

Show comment
Hide comment
@kytrinyx

kytrinyx Jul 29, 2012

Contributor

Resulting link goes to 1/test/ instead of 1/test/something-thats-not-default

I'm not sure how =link_to "link text", test_show_path(some_object) can be expected to link to 1/test/something-thats-not-default, since you're creating a link, and you're not giving it a value for foo.

Could you point me to the spot in the documentation that suggests that link_to would pass url_options into the named url method?

Contributor

kytrinyx commented Jul 29, 2012

Resulting link goes to 1/test/ instead of 1/test/something-thats-not-default

I'm not sure how =link_to "link text", test_show_path(some_object) can be expected to link to 1/test/something-thats-not-default, since you're creating a link, and you're not giving it a value for foo.

Could you point me to the spot in the documentation that suggests that link_to would pass url_options into the named url method?

@the8472

This comment has been minimized.

Show comment
Hide comment
@the8472

the8472 Jul 30, 2012

http://api.rubyonrails.org/classes/ActionView/Helpers/UrlHelper.html#method-i-url_options

url_options work just fine if no defaults are specified in the routes.

the8472 commented Jul 30, 2012

http://api.rubyonrails.org/classes/ActionView/Helpers/UrlHelper.html#method-i-url_options

url_options work just fine if no defaults are specified in the routes.

@kytrinyx

This comment has been minimized.

Show comment
Hide comment
@kytrinyx

kytrinyx Jul 30, 2012

Contributor

I don't see anything there which suggests that url_for will call url_options for you.

url_options work just fine if no defaults are specified in the routes

You mean that if you change your routes.rb file to remove defaults, this works? I.e:

match "/test(/:foo)" => "my_controller#index", :as => :test_index
match ":id/test(/:foo)" => "my_controller#show", :as => :test_show
Contributor

kytrinyx commented Jul 30, 2012

I don't see anything there which suggests that url_for will call url_options for you.

url_options work just fine if no defaults are specified in the routes

You mean that if you change your routes.rb file to remove defaults, this works? I.e:

match "/test(/:foo)" => "my_controller#index", :as => :test_index
match ":id/test(/:foo)" => "my_controller#show", :as => :test_show
@the8472

This comment has been minimized.

Show comment
Hide comment
@the8472

the8472 Jul 30, 2012

I don't see anything there which suggests that url_for will call url_options for you.

It does.

Here, a puts caller straight from the url_options method in the controller:

/home/private/.rvm/gems/ruby-1.9.3-p194@project/gems/actionpack-3.2.6/lib/action_view/helpers/url_helper.rb:37:in `url_options'
/home/private/.rvm/gems/ruby-1.9.3-p194@project/gems/actionpack-3.2.6/lib/action_dispatch/routing/url_for.rb:148:in `url_for'
/home/private/.rvm/gems/ruby-1.9.3-p194@project/gems/actionpack-3.2.6/lib/action_view/helpers/url_helper.rb:107:in `url_for'
/home/private/.rvm/gems/ruby-1.9.3-p194@project/gems/actionpack-3.2.6/lib/action_dispatch/routing/route_set.rb:213:in `job_portal_publications_path'
/home/private/workspace/project/vendor/core-extensions/job_portal/app/views/project/job_portal/publications/index.html.haml:22:in `__home_private_workspace_project_vendor_core_extensions_job_portal_app_views_project_job_portal_publications_index_html_haml___1805346634538105884_90943260'

And other people use it too: http://stackoverflow.com/questions/6100954/default-url-options-and-rails-3

the8472 commented Jul 30, 2012

I don't see anything there which suggests that url_for will call url_options for you.

It does.

Here, a puts caller straight from the url_options method in the controller:

/home/private/.rvm/gems/ruby-1.9.3-p194@project/gems/actionpack-3.2.6/lib/action_view/helpers/url_helper.rb:37:in `url_options'
/home/private/.rvm/gems/ruby-1.9.3-p194@project/gems/actionpack-3.2.6/lib/action_dispatch/routing/url_for.rb:148:in `url_for'
/home/private/.rvm/gems/ruby-1.9.3-p194@project/gems/actionpack-3.2.6/lib/action_view/helpers/url_helper.rb:107:in `url_for'
/home/private/.rvm/gems/ruby-1.9.3-p194@project/gems/actionpack-3.2.6/lib/action_dispatch/routing/route_set.rb:213:in `job_portal_publications_path'
/home/private/workspace/project/vendor/core-extensions/job_portal/app/views/project/job_portal/publications/index.html.haml:22:in `__home_private_workspace_project_vendor_core_extensions_job_portal_app_views_project_job_portal_publications_index_html_haml___1805346634538105884_90943260'

And other people use it too: http://stackoverflow.com/questions/6100954/default-url-options-and-rails-3

@kytrinyx

This comment has been minimized.

Show comment
Hide comment
@kytrinyx

kytrinyx Jul 30, 2012

Contributor

Ok, thanks for patiently walking me through this.

I've made a sample app so that I can see this happening. I think it is the reverse_merge! in this line that is causing this behavior:

https://github.com/rails/rails/blob/master/actionpack/lib/action_dispatch/routing/url_for.rb#L148

The options that come into the method are the defaults (:foo => 'my_default'), and the url_options contains the override from the controller (:foo => 'something-thats-not-default'). Since this is a reverse merge rather than a merge, the defaults from options are overwriting any identical keys from url_options.

A lot of people have worked on this method, I'm not sure who knows what the intended behavior is. @wycats, is this familiar territory for you?

Contributor

kytrinyx commented Jul 30, 2012

Ok, thanks for patiently walking me through this.

I've made a sample app so that I can see this happening. I think it is the reverse_merge! in this line that is causing this behavior:

https://github.com/rails/rails/blob/master/actionpack/lib/action_dispatch/routing/url_for.rb#L148

The options that come into the method are the defaults (:foo => 'my_default'), and the url_options contains the override from the controller (:foo => 'something-thats-not-default'). Since this is a reverse merge rather than a merge, the defaults from options are overwriting any identical keys from url_options.

A lot of people have worked on this method, I'm not sure who knows what the intended behavior is. @wycats, is this familiar territory for you?

@timraymond

This comment has been minimized.

Show comment
Hide comment
@timraymond

timraymond Dec 12, 2012

Contributor

5 months, is this still an issue?

Contributor

timraymond commented Dec 12, 2012

5 months, is this still an issue?

@ghost ghost assigned pixeltrix Dec 14, 2012

@Flauschbaellchen

This comment has been minimized.

Show comment
Hide comment
@Flauschbaellchen

Flauschbaellchen Dec 17, 2012

It seems it is, I've just stumbled over it in my own app

It seems it is, I've just stumbled over it in my own app

@timraymond

This comment has been minimized.

Show comment
Hide comment
@timraymond

timraymond Dec 19, 2012

Contributor

Okay, thanks for confirming. Are you also using 3.2.6 or a different version?

Contributor

timraymond commented Dec 19, 2012

Okay, thanks for confirming. Are you also using 3.2.6 or a different version?

@Flauschbaellchen

This comment has been minimized.

Show comment
Hide comment
@Flauschbaellchen

Flauschbaellchen Dec 19, 2012

Sorry I didn't mention it.
I'm using rails 3.2.9/actionpack 3.2.9.

Sorry I didn't mention it.
I'm using rails 3.2.9/actionpack 3.2.9.

@bundacia

This comment has been minimized.

Show comment
Hide comment
@bundacia

bundacia Aug 8, 2013

Just hit this issue myself (on rails 3.2.13). Is there a good workaround? Or a timeline for getting this fixed?

bundacia commented Aug 8, 2013

Just hit this issue myself (on rails 3.2.13). Is there a good workaround? Or a timeline for getting this fixed?

@Govinda-Fichtner

This comment has been minimized.

Show comment
Hide comment
@Govinda-Fichtner

Govinda-Fichtner Aug 26, 2013

Is this still an issue? I think I also hit this problem...

Is this still an issue? I think I also hit this problem...

@njakobsen

This comment has been minimized.

Show comment
Hide comment
@njakobsen

njakobsen Feb 7, 2014

Contributor

I'm having this issue in Rails 4.0.2.

# routes.rb
namespace :staff do
  resources :stores, :only => [:update] do
    get :edit_layout, :on => :member, :defaults => {:section => 'general'}
  end
end

# in my view
url_for(:section => 'accounts') # => http://localhost:3000/staff/stores/1/edit_layout

Note the section param is not present in the url.

Contributor

njakobsen commented Feb 7, 2014

I'm having this issue in Rails 4.0.2.

# routes.rb
namespace :staff do
  resources :stores, :only => [:update] do
    get :edit_layout, :on => :member, :defaults => {:section => 'general'}
  end
end

# in my view
url_for(:section => 'accounts') # => http://localhost:3000/staff/stores/1/edit_layout

Note the section param is not present in the url.

@raphaelcosta

This comment has been minimized.

Show comment
Hide comment
@raphaelcosta

raphaelcosta Mar 27, 2014

I'm having this issue too in Rails 4.1.0.rc1, you guys know any workaround for that?

I'm having this issue too in Rails 4.1.0.rc1, you guys know any workaround for that?

@pixeltrix

This comment has been minimized.

Show comment
Hide comment
@pixeltrix

pixeltrix Apr 19, 2014

Member

Closing for the reasons outlined in this comment

Member

pixeltrix commented Apr 19, 2014

Closing for the reasons outlined in this comment

@pixeltrix pixeltrix closed this Apr 19, 2014

@the8472

This comment has been minimized.

Show comment
Hide comment
@the8472

the8472 Apr 19, 2014

The linked comment refers to the distinction of query vs. path parameters, but my example above is about path parameter defaults, not query ones.

the8472 commented Apr 19, 2014

The linked comment refers to the distinction of query vs. path parameters, but my example above is about path parameter defaults, not query ones.

@pixeltrix

This comment has been minimized.

Show comment
Hide comment
@pixeltrix

pixeltrix Apr 20, 2014

Member

@the8472 okay, I'll take another look - the PR was using query parameters as an example

Member

pixeltrix commented Apr 20, 2014

@the8472 okay, I'll take another look - the PR was using query parameters as an example

@pixeltrix pixeltrix reopened this Apr 20, 2014

@robin850 robin850 added this to the 4.0.6 milestone Apr 25, 2014

@kuldeepaggarwal

This comment has been minimized.

Show comment
Hide comment
@kuldeepaggarwal

kuldeepaggarwal Jun 14, 2014

Contributor

@pixeltrix any updates? Is this still an issue?

Contributor

kuldeepaggarwal commented Jun 14, 2014

@pixeltrix any updates? Is this still an issue?

@pixeltrix

This comment has been minimized.

Show comment
Hide comment
@pixeltrix

pixeltrix Jun 15, 2014

Member

@kuldeepaggarwal I'm going to close this as it appears to work as expected. The named url helpers explicitly use the route defaults unless overridden by arguments passed to it. I think what @the8472 was thinking is that they should use recall parameters (i.e. params from the current request) to override the defaults in the url helper but this would likely to cause problems as it would pick up other things as well like controller, action, id, etc. Using url_for with an options hash may work for what they want but it's unclear from the original example - typically you'd use it when switching from one action to another on an object, e.g:

def show
  redirect_to action: :edit
end

Here url_for would use the current request's params as input to the url (including :foo in @the8472's case) and then overwrite the :action parameter.

Member

pixeltrix commented Jun 15, 2014

@kuldeepaggarwal I'm going to close this as it appears to work as expected. The named url helpers explicitly use the route defaults unless overridden by arguments passed to it. I think what @the8472 was thinking is that they should use recall parameters (i.e. params from the current request) to override the defaults in the url helper but this would likely to cause problems as it would pick up other things as well like controller, action, id, etc. Using url_for with an options hash may work for what they want but it's unclear from the original example - typically you'd use it when switching from one action to another on an object, e.g:

def show
  redirect_to action: :edit
end

Here url_for would use the current request's params as input to the url (including :foo in @the8472's case) and then overwrite the :action parameter.

@pixeltrix pixeltrix closed this Jun 15, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment