This project is archived and is in readonly mode.
:requirements not passed to named_route generated by :member
-
Marc Bowes
Oh dddear. The blockquotes didn't work so well for the example code, it seems to have been squished onto one line. Sorry :)
-
DHH
- Assigned user set to josh
- Milestone cleared.
-
CancelProfileIsBroken
The problem is in resources.rb, in action_options_for. This method comes up with the options for a route, including which requirements to use, by looking at the action name. The baz route falls into the else:
default_options.merge(add_conditions_for(resource.conditions, method)).merge(resource.requirements)
That's correct for an added collection route (no ID required in the route), but for an added member route it ought to be:
default_options.merge(add_conditions_for(resource.conditions, method)).merge(resource.requirements(require_id))
One possible fix would be to change the else to:
resource.member_methods.include?(method) ? default_options.merge(add_conditions_for(resource.conditions, method)).merge(resource.requirements(require_id)) : default_options.merge(add_conditions_for(resource.conditions, method)).merge(resource.requirements)
-
josh
- State changed from new to wontfix
- Milestone set to 2.x
@Mike that seems to cause a bunch of other problems with routes with name prefixes.
Please attach some failing unit tests otherwise punting till 2.x
-
CancelProfileIsBroken
Here's a failing test that passes with the fix I made (though as you point out that fix has other issues). I'll spend a bit more time looking at a possible patch.
-
CancelProfileIsBroken
- State changed from wontfix to open
And...here's a patch that passes the test, and doesn't fail any other tests. (this patch includes the tests as well)
-
Repository
- State changed from open to resolved
(from [5b7527ca44521edf9782b3d7f449bf09a29267f2]) Failing test for routes with member & requirement [#2054 :requirements not passed to named_route generated by :member state:resolved]
Signed-off-by: Joshua Peek josh@joshpeek.com http://github.com/rails/rails/co...
-
Luke Melia
This change breaks collection routes when a resource has a collection action and member action named the same. Attached is a failing test demonstrating as much. In the context of action_options_for, there is not enough information to determine whether to add the ID resource requirement or not.
Given we're close to 2.3, I'd recommend rolling back this commit to the previous broken behavior, rather than moving the problem, but perhaps someone can come up with a solution quickly.
-
josh
- Milestone cleared.
- State changed from resolved to open
-
Repository
- State changed from open to wontfix
(from [5b025a1d119eaf09f5376209da212b76725747f8]) Revert 5b7527ca "Failing test for routes with member & requirement" [#2054 :requirements not passed to named_route generated by :member state:wontfix] http://github.com/rails/rails/co...
-
CancelProfileIsBroken
- State changed from wontfix to open
Attached patch handles both cases (the original and the breakage that Luke spotted) by passing additional information into action_options_for. I'd like a more elegant solution, but I believe this one is safe.
-
Luke Melia
I can confirm that Mike's new patch not only passes the test but also works properly in the real-world app that surfaced this issue.
-
Repository
- State changed from open to resolved
(from [07710fd3e0c5c84521b7929738ba33cea99bc108]) Fix requirements for additional member/collection routes [#2054 :requirements not passed to named_route generated by :member state:resolved]
Signed-off-by: Joshua Peek josh@joshpeek.com http://github.com/rails/rails/co...
-
David Krmpotic
Before the change this (minimal case):
class Spot def to_param
idend end
worked. After the change, I got:
app.spot_path(Spot.first) TypeError: can't convert Fixnum into String
from generated code (/Users/david/Projects/oc/vendor/rails/actionpack/lib/action_controller/routing/route.rb:160):6:in `generate_raw' from /Users/david/Projects/oc/vendor/rails/actionpack/lib/action_controller/routing/route.rb:131:in `generate' from /Users/david/Projects/oc/vendor/rails/actionpack/lib/action_controller/routing/route_set.rb:384:in `generate' from /Users/david/Projects/oc/vendor/rails/actionpack/lib/action_controller/url_rewriter.rb:205:in `rewrite_path' from /Users/david/Projects/oc/vendor/rails/actionpack/lib/action_controller/url_rewriter.rb:184:in `rewrite_url' from /Users/david/Projects/oc/vendor/rails/actionpack/lib/action_controller/url_rewriter.rb:162:in `rewrite' from /Users/david/Projects/oc/vendor/rails/actionpack/lib/action_controller/integration.rb:243:in `url_for' from (eval):16:in `spot_path' from (irb):3I realize that I should have returned id.to_s in the first place, but I'm not sure if is/was expected to work with integers too... All in all now it doesn't anymore... Don't know enough about this subject to contribute a patch, but would love to learn more - especially if it's ok as it is or it does need to be patched.
Thank you david
