Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Removing to_shorthand to fix #6497 #7668

Merged
merged 1 commit into from

4 participants

@Draiken

When using shortcut routes inside an engine the "to_shorthand" variable
is set to true, causing the module scope of the route to not be applied.

@Draiken

@steveklabnik pull request created. All actionpack tests passed in my local machine.

@rafaelfranca

linking #6497

@steveklabnik
Collaborator

@Draiken thanks! For future reference, if you include 'Fixes #6498' in the body of your commit message (not the title), it will automatically close that issue when your stuff gets merged. If you're bored, feel free to change it up, and if not, we can close it manually when this gets merged.

@Draiken

@steveklabnik Oh, I thought "fix #number" would do it. Next time I'll use the "fixes". Thanks!

@steveklabnik
Collaborator

I think it's that it doesn't work in the title, it has to be in the body. Yeah: https://github.com/blog/831-issues-2-0-the-next-generation

@Draiken

Then I think it'll work, since the commit has the same name as the pull request.

About the change itself, I have no idea if this could break something, I can't imagine a scenario that adding the module scope would be wrong, but presumably the to_shorthand condition was there for a reason, I just have no clue what reason

@rafaelfranca

We have to add test to this change because in the future someone can add again this check.

@Draiken

I am trying to add a test for this but I am not sure how to reproduce this error in the test env. Tried this:

def test_shorthand_route_with_namespace
  fakeset = FakeSet.new
  mapper = Mapper.new fakeset
  mapper.scope :module => 'engine' do
    mapper.get 'controller/action'
  end
  assert_equal '/engine/controller/action(.:format)', fakeset.conditions.first[:path_info]
end

But it doesn't add the /engine path, since the :module option just adds the module to the controller part, not to the path. Looking around in the engine code, the only default_scope added by the engine's routes is the :module, so it also doesn't add the prefix to the route path.

My guess is this specific path is added somewhere later in the process when the engine is loaded and the isolated flag is true, then it fixes the path.

So, how should I proceed with testing? Is this the right direction? Should I keep it on mapper_test, or move it somewhere else?

Sorry for all these questions... This is my first rails contribution

@rafaelfranca

@Draiken thanks to trying to add the test. Unfortunately I don't have the answer because I'm not familiar with the engines code. But @drogus can help you.

@drogus I'm assigning this to you, please try to help @Draiken with the test reproduction.

@drogus drogus was assigned
@Draiken

Okay, I this has nothing to do with engines, the engine's path is added by the mount and it has nothing to do with the shorthand. I just need to figure out where to test for the recognized controller/action pair for the routes

@Draiken

@rafaelfranca is this test good enough?

@rafaelfranca

If it is failing without remove the to_shorthand and passing removing, so it is enough.

Also please add a CHANGELOG entry as described in the contributing guide.

@Draiken

@rafaelfranca updated changelog and squashed commits.

@rafaelfranca

Thanks. Now we need a rebase.

@Draiken Draiken Removing to_shorthand from default_controller_and_action. Fixes #6497
When using shortcut routes inside an engine the "to_shorthand" variable
is set to true, causing the module scope of the route to not be applied.
641ea69
@Draiken

rebased

@rafaelfranca rafaelfranca merged commit f4ad0eb into rails:master
@rafaelfranca

Thanks.

@rafaelfranca rafaelfranca referenced this pull request from a commit
@rafaelfranca rafaelfranca Revert "Merge pull request #7668 from Draiken/fix_issue_6497"
This reverts commit 61d5d2d.

Conflicts:
	actionpack/CHANGELOG.md

REASON: This added a backward incompatible change.
4a86362
@rafaelfranca rafaelfranca referenced this pull request from a commit
@rafaelfranca rafaelfranca Revert "Merge pull request #7668 from Draiken/fix_issue_6497"
This reverts commit f4ad0eb, reversing
changes made to 8b2cbb3.

Conflicts:
	actionpack/CHANGELOG.md

REASON: This added introduced a bug when you have a shorthand route
inside a nested namespace.

See
rafaelfranca/rails@281367e
1bfc5b4
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Sep 19, 2012
  1. @Draiken

    Removing to_shorthand from default_controller_and_action. Fixes #6497

    Draiken authored
    When using shortcut routes inside an engine the "to_shorthand" variable
    is set to true, causing the module scope of the route to not be applied.
This page is out of date. Refresh to see the latest.
View
14 actionpack/CHANGELOG.md
@@ -4,9 +4,19 @@
*Yves Senn*
-* Allow pass couple extensions to ActionView::Template.register_template_handler call. *Tima Maslyuchenko*
+* Allow pass couple extensions to `ActionView::Template.register_template_handler` call. *Tima Maslyuchenko*
-* Sprockets integration has been extracted from Action Pack and the `sprockets-rails`
+* Fixed a bug with shorthand routes scoped with the `:module` option not
+ adding the module to the controller as described in issue #6497.
+ This should now work properly:
+
+ scope :module => "engine" do
+ get "api/version" # routes to engine/api#version
+ end
+
+ *Luiz Felipe Garcia Pereira*
+
+* Sprockets integration has been extracted from Action Pack and the `sprockets-rails`
gem should be added to Gemfile (under the assets group) in order to use Rails asset
pipeline in future versions of Rails.
View
2  actionpack/lib/action_dispatch/routing/mapper.rb
@@ -182,7 +182,7 @@ def default_controller_and_action(to_shorthand=nil)
controller ||= default_controller
action ||= default_action
- unless controller.is_a?(Regexp) || to_shorthand
+ unless controller.is_a?(Regexp)
controller = [@scope[:module], controller].compact.join("/").presence
end
View
7 actionpack/test/dispatch/routing_test.rb
@@ -363,6 +363,7 @@ def self.call(params, request)
resources :errors, :shallow => true do
resources :notices
end
+ get 'api/version'
end
scope :path => 'api' do
@@ -1280,6 +1281,12 @@ def test_match_shorthand_inside_namespace
assert_equal 'account#shorthand', @response.body
end
+ def test_match_shorthand_with_module
+ assert_equal '/api/version', api_version_path
+ get '/api/version'
+ assert_equal 'api/api#version', @response.body
+ end
+
def test_dynamically_generated_helpers_on_collection_do_not_clobber_resources_url_helper
assert_equal '/replies', replies_path
end
Something went wrong with that request. Please try again.