Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix url_for method's behavior. GH #3684. #4908

Merged
merged 1 commit into from
Feb 6, 2012
Merged

Conversation

kennyj
Copy link
Contributor

@kennyj kennyj commented Feb 6, 2012

Fix url_for method's behavior when it is passed with :controller option which starts with "/" in a multiple nested controller. Please see a testcase, and #3864

 1) Failure:
test_controller_option_which_starts_with_'/'_from_multiple_nested_controller(TestMultipleNestedController) [test/dispatch/routing_test.rb:2593]:
Expected: "/pooh"
  Actual: "/foo//pooh"

It seems that this problem is occured on 3-1-stable, 3-2-stable, and master.

Closes #3864

…on which starts with "/" from multiple nested controller.

Closes rails#3864
josevalim pushed a commit that referenced this pull request Feb 6, 2012
Fix url_for method's behavior. GH #3684.
@josevalim josevalim merged commit 6e054b1 into rails:master Feb 6, 2012
josevalim pushed a commit that referenced this pull request Feb 6, 2012
Fix url_for method's behavior. GH #3684.
@iHiD
Copy link
Contributor

iHiD commented Feb 6, 2012

Does this fix #2575?

@kennyj
Copy link
Contributor Author

kennyj commented Feb 6, 2012

It seems that this problem is same one ;-)

@kennyj
Copy link
Contributor Author

kennyj commented Feb 6, 2012

@iHiD Do you think about this PR ? Will we have any problem ? (or should close #2575 too ?)

@iHiD
Copy link
Contributor

iHiD commented Feb 6, 2012

I've got no issue with this PR at all. It's good. Thanks! :)
I'll check later to check it fixes my issue. I'll either mark the issue as closed or send a further PR that fixes that as well.

@houen
Copy link

houen commented Feb 10, 2012

Great update guys - wonderful to see this fixed :-) Is it too much to ask to have it posted here when it is inserted into core?

@houen
Copy link

houen commented Feb 10, 2012

Oh, by the way, with core, I mean stable release

@iHiD
Copy link
Contributor

iHiD commented Feb 10, 2012

I closed the other ticket and then realised I still had a monkeypatch there that was causing it to work regardless of your patch. I've just spent half an hour trying to get master branch working on my codebase and I'm giving up. I get so many dependancies failing in my Gemfile.

I'm pretty certain that what you've got there will fix it, so I'm just going to leave the other ticket closed and I'll keep an eye on it. Thanks, @kennyj.

@iHiD
Copy link
Contributor

iHiD commented Feb 10, 2012

@kennyj, sorry - could you put a PR for 3-2-stable too please so I can get rid of my monkey-patch? :)

@parndt
Copy link
Contributor

parndt commented Mar 2, 2012

Sorry to jump on the end of this ticket but.. any idea why this would cause resolve/refinerycms#1383 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Namespaced routes bug - in view for Backend::Stats::HitsController i can`t link to ArticlesController
5 participants