Skip to content
This repository

Fixed issue #1859: ArgumentError for to_xs after instruct! is called #2076

Closed
wants to merge 5 commits into from

10 participants

Brad Folkens Damien Mathieu Brian Lopez Josh Szmajda Gabor Garami Thibaut Barrère Michael Koziarski Jon Kessler Carlos Antonio da Silva John Lynch
Brad Folkens
x = Builder::XmlMarkup.new
x.instruct!

Gives the following stack trace with Ruby 1.8.7, Builder 3.0.0, Rails 3.1.0.rc[1-4]:
https://gist.github.com/1046481

Looks like an incompatibility between fast_xs and Builder 3.0.0, when it's included and overridden in ActiveSupport. The Builder 3.0.0 version takes an encoding argument.

Damien Mathieu
Collaborator

Look at the file you edited. The indentation is terribly messed up. You should use two spaced for indentation.

Shouldn't this be fixed in fast_xs ? The problem could occur similarly with any other library using it and not depending on activesupport.

Damien Mathieu
Collaborator
Brad Folkens

Yikes, sorry about that - I updated the commit to fix the whitespace issues.

From what I could tell this is a pretty specific issue to active_support where it replaces the Builder to_xs implementation with fast_xs. The fast_xs extension's method doesn't accept the encoding argument that the newer versions of Builder use. In reality, we're losing an argument by doing this so throwing an exception may be desired instead of this hack, or just not including fast_xs when the argument requirements aren't the same.

Brian Lopez

I say +1 this patch because it seems like ActiveSupport should have the responsibility of making sure the method signatures are compatible with both implementations if they want swap it out.

Josh Szmajda

Agreed with @brianmario. If ActiveSupport is changing how Builder behaves, it should ensure it presents the same API.

John Lynch

I cant believe that its been 8 months and this still has not been pulled into Rails. WAT?

Gabor Garami

If anyone would like work around this bug until rails developers finally takes care about it, then:

# config/initializers/fix_rails_2076
require 'builder'

module Builder
  class XmlBase
    # We aren't under Ruby 1.9?
    unless ::String.method_defined?(:encode)
      def _escape(text)
        text.to_xs
      end
    end
  end
end

Terrible, but works.

Thibaut Barrère
thbar commented March 16, 2012

@hron84 thanks!

Michael Koziarski
Owner
NZKoz commented April 21, 2012

Pulling this fix in involves monkeypatching a library which is still under active maintenance and perfectly capable of fixing it themselves. Accreting junk like this in our code base really isn't an appropriate fix.

Unless we get a definitive statement from @jimweirich that builder won't ever ship a fix for this arity problem, then adding the monkeypatch to our codebase is the wrong decision.

Michael Koziarski
Owner
NZKoz commented April 21, 2012

We could remove all the monkeypatching we have ourselves so we're just using 'raw' builder, but accreting more stuff that'll be legacy cruft soon isn't really a sustainable fix.

Gabor Garami

@NZKoz I understood this, however many developers wouldn't like waiting an undefined time for statement of builder's author, especially because this bug is not 'something issues a warning' but a fatal error.

To make clear: I would not recommend to include this patch into rails' codebase I know my solution is an ugly monkeypatching, I suggest use my fix until this bug is fixed and if reader cannot wait (or stuck on an older version of stuffs) until somebody, somewhere, somehow fix it.

Jon Kessler

@NZKoz Hi, @jimweirich has fixed this issue in builder 3.1.0. Alas, 3-1-stable, 3-2-stable and master are all locked down to builder ~> 3.0.0. I've asked Jim if he can release a patch version so rails users can use the change, but it seems that rails shouldn't be locked down to ~> 3.0.0 if we're depending on builder to fix the issue. The relevant pull request: jimweirich/builder#12

Carlos Antonio da Silva

This has been fixed with builder 3.0.1, run bundle update builder to get the new version with the proper fix. Please see @jonkessler link to builder for the discussion. Closing here, thanks.

Carlos Antonio da Silva carlosantoniodasilva closed this September 07, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
9  activesupport/lib/active_support/core_ext/string/xchar.rb
@@ -13,6 +13,13 @@
13 13
 
14 14
   class String
15 15
     alias_method :original_xs, :to_xs if method_defined?(:to_xs)
16  
-    alias_method :to_xs, :fast_xs
  16
+
  17
+    # to_xs expects 0 args from Builder < 3 but not >= 3, although fast_xs is 0 args
  18
+    if instance_method(:to_xs).arity == 0
  19
+      alias_method :to_xs, :fast_xs
  20
+    else
  21
+      def fast_xs_absorb_args(*args); fast_xs; end
  22
+      alias_method :to_xs, :fast_xs_absorb_args
  23
+    end
17 24
   end
18 25
 end
12  activesupport/test/core_ext/string_ext_test.rb
@@ -2,6 +2,7 @@
2 2
 require 'date'
3 3
 require 'abstract_unit'
4 4
 require 'inflector_test_cases'
  5
+require 'builder'
5 6
 
6 7
 require 'active_support/inflector'
7 8
 require 'active_support/core_ext/string'
@@ -476,3 +477,14 @@ class StringExcludeTest < ActiveSupport::TestCase
476 477
     assert_equal true, 'foo'.exclude?('p')
477 478
   end
478 479
 end
  480
+
  481
+class StringBuilderFastXSTest < Test::Unit::TestCase
  482
+  def test_build_xml_instruct_to_escaped_string
  483
+    xml = Builder::XmlMarkup.new
  484
+
  485
+    assert_nothing_raised do
  486
+    	xml.instruct!
  487
+      assert_equal '<?xml version="1.0" encoding="UTF-8"?>', xml
  488
+    end
  489
+  end
  490
+end
Commit_comment_tip

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.