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

Merged
merged 1 commit into from Sep 18, 2011

Projects

None yet

3 participants

@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
Ruby on Rails member

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
Ruby on Rails member

@brainopia actually, you're right

@spastorino spastorino merged commit cb0dbe3 into rails:master Sep 18, 2011
@arunagw
Ruby on Rails member

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
Ruby on Rails member

Fix?

@shwoodard shwoodard added a commit to shwoodard/rails-1 that referenced this pull request Sep 19, 2011
@shwoodard shwoodard Merge branch '3-1-stable' of github.com:rails/rails into 3-1-stable
* '3-1-stable' of github.com:rails/rails: (91 commits)
  Merge pull request #3049 from brainopia/fix_to_query_edge_case
  Bump AR-JDBC version. THis version is compatible with 3.1 and above
  Revert "Fixed Apache configuration for gzipped assets: FilesMatch and LocationMatch cannot be nested."
  Fixed Apache configuration for gzipped assets: FilesMatch and LocationMatch cannot be nested. (cherry picked from commit 5e2bf4d)
  Proper lines numbers for stack trace info
  Merge pull request #3040 from guilleiguaran/asset-host-should-not-be-nil-precompile
  cherry-pick #1318 into 3-1-stable
  Bumping up to 3.1.1.rc1
  Add some CHANGELOG missing stuff
  Allow asset tag helper methods to accept :digest => false option in order to completely avoid the digest generation.
  Add equivalent nginx configuration
  Fix typos and broken link on asset pipeline guide
  Merge pull request #3015 from guilleiguaran/clear-tmp-cache-on-precompile
  Merge pull request #3012 from guilleiguaran/3-1-1-changelogs
  Revert "Provide a way to access to assets without using the digest, useful for static files and emails"
  Merge pull request #3011 from guilleiguaran/disable-sprockets-server
  Provide a way to access to assets without using the digest, useful for static files and emails
  Merge pull request #2922 from wayneeseguin/master
  getting_started.textile: Fix typo and split up a sentence in section "Building a Multi-Model Form"
  getting_started.textile: Fix typos in section "Rendering a Partial Form"
  ...
4b2c9a9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment