Rails Ruby on Rails → 2.x

Comments and changes to this ticket

• 

  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

• You flagged this item as spam.
  

  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 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 ;)

• You flagged this item as spam.
  

  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.

• You flagged this item as spam.
  

  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?

• You flagged this item as spam.
  

  Gus June 11th, 2009 @ 12:33 AM

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

  http://github.com/rails/rails/blame/47ff57f6d14fe161900bf85e2d2cf6d...

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

  http://github.com/rails/rails/commit/bb6e8eea5a8190aaab67da0a7efedb...

  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?

• You flagged this item as spam.
  

  Gus June 11th, 2009 @ 12:37 AM

  • Assigned user set to “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 ;)

• You flagged this item as spam.
  

  Gus June 11th, 2009 @ 03:44 AM

  Woot! Thanks so much.

• You flagged this item as spam.
  

  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 %>
  

• You flagged this item as spam.
  

  Dmitry V December 25th, 2009 @ 03:16 PM

  i second this

• 

  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:

  http://github.com/rails/rails/blob/master/actionpack/test/activerec...

  http://github.com/rails/rails/blob/2-3-stable/actionpack/test/contr...

  So this is working or will in the next release.