replace regexp global in #url_for

[senny]

WIP:

I replaced $& with "\&" in the sub expression.

We don't have any test-coverage for this case though. (everything passes without the reference to the match). I think we should add coverage to see what it actually does 😄

[al2o3cr]

$& contains the piece of the string that was matched by the regexp; so in this case, it's saying "if the string has a ? character, use that - otherwise nothing". The intent, I'd guess, is correctly handle both of these cases:

http://example.com/foo/bar => http://example.com/foo/bar/
http://example.com/foo/bar?baz=wat => http://example.com/foo/bar/?baz=wat

The literal & is definitely wrong here.

[senny]

@al2o3cr it should be the same thing:

1.9.3p194 :003 > "welcome".sub(/e/, '\&')
 => "welcome" 
1.9.3p194 :004 > "welcome".sub(/e/) { $& }
 => "welcome" 

[fxn]

I don't think this change is necessary.

In spite of the sigil the $ family of variables for regexps are not really global, they are local to the current scope and per-thread. Moreover, in their scope $& and friends are available regardless of whether the block form was used.

[senny]

@tenderlove asked me to replace the global. In the end I'm not really concerned about the change that much but what I would really like to add is test-coverage for the case. Currently you can simply remove the global and everything passes. I'll write up a test-case once I got a couple of minutes.

[fxn]

👍 to the test.

@tenderlove is there any particular reason you want to change this?

[senny]

I added a test-case but I don't have a good feeling. I ran every test there is, and collected the :path argument. There was not a single path containing a ?. Is this something we even support?

[senny]

/cc @rafaelfranca @steveklabnik @carlosantoniodasilva

[timraymond]

The only problem I've found is if a trailing slash is meant to be part of the query string, such as "?spareslashes=////" where I expect the value of spareslashes to be "////". I can't really think of a reason someone would do this (maybe putting another path to redirect to later?), but it seems RFC 3986 allows it (specifically section 3.4... "The characters slash ("/") and question mark ("?") may represent data within the query component."). If this is the case it would seem that the slash could conceivably come at the end of a query string somehow. What does everyone think?

I've added this as a failing test case here: timraymond/rails@a18cc78

[senny]

If it's part of the RFC I think we should support it. As you mentioned it could actually happen when you pass an URL in the query string and you have trailing_slash: true.

I added your test-case and updated the implementation to get it passing.

@fxn could you review it?