Fix Hash#to_query edge case with html_safe string on 1.8 ruby

[brainopia]

As follows from #1555 ActiveSupport::SafeBuffer#sub/gsub won't set method-local globals for block on 1.8 ruby.

Because of that Hash#to_query breaks when safe string is used as key (underneath CGI.escape invokes gsub with block on all items of hash):

  >> { '^^'.html_safe => ':(' }.to_query
  NoMethodError: You have a nil object when you didn't expect it!
  You might have expected an instance of Array.
  The error occurred while evaluating nil.size

The fix is simple use to_param on key instead of to_s since it is an alias of to_s for simple objects and alias of to_str for ActiveSupport::SafeBuffer.

I've provided test to show behavior.

[spastorino]

This is not a proper fix, we should stop adding hacks to SafeBuffer. People should just call to_str over SafeBuffer stuff until we provide a better solution for it.
Can you show me a real world example that might make me change my mind?

[brainopia]

Well, I'm not sure if it can be called a hack since I don't add anything specific to SafeBuffer. If anything commit adds consistency by calling to_param on both keys and values of hash (not only on values as previously).

By definition, to_param method is a common interface to represent objects in url and therefore it seems logical to use it inside to_query method not only for values, but for keys too (if anything, I think to_s calls in to_query method are not needed, but it's not my place to decide)

So this simple sticking to existing convention will allow us to support ActiveRecord and SafeBuffer, etc, objects as keys when we generate query from hash without any hacks.

So this change is not for sake of SafeBuffer but to use existing established interface for purposes it was created. And the positive effect this change has on SafeBuffer is just a consequence of a good interface.

Now back to the real world, I'm creating an interface where values supplied by user will be used as part of parameter names. You can see a similar interface at http://hurl.it by clicking on "add header" link and submitting a form. I hope it's persuasive enough :)

Whatever decision you make, I'll accept it with good faith since your guys really rock and I'm very thankful for your constant work on rails. I'm just trying to help out as best as I can :)

[spastorino]

@brainopia actually, you're right

[arunagw]

It's failing one test

Not sure if the test need a fix here

Failing

assert_equal 'custom2=param2-1&custom=param-1', {ToParam.new('custom') => ToParam.new('param'), ToParam.new('custom2') => ToParam.new('param2')}.to_param

Passed

assert_equal 'custom-1=param-1&custom2-1=param2-1', {ToParam.new('custom') => ToParam.new('param'), ToParam.new('custom2') => ToParam.new('param2')}.to_param

Do we need to change the test here?

[brainopia]

Sorry, missed that test. But yes, I think it should be updated to reflect that key is called with to_param like you've shown.

[spastorino]

Fix?

[brainopia]

#3065