Removing to_shorthand to fix #6497 #7668

Merged
merged 1 commit into from Sep 19, 2012

Conversation

Projects
None yet
4 participants
@Draiken
Contributor

Draiken commented Sep 17, 2012

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

This comment has been minimized.

Show comment Hide comment
@Draiken

Draiken Sep 17, 2012

Contributor

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

Contributor

Draiken commented Sep 17, 2012

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

@rafaelfranca

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Sep 17, 2012

Member

cc @drogus

Member

rafaelfranca commented Sep 17, 2012

cc @drogus

@rafaelfranca

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Sep 17, 2012

Member

linking #6497

Member

rafaelfranca commented Sep 17, 2012

linking #6497

@steveklabnik

This comment has been minimized.

Show comment Hide comment
@steveklabnik

steveklabnik Sep 17, 2012

Member

@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.

Member

steveklabnik commented Sep 17, 2012

@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

This comment has been minimized.

Show comment Hide comment
@Draiken

Draiken Sep 17, 2012

Contributor

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

Contributor

Draiken commented Sep 17, 2012

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

@steveklabnik

This comment has been minimized.

Show comment Hide comment
@steveklabnik

steveklabnik Sep 17, 2012

Member

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

Member

steveklabnik commented Sep 17, 2012

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

This comment has been minimized.

Show comment Hide comment
@Draiken

Draiken Sep 17, 2012

Contributor

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

Contributor

Draiken commented Sep 17, 2012

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

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Sep 17, 2012

Member

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

Member

rafaelfranca commented Sep 17, 2012

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

@Draiken

This comment has been minimized.

Show comment Hide comment
@Draiken

Draiken Sep 18, 2012

Contributor

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

Contributor

Draiken commented Sep 18, 2012

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

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Sep 18, 2012

Member

@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.

Member

rafaelfranca commented Sep 18, 2012

@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.

@ghost ghost assigned drogus Sep 18, 2012

@Draiken

This comment has been minimized.

Show comment Hide comment
@Draiken

Draiken Sep 18, 2012

Contributor

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

Contributor

Draiken commented Sep 18, 2012

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

This comment has been minimized.

Show comment Hide comment
@Draiken

Draiken Sep 18, 2012

Contributor

@rafaelfranca is this test good enough?

Contributor

Draiken commented Sep 18, 2012

@rafaelfranca is this test good enough?

@rafaelfranca

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Sep 18, 2012

Member

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.

Member

rafaelfranca commented Sep 18, 2012

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

This comment has been minimized.

Show comment Hide comment
@Draiken

Draiken Sep 18, 2012

Contributor

@rafaelfranca updated changelog and squashed commits.

Contributor

Draiken commented Sep 18, 2012

@rafaelfranca updated changelog and squashed commits.

@rafaelfranca

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Sep 18, 2012

Member

Thanks. Now we need a rebase.

Member

rafaelfranca commented Sep 18, 2012

Thanks. Now we need a rebase.

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.
@Draiken

This comment has been minimized.

Show comment Hide comment
@Draiken

Draiken Sep 19, 2012

Contributor

rebased

Contributor

Draiken commented Sep 19, 2012

rebased

rafaelfranca added a commit that referenced this pull request Sep 19, 2012

@rafaelfranca rafaelfranca merged commit f4ad0eb into rails:master Sep 19, 2012

@rafaelfranca

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Sep 19, 2012

Member

Thanks.

Member

rafaelfranca commented Sep 19, 2012

Thanks.

rafaelfranca added a commit that referenced this pull request Sep 19, 2012

Merge pull request #7668 from Draiken/fix_issue_6497
Removing to_shorthand to fix #6497
Conflicts:
	actionpack/CHANGELOG.md

rafaelfranca added a commit that referenced this pull request Nov 2, 2012

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.

rafaelfranca added a commit that referenced this pull request Nov 2, 2012

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@281367e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment