Remove variables when determining the controller for match shorthand routes #7944

Closed
wants to merge 1 commit into
from

4 participants

@senny
Ruby on Rails member

When match is used inside a scope that contains path variables, the logic to determine the controller will return unusable results.

This patch removes the path variables from the segment, before determining the controller.

@senny
Ruby on Rails member

this is a fix for #7554

@rafaelfranca I was mistaken and the problem from #7554 was persistent on master. I included the test-case and a fix along with it.

@rafaelfranca
Ruby on Rails member

@pixeltrix could you take a look on this one.

If this fix doesn't break anything else I agree with it.

@pixeltrix pixeltrix was assigned Oct 14, 2012
@pixeltrix
Ruby on Rails member

@rafaelfranca @senny as I pointed out it's trivially easy to break this by changing the path <-> namespace mapping somehow, either by inserting extra modules or changing the path. The problem is the shorthand is normalized in Mapping using the full path whereas it should be using the current module from the scope and only deriving the additional controller/action details from the last segment passed to match, etc.

We either need to fix it so match shorthand syntax works everywhere or deprecate it.

@robin850 robin850 and 1 other commented on an outdated diff Oct 15, 2012
actionpack/test/dispatch/routing_test.rb
@@ -347,6 +347,10 @@ def self.call(params, request)
root :to => 'projects#index'
end
+ scope ':locale' do
+ match 'questions/new', :via => [:get]
@robin850
Ruby on Rails member
robin850 added a line comment Oct 15, 2012

Since we work with 1.9.3+ on master, can we use 1.9 Hash syntax please ? ^^ (sorry to «quibble» with that)

@steveklabnik
Ruby on Rails member
steveklabnik added a line comment Oct 15, 2012

Nice catch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@senny
Ruby on Rails member

@robin850 @steveklabnik updated the hash syntax to 1.9

@robin850
Ruby on Rails member
@frodsan frodsan commented on the diff Oct 25, 2012
actionpack/CHANGELOG.md
@@ -1,5 +1,10 @@
## Rails 4.0.0 (unreleased) ##
+* Remove variables when determining the controller for match shorthand routes
@frodsan
frodsan added a line comment Oct 25, 2012

fix punctuation please :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@frodsan frodsan commented on an outdated diff Oct 25, 2012
actionpack/CHANGELOG.md
@@ -1,5 +1,10 @@
## Rails 4.0.0 (unreleased) ##
+* Remove variables when determining the controller for match shorthand routes
+ Fix #7554
+
+ *Yves Senn*
+
* `assert_template` can be used to assert on the same template with different locals
@frodsan
frodsan added a line comment Oct 25, 2012

ditto.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@senny
Ruby on Rails member

@frodsan better?

Is this still up for a merge?

@rafaelfranca
Ruby on Rails member

I agree with @pixeltrix that we need to fix it so match shorthand syntax works everywhere or deprecate it.

I'll start a discussion about this.

@rafaelfranca rafaelfranca was assigned Oct 26, 2012
@pixeltrix pixeltrix was assigned Jan 22, 2013
@pixeltrix
Ruby on Rails member

Closing this as #9361 has been merged

@pixeltrix pixeltrix closed this Feb 21, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment