bug in recently fixed Array.to_query?

Hello,

I posted a message about this previously on the prototype core board
(http://groups.google.com/group/prototype-core/browse_thread/thread/107f1dafddb773d7),
but I'm now thinking the issues lies in Rails.

This issue came about with recent fixes to Array.to_query. One of the
related tickets is http://dev.rubyonrails.org/changeset/6343.

I think the issue lies with a call to CGI::escape somewhere within
rails. Basically the issue is the following...

<%= url_for(:controller => 'test', :action =>
'auto_complete_for_test', :field => %w(a b c)) %>
generates...
'/test/auto_complete_for_test?field%5B%5D=a&amp;field%5B%5D=b&amp;field%5B%5D=c'

The &amp;'s get substituted to & in the html normally producing the
correct url for rails to parse. This is fine for urls generated
within "normal" html, but when used within a <script> tag, the &amp;
doesn't get substituted back into a single &. So for urls generated
within a script tag like those with auto_complete_field for example,
the url is incorrect.

This is how rails receives the data when the url resides in a script
tag (note the "field" and "amp;field" params)...

Processing TestController#auto_complete_for_test (for 127.0.0.1 at
2007-10-14 18:21:22) [POST]
  Parameters: {"action"=>"auto_complete_for_test", "field"=>["a"],
"controller"=>"test", "amp;field"=>["b", "c"], "test"=>"test"}

I would have expected an issue like this to have popped up before (and
maybe it has and been fixed), but it's occuring in the latest 1.2.5
release at least now.

So to summarize...

no issues with this alone...
url_for(:controller => 'test', :action => 'auto_complete_for_test',
:field => %w(a b c))

but this causes a problem...
<%= javascript_tag url_for(:controller => 'test', :action =>
'auto_complete_for_test', :field => %w(a b c)) %>

Is this an issue that has come up before?

Thanks,
Andrew

In response to my own question, further testing has shown that this
problem appears at least as far back as 1.2.2.(likely further) with
any parameters used in a script tag.

As this example shows, it's not specific to arrays, it's just applies
in general to a url with more than one parameter.

A url like this in a script tag...
url_for(:action => 'auto_complete_for_test', :test1 => 'a', :test2 => 'b')

Is received by rails like so...
Parameters: {"amp;test2"=>"b", "action"=>"auto_complete_for_test", "test1"=>"a"}

I searched for a ticket on this and couldn't find anything. Am I
simply doing something dumb here, which is why I'm not seeing much on
this in Trac?

Thanks,
Andrew

I searched for a ticket on this and couldn't find anything. Am I
simply doing something dumb here, which is why I'm not seeing much on
this in Trac?

I'm pretty sure this has come up before and I'm not sure we ever got a
'real' resolution.

It certainly seems messed up that we're generating URLs and escaping &
in url_for rather than in link_to, form_for etc.

I was just responding to my own post....

I see that simply passing :escape => false to any urls used within
script tags will solve the problem.

The only thing I see now that comes out of this I suppose is that by
default, methods like auto_complete_field should perhaps use :escape
=> false as well as any other methods that are wrapped in a
javascript_tag call and deal with urls. Perhaps the documentation for
url_for needs to stress :escape => false usage for urls generated in
script tags.

I'll try to create a patch shortly for the docs and at least the
auto_complete_field.

Sound acceptable?

I see that simply passing :escape => false to any urls used within
script tags will solve the problem.

But, why do we even have that option? Couldn't we just not escape,
then do that in link_to, url_for etc.?

Sound acceptable?

Only if we really need escaping in url_for. This will mess up things
like urls in mailers, etc. etc.

Aren't the urls escaped to produce valid HTML? A & that isn't escaped
in a url is not valid. But this escaping causes problems in script
tags, and mailers like you mention.

Basically it looks like a case of most urls occuring within normal
HTML, so escape it, otherwise manual override is necessary for the
other cases.

Couldn't url_for be overridden with in various modules to prevent the
escaping? Add a url_for to ActionMailer::Base,
ActionView::Helpers::JavaScriptMacrosHelper and any other places where
escaping should not be done... simply set :escape => false before
passing the url off to the main url_for method?

Just some thoughts.

Andrew

Aren't the urls escaped to produce valid HTML? A & that isn't escaped
in a url is not valid.

I agree, an '&' should normally be escaped as &amp; when rendering the url in html.

>Aren't the urls escaped to produce valid HTML? A & that isn't escaped
>in a url is not valid.

I agree, an '&' should normally be escaped as &amp; when rendering the url in html.

Sure, but shouldn't *link_to* do this escaping. It's the thing that
knows the URL is destined for html.

The top level remote methods (such as remote_form_for, link_to_remote)
would also require the escaping. I'm sure there are other methods
which would require it as well. This couldn't be specified in
remote_function though as this method can be used in a script tag
where escaping should be false, so setting :escape would have to be
set higher in the call stack where remote_function is invoked.

Methods like auto_complete_field and in_place_editor would set
escaping to false since it's context for the url is always within a
script tag.

So to do what you propose seems doable, but it looks like it would
require checking and modification to any method that wraps url_for.

The changes would still require the :escape option to exist though for
the cases (like remote_function) where escaping depends on it's
context.