Patch review request: [BUG] hash_for_*_url does not accept ordered parameters as does the regular *_url named route helper

rubyonrails-core
1

Hi,

I was developing an app that uses hash_for_NAMED_ROUTE_url helper instead of the more common NAMED_ROUTE_url helper and found out the former doesn't work exactly as the latter.

I doesn't work with ordered parameters such as:

hash_for_user_post_path(@user, @post)

(I needed to use hash_for_user_post_path(:user_id=>@user, :id=>@post)

So I created a ticket:

http://dev.rubyonrails.org/ticket/11460

With a little hacking a managed to fix it and posted a patch there as well. It includes passing tests.

Could someone review it and give their "+1"s so it gets accepted?

Thanks,

Bernardo de Pádua

2

I think my request is getting lost in the middle of a zillion of "build failed, build fixed" messages, so I am "pinging" it.

Come on, guys, its is a really simple patch claiming for just a little love!

Cheers,

Bernardo

3

I was developing an app that uses hash_for_NAMED_ROUTE_url helper instead of the more common NAMED_ROUTE_url helper and found out the former doesn't work exactly as the latter.

I doesn't work with ordered parameters such as:

hash_for_user_post_path(@user, @post)

(I needed to use hash_for_user_post_path(:user_id=>@user, :id=>@post)

So I created a ticket:

http://dev.rubyonrails.org/ticket/11460

With a little hacking a managed to fix it and posted a patch there as well. It includes passing tests.

Could someone review it and give their "+1"s so it gets accepted?

I'm a little confused, why are you accessing those hash helpers? From my perspective they're just an implementation detail...

4

Well, it is documented, so...

But there is an actual reason. I was designing a helper that creates links and wanted the following interface:

  link_helper(optional_link_text, url, optional_html_options)

I will explain why I need the optional text in the end of the message (you can just trust I am not that lame or read the more detailed explanation below). I also don't want to change the order of the parameters not to confuse everyone that is already used to the link_to helper interface.

The problem is if I accept url to be a string (that *_url will generate), I will have a big problem trying to differentiate the parameters for the following hypothetical calls:

  link_helper(named_route_url, {:title=>'link!!'})   link_helper("My link optional text", {:controller=>'foo'})

See, the types are the same, so I would have to investigate the hash content to see if it is a url_options hash or a html_options hash. The code to only parse the parameters gets 10 times bigger than the actual helper (this is the price you pay for dynamic typing and no function overloading)... Too much for a simple helper.

So, I just took an easier path and decided to force url to be an url_options hash, so the parameters get really easy to parse. Using 'hash_for_*_url' instead of '*_url' seemed like a small price to pay (if it worked).

  link_helper(optional_string, url_hash_options, optional_html_options)

I am sure there must be other use cases for the hash_for_*_url. If the Rails Core team felt otherwise it shouldn't be exposed.

****** More details I actually created a menu/tab generator helper. In it, every link must have a unique identifier (actually a symbol), so it can be set as selected (and the corresponding tab maybe be highlighted using CSS) somewhere else in the view or controller. To reduce verbosity, if the link text is not provided, the identifier is 'titleized' and used instead. Some code excerpt:

<% tabs_for :menu_name do |tab| %>   <%= tab.home :controller =>'/user', :action => 'index' %>   <%= tab.profile "My profile", hash_for_user_profile_path(current_user) %>   <%= tab.contacts hash_for_contacts_path %>   <%= tab.jobs hash_for_jobs_path, {:title=>'Just showing off'} %> <% end %>

Cheers,

Bernardo

5

I am sure there must be other use cases for the hash_for_*_url. If the Rails Core team felt otherwise it shouldn't be exposed.

In your particular case you could also make it easier by using an options hash instead of postional arguments.

So link_helper("My link optional text", {:controller=>'foo'}) becomes

link_helper(:text=>"My link optional text", :url=>foo_url)

I'm still not seeing anything which justifies having these helpers be considered part of the public api, perhaps they should be 'undocumented' for 2.1?

6

I once built the hash_for methods into a project of mine because they showed up via ‘rake routes’ and I figured they’d be a shortcut. It took me a while to figure out where I’d gone wrong. So I’m for either undocumenting them or at the very least putting some documentation in that explains the specific cases where one might want to use them (if there are any).

::Jack Danger

7

Koz,

Yes, you are right, in my use case your solution is much better.

I think hash_for_*_url has been documented for quite a while and I bet it's been used in a lot of projects (a plugin that requires its usage: http://groups.google.com/group/rails-widgets/browse_thread/thread/d1ff75e9f5ff428e/63c148ddf3e89d36), I don't see a reason to deprecate it, it would just break stuff. And, if it stays, it should behave like *_url parameter-wise.

But I think there are other use cases that actually need hash_for_*_url (mine was just my motivation, I didn't quite needed it for what it was, just to make my life a little easier).

One that crosses my mind now is if you wanted to "inject" parameters in a given path or url depending on some context (or just manipulate it):

#route map.resources :users do |user|   user.resource :profile   user.resource :blog end

#some helper my_link_helper(text, url_hash) do   url_hash[:user_id] = "me" if url_hash[:user_id] == current_user.id end

So I could enforce nice urls to show the current user stuff: /users/me/profile instead of /users/132123123/profile

Or

#other helper user_stuff_link_helper(text, url_hash) do   url_hash[:user_id] = current_user unless url_hash[:user_id] end

To always send a given parameter depending on the context.

I actually think it would make a lot more sense for Rails to always have urls in the hash form (easy to manipulate) and only convert them to the string form in the last minute, but that is another story...

Cheers,

Bernardo