This project is archived and is in readonly mode.

#2760 ✓resolved

polymorphic_url assumes a record is provided

Reported by Gus | June 4th, 2009 @ 08:02 PM | in 2.x

I have a very special case whereby I am building a polymorphic_url using only singleton resources. For example, if my routes were defined as such:

map.resource :person do |person|
  person.resource :preference

and I attempt to build a URL like so:

polymorphic_url([:person, :preference])

The method call that is generated would end up looking like person_preference_nil_class_path. This is because build_named_route_call in polymorphic_routes.rb assumes a record is provided to polymorphic_url.

Attached is a fix.

Comments and changes to this ticket

  • Michael Koziarski

    Michael Koziarski June 9th, 2009 @ 09:44 AM

    • State changed from “new” to “wontfix”

    if there's no record, then you don't need polymorphic_url

    You want person_preference_url

  • Gus

    Gus June 9th, 2009 @ 08:26 PM

    Normally I would agree. Using person_preference_url is specific and that would be what I would go to first.

    However, I am using a polymorphic url in this very specific instance for - what I consider to be - a good reason. And in using it, I expected my above case to work. It did not work and I considered that fact to be a bug. Being a bug, I sought to remedy it, which I did do :)

    I can't see any reason why this is not an appropriate fix given the documentation for polymorphic_url. Tests were written before making the change; all tests pass after making the change.

  • Michael Koziarski

    Michael Koziarski June 10th, 2009 @ 04:40 AM

    If the documentation implies you should be using polymorphic_url when there's no instances, nor any chance you'd be passing in an instance then that's what we should be fixing.

    But what's the good reason you have in mind for using it here? It's entirely possible I'm missing something ;)

  • Gus

    Gus June 10th, 2009 @ 01:42 PM

    Constructs a call to a named RESTful route for the given record and returns the resulting URL string

    The first third of that piece of documentation applies in this case, "Constructs a call to a named RESTful route". The second third would apply, "for the given record", if I were not using singleton resources. It seems illogical to me to say that polymorphic_url should handle the case foos_bar(bar) if foos is a collection resource, but not foo_bar when foo is a singleton resource. The point is to aid in the construction of a named route.

    My good reason is context sensitive and meaningful to my project. In generic terms, we have a controller that can be called through two different paths. It used to be that these paths where symmetrical in terms of the parameters required. We went through the process of turning one of root controllers for a path into singleton resource and found this bug.

    I honestly don't see why polymorphic_url should not handle this case.

  • Gus

    Gus June 10th, 2009 @ 10:30 PM

    How about I open up a patch to fix the documentation, then we can come back to this one?

  • Gus

    Gus June 11th, 2009 @ 12:33 AM

    Oh boy ... did a little bit of digging. I did a blame on polymorphic_routes, see:

    Notice line 166. Ok, if you check the commit message for the change of line 166 (and the other related lines), you find:

    To quote, the message says "Fixed polymorphic_url to be able to handle singleton resources". Well, it didn't handle the case I spoke of, therefore, my patch makes it better.

    Can we mark this as "Will fix" now?

  • Gus

    Gus June 11th, 2009 @ 12:37 AM

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

    Michael Koziarski June 11th, 2009 @ 12:56 AM

    • Assigned user changed from “Michael Koziarski” to “Jeremy Kemper”
    • State changed from “wontfix” to “new”

    Sure thing,

    Jeremy can take it though as I'm clearly too skeptical ;)

  • Gus

    Gus June 11th, 2009 @ 03:44 AM

    Woot! Thanks so much.

  • Slipp Douglas

    Slipp Douglas November 16th, 2009 @ 08:58 PM

    I agree. This fix would help app code consistency and help it follow "the principle of least surprise". ;-D

    A quick semi-fictional example of why this is needed:

    <% unless current_user.admin? %>
        <%= link_to "My Account", [current_user] %><%# works: generates "/users/1" %>
        <%= link_to "Logout", [:user_session], :method => :delete %><%# works: generates "/user_session" %>
    <% else %>
        <%= link_to "My Account", [:admin, current_user] %><%# works: generates "/admin/users/1" %>
        <%= link_to "Logout", [:admin, :user_session], :method => :delete %><%# breaks: "undefined method `admin_user_session_nil_class_path'" %>
    <% end %>
  • Dmitry V
  • José Valim

    José Valim January 3rd, 2010 @ 12:20 AM

    • State changed from “new” to “resolved”

    Both master and 2-3-stables have tests proving this works:

    So this is working or will in the next release.

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=""></a>