Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

format are broken is with route globbing in 3.2.x #4817

Closed
sgruhier opened this Issue · 8 comments

3 participants

@sgruhier

I'm gonna try to be as clear as possible

I have a route:

  match "/(*filters)" => 'properties#index'

And in my controller I do

    respond_to :html, :js
    ...
    respond_to do |format|
      format.html 
      format.js { ...}
    end

As it's a map based application, routes could be like /ne_27.065938,-80.6092/sw_25.489856,-82.542794
it works in rails 3.1.3 but since rails 3.2.x decimals .542794 becomes the format so format.html is not called and a I got a

Completed 406 Not Acceptable

response

@tenderlove
Owner

OK! I will take a look

@tenderlove tenderlove was assigned
@tenderlove
Owner

Hi @sgruhier, sorry it took so long to get back to you. According to 51551a0, your route should not have worked in 3.1 without adding the :format => false parameter. Now the behavior seems to be matching the documentation.

I've added some better tests to document the behavior here: 5cc47e4

You should be able to fix your route by adding :format => false to the match declaration like so:

match "/(*filters)" => 'properties#index', :format => false

Sorry for the inconvenience, but at least you have a path forward! Thanks.

@tenderlove tenderlove closed this
@sgruhier

Thanks a lot for your time!
I swear that in rails 3.1.3 it works without :format => true :)

Started GET "/properties/ne_34.134525,-75.709298/sw_21.749277,-91.178048" for 127.0.0.1 at 2012-02-15 10:21:23 +0100
  Processing by PropertiesController#index as HTML
  Parameters: {"filters"=>"ne_34.134525,-75.709298/sw_21.749277,-91.178048"}

I'm gonna upgrade to 3.2.1 with :formatoption and share my results

@sgruhier

Perfect! it works
Thanks again

@tenderlove
Owner

@sgruhier yes, I verified that it works without the :format => true in 3.1.3. You are correct about that. The problem is that it wasn't supposed to work. The intention was that you had to add the new parameter. Unfortunately, the commit that was supposed to enforce the new format parameter didn't actually work, and when we switched from rack-mount to journey in 3.2, that happened to make it work the way it was intended.

Sorry about the confusion! :(

@pixeltrix
Owner

@tenderlove sorry for only spotting this now - the cause of the problem is that the glob parameter is optional in the route above and the regexp in 51551a0 is slightly buggy in that it picks up the closing parenthesis in the name of the requirement. This means that it doesn't match the param name in Rack::Mount and the default greedy regexp for glob params is applied instead which is the behaviour pre-51551a0.

I'll fix it in 3-1-stable and add some tests tonight so you can pull it into 3-1-4 if you want.

@pixeltrix pixeltrix reopened this
@pixeltrix
Owner

One more thing the test test_star_paths_are_greedy_but_not_too_much isn't really testing the intent of 5155a0 as the format is a required param - 5155a0 was added to address the fact that the format was being eaten up by the glob param. I will change the tests on 3-2-stable and master to remove the fixed :format and also add two extra tests to check non-optional glob params.

@pixeltrix
Owner

@tenderlove you fixed this on 3.2 in ba43b9b

@pixeltrix pixeltrix closed this in 5c18b99
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.