Skip to content
This repository

link_to / form_for doesn't work for singular resource #1769

Open
Godisemo opened this Issue June 19, 2011 · 46 comments
Godisemo

In my routes I have

resource :company

and in my views

<% form_for(@store) do |f| %>

<% end %>

When

new action gets an error when rendering

undefined method `hash_for_companies_path' for #<Module:0x00000102bfd3f0>

The stack trace reviels that the problem occurs in

lib/action_dispatch/routing/polymorphic_routes.rb:133:in `polymorphic_url'

edit action can render without errors but the forms url is /company.4 where 4 is the id of the company.

This seems to be a bug that has been around for a very long time so I think it's time to fix it.

Andrew White
Owner

You're right the error has been around for a long while however a clean solution doesn't readily represent itself. The polymorphic_url helper has no 'intelligence' in how it operates - it has no information about what you've declared as resources in your routes.rb. All it has to go on it the name of the model and how that maps to the named url helpers.

In your case when passing a new Company instance it tries to map to the plural path as that's the standard route for the create action. The question is how to determine a singular resource from a regular resource - suggestions are welcome. The best solution I can think of is to try a respond_to? first and if it doesn't then try the singular option - I'll take a look at it.

Corin Langosch
gucki commented July 18, 2011

Ah, didn't know there's already a ticket for htis. So here's my post from the mailing list:

What about making the polymorphic routing in rails to try a plural route first. If it fails, try a singleton route. If it fails, raise.

This is not the nicest and most performant way (the result could be cached in production so it doesn't really matter in the end) but I guess it should solve most cases. Or is this too much magic?

Alexander Uvarov

I am wondering why this ticket pointed to 3.2 milestone?

Andrew White
Owner

Because it’s not a blocker for the 3.1 release and it may require too many changes to go into a patch release.

Mike Ni

i think this bug should be written in rails guides somewhere.

for anyone using a singular resource, they will eventually find themselves on this bug page. better to announce first

Maxime Garcia

Still occurring.

Mike Ni

for now i just use scope =(

Ben Turner

Yep, my first time with the singular resource form, and I walked smack into this bug. Would be great to see a fix in an upcoming version - don't see any target milestone, so I presume it's not under active development at the moment ?

kjakub

Yep, here too, my first time with singular resource, lost 3h coz i was blaming myself like a first

Gleb Mazovetskiy
glebm commented May 03, 2012

+1

Felipe Rodrigues de Almeida

I believe respond_with has the same problem.

Andrew White
Owner

@felipero yes it will - as will anything where you can pass a model instance and get a url.

I haven't forgotten about this issue, it's just trickier than it seems to fix. The problem is there is no easy way to discern whether a model maps to a singular or a regular resource url. Checking for the presence of a collection url doesn't work as the resource may have been specified with :except => :index and trying to rescue route generation errors doesn't work because passing an instance to a singular resource url helper will generate a url with the format set to the id and no exception.

The only apparent way I can think of doing it is by creating a data structure that keeps a track of whether a set of routes is a singular resource and then querying that data structure in polymorphic_url. However before I or someone else can go ahead with that there needs to be a discussion whether we want the overhead of that structure.

Felipe Rodrigues de Almeida

Well, it is a non-blocking issue. Thanks for you prompt answer. :)

Viktor Trón
zelig commented June 15, 2012

I think someone ought to patch this. My thinking would not be along the lines of trying routes to fail (and fall back on the plural as in the comments in the rails issue) but go after association reflections.

Can we make the assumption that behind every nested resource form there is an activerecord association? No but we can test it

for form_for a, b if there is a model class A with has_many assoc to B, we can safely assume, expect that routes are set up to support a plural ("a_bs_path") for create. If there is a has_one we can safely assume, expect that routes are set up to support a singular ("a_bs_path"). If we find none, we fall back to the plural as the status quo educated guess. My 2 cents. Is this against modularity and decoupling MVC?

Andrew White
Owner

@zelig firstly, I think anything like that is tying your models to your routes too much. Secondly, what about non-AR models, and finally it doesn't fix the problem for top-level singular resources, e.g;

class Basket < ActiveRecord::Base
end

resource :basket

There's nothing in the model that's going to help with that - the only way it to store the fact that it's a singular resource and query that when generating the url. Adding metadata to the model won't help as a model may sometimes be a singular resource (/account) or a multiple resource (/admin/users/1).

Viktor Trón
zelig commented June 16, 2012

Surely this doesn't solve all problems but in the special case of nested AR resources, plurality can be reasonably guessed using model class associations, for others (i.e., top-level resource or non-AR models or models with implicit associations) the plural is a decent guess. This sounds to me a simple improvement but maybe the caveat is that it is not the right solution.

So where would you store resource singularity so that polyorphic_url could access this info? In case people are interested in fixing this ugly duckling.

jakevose

Add me to the list of people who just lost a bunch of time trying to figure out the "right" way to work around this and thinking myself crazy before stumbling upon this obscure reference to a long-standing, known bug.

Paweł Gościcki
pjg commented October 17, 2012

Add me to the list of people who just lost a bunch of time trying to figure out the "right" way to work around this and thinking myself crazy before stumbling upon this obscure reference to a long-standing, known bug.

And me as well.

Paweł Gościcki
pjg commented October 18, 2012

To anyone stumbling upon this issue, the workaround is quite simple. Given:

resource :billing_info

You generate the form_for call like this:

= form_for @billing_info, url: billing_info_path do |f|
Pirogov Evgenij

@pjg one of the nice features of form_for is to generate a proper url, based on object's persistance state, i.e. autodetect a path with either _create or _update affixes. That said, i'm unsure if the solution you propose will handle both cases at the same time. Will it?

Paweł Gościcki
pjg commented October 19, 2012

Yeah, it does handle both :create and :update actions.

Lorenzo Manacorda

It would be really nice to have this fixed, or at least to have the problem prominently divulged somehow. Just spent 2hrs thinking it was an error on my end!

Jose Bencosme

Wasted 2+ hours hunting this issue down, :(

Thanks @pjg for your workaround! :+1:

Gaurish Sharma

Today, I faced issue too. solved via workaround given Thanks @pjg :+1:

Hamed Asghari

I agree with @mikeni that this bug should be documented in the Rails guides.

Andrew Thal athal7 referenced this issue from a commit March 31, 2013
Commit has since been removed from the repository and is no longer available.
Andrew Thal athal7 referenced this issue from a commit in athal7/rails March 31, 2013
Add tests to demonstrate form_for issues with a singular resource. Is…
…sue #1769

These failing tests were written in an attempt to demonstrate the issues
discussed in #1789 related to attempting to use form_for with a singular
resource. I am aware from the discussion that the fix will most likely
not be an easy one, but I figured it could be helpful to have tests to
demonstrate the issue, and I hope that these tests serve that purpose.
6534486
Nazar Aziz
nazar commented April 02, 2013

Thanks @pijg for saving me an hour or three

Gustavo Kloh

+1 for this issue!

And one more thanks @pjg

Peter V
nl0pvm commented May 09, 2013

Wasted 1,5 hours hunting this issue down :-1:

Lee Brooks

All in All using singular resources as a newbie is a huge PITA

Cody Robbins

The above documentation fix should at least hopefully prevent anyone else from running into this bug until it’s fixed.

Tute Costa tute referenced this issue from a commit June 18, 2013
Commit has since been removed from the repository and is no longer available.
jonleung

I love how this is still a bug lol

Tute Costa
tute commented July 25, 2013

@rafaelfranca, can I help you squash this?

Rafael Mendonça França
Owner

Of course. Pull requests are always welcome. Make sure to ping @pixeltrix if you got a fix

Tute Costa
tute commented July 25, 2013

@rafaelfranca got the failing test from @athal7 running on current master, but seems I'll need some help. The issue is ActionDispatch::Routing::PolymorphicRoutes#polymorphic_path not knowing if the route was defined as singular or not. See #1769 (comment). How to approach it?

Andrew White
Owner

@tute you refactor the routing code, adding a :model option to resource and resources so that we can map from the model class to the route and the route needs to keep track of whether it's a singular or collection resource. That way you can directly lookup the correct route and not infer the route name from the model class.

That's how I plan to do if for Rails 4.1 :persevere:

Tute Costa
tute commented July 25, 2013

I'd love a way for PolymorphicRoutes to check the routes of the app. That way, and I know it's obvious, from the record it could check if it relates to a singular ot collection resource.

Too many moving parts for my new-to-rails-code brain. Would love to peek onto your machine while you work if that's fine with you, so I learn and can help in upcoming ones!

Stephanie Datu

Can't wait til this is fixed! Thanks, yet again, @pjg

Mike Pence

+1

Ross Wilson

I too have just encountered this issue, it'd be great if a fix can be found

How about having a singular option? What do you think?

form_for @resource, singular: true

I guess, this would then have to be passed down to polymorphic_url.

Andrew White
Owner

@RKushnir there's already a :url option so that doesn't really buy us anything since the create url and update url are the same, e.g:

form_for @resource, url: resource_url

I promise to fix it for 4.1 :smile:

@pixeltrix I can't totally agree. True that it doesn't buy anything in terms that we still have to pass an option, yet there's some profit in it. It's easier to type and abstract the singular option(or even make a helper singular_form_for) to use in all cases, instead of providing specific url option for each case individually.

Do you already have any ideas on how you would fix it?
An easier way would be to allow users to override some method in the model(i.e. model_name.route_key) to specify a singular form, but it's a view+routes issue and involving the model would be highly undesirable.
Another option would be to make polymorphic_url dependent on the router, so it has enough information about the resource, but seems like it would add much logic into polymorphic_url.

Cody Robbins

@RKushnir: I’m with @pixeltrix that adding a singular option doesn’t really gain you anything when you can already use the url option to work around the problem. Adding an additional special-purpose option when there’s already a workaround is just cruft as opposed to fixing the underlying behavior.

@codyrobbins Yeah, agree it's a cruft. But in a situation when workaround has become the standard because the bug stays unresolved for years, the hope is almost gone :frowning: I doubt if there's a clean way for

fixing the underlying behavior

Leaving the form_for options aside, if we had a concrete idea on how it should be done, it would increase the chance that someone makes a so long-awaited pull request. So, ideas?

Andrew White
Owner

@RKushnir you fix it in the router by linking the model class directly to the route rather than guessing the url helper name based on the model class, e.g.

# default :model option of Post
resources :posts

# default :model option of Basket - router knows that it's a singular resource route
resource :basket

# custom :model option - form_for @user uses singular resource route account_url
resource :account, model: 'User'

This needs significant refactoring of Journey, Mapper, RouteSet, etc. classes - I'm not expecting a PR to appear like magic so I'll be doing it myself. I had hoped to get it done for 4.0 but illness restricted the amount of time I had available.

There are some kinks to work out like what happens when a resource is used in a singular and normal context (e.g. /account for current user and /users/1 for user pages). Also there's the issue of how to handle namespaces routes like admin - currently you use an array like [:admin, @user] so I'd like to be able to continue using that.

what happens when a resource is used in a singular and normal context

That's one of the reasons why you shouldn't let the model mess with this.

Tute Costa tute referenced this issue from a commit in tute/rails July 25, 2013
Tute Costa Failing test for rails#1769 464c54e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.