Merge pull request rails#54293 from byroot/mb-chars-deprecation

Improve deprecation of String#mb_chars

Author: byroot

activerecord/lib/active_record/connection_adapters/abstract/quoting.rb

@@ -71,7 +71,7 @@ def quote_table_name(table_name)
       # {SQL injection attacks}[https://en.wikipedia.org/wiki/SQL_injection].
       def quote(value)
         case value
-        when String, Symbol
+        when String, Symbol, ActiveSupport::Multibyte::Chars
           "'#{quote_string(value.to_s)}'"
         when true       then quoted_true
         when false      then quoted_false
@@ -84,11 +84,7 @@ def quote(value)
         when Date, Time then "'#{quoted_date(value)}'"
         when Class      then "'#{value}'"
         else
-          if value.class.name == "ActiveSupport::Multibyte::Chars"
-            "'#{quote_string(value.to_s)}'"
-          else
-            raise TypeError, "can't quote #{value.class.name}"
-          end
+          raise TypeError, "can't quote #{value.class.name}"
         end
       end
 
@@ -97,7 +93,7 @@ def quote(value)
       # to a String.
       def type_cast(value)
         case value
-        when Symbol, Type::Binary::Data
+        when Symbol, Type::Binary::Data, ActiveSupport::Multibyte::Chars
           value.to_s
         when true       then unquoted_true
         when false      then unquoted_false
@@ -107,11 +103,7 @@ def type_cast(value)
         when Type::Time::Value then quoted_time(value)
         when Date, Time then quoted_date(value)
         else
-          if value.class.name == "ActiveSupport::Multibyte::Chars"
-            value.to_s
-          else
-            raise TypeError, "can't cast #{value.class.name}"
-          end
+          raise TypeError, "can't cast #{value.class.name}"
         end
       end
 

activerecord/test/cases/sanitize_test.rb

@@ -13,19 +13,29 @@ def setup
   def test_sanitize_sql_array_handles_string_interpolation
     quoted_bambi = ActiveRecord::Base.lease_connection.quote_string("Bambi")
     assert_equal "name='#{quoted_bambi}'", Binary.sanitize_sql_array(["name='%s'", "Bambi"])
-    assert_equal "name='#{quoted_bambi}'", Binary.sanitize_sql_array(["name='%s'", "Bambi".mb_chars])
+    assert_deprecated ActiveSupport.deprecator do
+      assert_equal "name='#{quoted_bambi}'", Binary.sanitize_sql_array(["name='%s'", "Bambi".mb_chars])
+    end
+
     quoted_bambi_and_thumper = ActiveRecord::Base.lease_connection.quote_string("Bambi\nand\nThumper")
     assert_equal "name='#{quoted_bambi_and_thumper}'", Binary.sanitize_sql_array(["name='%s'", "Bambi\nand\nThumper"])
-    assert_equal "name='#{quoted_bambi_and_thumper}'", Binary.sanitize_sql_array(["name='%s'", "Bambi\nand\nThumper".mb_chars])
+
+    assert_deprecated ActiveSupport.deprecator do
+      assert_equal "name='#{quoted_bambi_and_thumper}'", Binary.sanitize_sql_array(["name='%s'", "Bambi\nand\nThumper".mb_chars])
+    end
   end
 
   def test_sanitize_sql_array_handles_bind_variables
     quoted_bambi = ActiveRecord::Base.lease_connection.quote("Bambi")
     assert_equal "name=#{quoted_bambi}", Binary.sanitize_sql_array(["name=?", "Bambi"])
-    assert_equal "name=#{quoted_bambi}", Binary.sanitize_sql_array(["name=?", "Bambi".mb_chars])
+    assert_deprecated ActiveSupport.deprecator do
+      assert_equal "name=#{quoted_bambi}", Binary.sanitize_sql_array(["name=?", "Bambi".mb_chars])
+    end
     quoted_bambi_and_thumper = ActiveRecord::Base.lease_connection.quote("Bambi\nand\nThumper")
     assert_equal "name=#{quoted_bambi_and_thumper}", Binary.sanitize_sql_array(["name=?", "Bambi\nand\nThumper"])
-    assert_equal "name=#{quoted_bambi_and_thumper}", Binary.sanitize_sql_array(["name=?", "Bambi\nand\nThumper".mb_chars])
+    assert_deprecated ActiveSupport.deprecator do
+      assert_equal "name=#{quoted_bambi_and_thumper}", Binary.sanitize_sql_array(["name=?", "Bambi\nand\nThumper".mb_chars])
+    end
   end
 
   def test_sanitize_sql_array_handles_named_bind_variables
@@ -224,8 +234,13 @@ def test_bind_chars
     quoted_bambi_and_thumper = ActiveRecord::Base.lease_connection.quote("Bambi\nand\nThumper")
     assert_equal "name=#{quoted_bambi}", bind("name=?", "Bambi")
     assert_equal "name=#{quoted_bambi_and_thumper}", bind("name=?", "Bambi\nand\nThumper")
-    assert_equal "name=#{quoted_bambi}", bind("name=?", "Bambi".mb_chars)
-    assert_equal "name=#{quoted_bambi_and_thumper}", bind("name=?", "Bambi\nand\nThumper".mb_chars)
+    assert_deprecated ActiveSupport.deprecator do
+      assert_equal "name=#{quoted_bambi}", bind("name=?", "Bambi".mb_chars)
+    end
+
+    assert_deprecated ActiveSupport.deprecator do
+      assert_equal "name=#{quoted_bambi_and_thumper}", bind("name=?", "Bambi\nand\nThumper".mb_chars)
+    end
   end
 
   def test_named_bind_with_postgresql_type_casts

activesupport/lib/active_support/core_ext/string/multibyte.rb

@@ -35,7 +35,16 @@ class String
   # For more information about the methods defined on the Chars proxy see ActiveSupport::Multibyte::Chars. For
   # information about how to change the default Multibyte behavior see ActiveSupport::Multibyte.
   def mb_chars
-    ActiveSupport::Multibyte.proxy_class.new(self)
+    ActiveSupport.deprecator.warn(
+      "String#mb_chars is deprecated and will be removed in Rails 8.2. " \
+      "Use normal string methods instead."
+    )
+
+    if ActiveSupport::Multibyte.proxy_class == ActiveSupport::Multibyte::Chars
+      ActiveSupport::Multibyte::Chars.new(self, deprecation: false)
+    else
+      ActiveSupport::Multibyte.proxy_class.new(self)
+    end
   end
 
   # Returns +true+ if string has utf_8 encoding.

activesupport/lib/active_support/multibyte/chars.rb

@@ -5,11 +5,6 @@
 require "active_support/core_ext/string/behavior"
 require "active_support/core_ext/module/delegation"
 
-ActiveSupport.deprecator.warn(
-  "ActiveSupport::Multibyte::Chars and String#mb_chars are deprecated and will be removed in Rails 8.2. " \
-  "Use normal string methods instead."
-)
-
 module ActiveSupport # :nodoc:
   module Multibyte # :nodoc:
     # = Active Support \Multibyte \Chars
@@ -58,7 +53,14 @@ class Chars
       delegate :<=>, :=~, :match?, :acts_like_string?, to: :wrapped_string
 
       # Creates a new Chars instance by wrapping _string_.
-      def initialize(string)
+      def initialize(string, deprecation: true)
+        if deprecation
+          ActiveSupport.deprecator.warn(
+            "ActiveSupport::Multibyte::Chars is deprecated and will be removed in Rails 8.2. " \
+            "Use normal string methods instead."
+          )
+        end
+
         @wrapped_string = string
         if string.encoding != Encoding::UTF_8
           @wrapped_string = @wrapped_string.dup

activesupport/test/abstract_unit.rb

@@ -22,7 +22,7 @@
 Thread.abort_on_exception = true
 
 # Show backtraces for deprecated behavior for quicker cleanup.
-ActiveSupport.deprecator.debug = true
+ActiveSupport.deprecator.behavior = :raise
 
 # Default to Ruby 2.4+ to_time behavior but allow running tests with old behavior
 ActiveSupport.deprecator.silence do
@@ -34,10 +34,6 @@
 # Disable available locale checks to avoid warnings running the test suite.
 I18n.enforce_available_locales = false
 
-ActiveSupport.deprecator.silence do
-  ActiveSupport::Multibyte.const_get(:Chars)
-end
-
 class ActiveSupport::TestCase
   if Process.respond_to?(:fork) && !Gem.win_platform?
     parallelize

activesupport/test/core_ext/string_ext_test.rb

@@ -801,7 +801,9 @@ def test_string_should_recognize_utf8_strings
   end
 
   def test_mb_chars_returns_instance_of_proxy_class
-    assert_kind_of ActiveSupport::Multibyte.proxy_class, UTF8_STRING.mb_chars
+    assert_deprecated ActiveSupport.deprecator do
+      assert_kind_of ActiveSupport::Multibyte.proxy_class, UTF8_STRING.mb_chars
+    end
   end
 end
 

activesupport/test/multibyte_chars_test.rb

@@ -7,6 +7,14 @@
 class MultibyteCharsTest < ActiveSupport::TestCase
   include MultibyteTestHelpers
 
+  def run(...)
+    result = nil
+    assert_deprecated ActiveSupport.deprecator do
+      result = super
+    end
+    result
+  end
+
   def setup
     @proxy_class = ActiveSupport::Multibyte::Chars
     @chars = @proxy_class.new UNICODE_STRING.dup
@@ -96,6 +104,14 @@ def test_should_return_string_as_json
 class MultibyteCharsUTF8BehaviorTest < ActiveSupport::TestCase
   include MultibyteTestHelpers
 
+  def run(...)
+    result = nil
+    assert_deprecated ActiveSupport.deprecator do
+      result = super
+    end
+    result
+  end
+
   def setup
     @chars = UNICODE_STRING.dup.mb_chars
     # Ruby 1.9 only supports basic whitespace
@@ -491,6 +507,14 @@ def test_acts_like_string
 class MultibyteCharsExtrasTest < ActiveSupport::TestCase
   include MultibyteTestHelpers
 
+  def run(...)
+    result = nil
+    assert_deprecated ActiveSupport.deprecator do
+      result = super
+    end
+    result
+  end
+
   def test_upcase_should_be_unicode_aware
     assert_equal "АБВГД\0F", chars("аБвгд\0f").upcase
     assert_equal "こにちわ", chars("こにちわ").upcase
@@ -569,6 +593,8 @@ def test_normalization_shouldnt_strip_null_bytes
   end
 
   def test_should_compute_grapheme_length
+    "foo".mb_chars # appease assert_deprecated
+
     [
       ["", 0],
       ["abc", 3],

activesupport/test/multibyte_proxy_test.rb

@@ -25,10 +25,12 @@ def with_custom_encoder(encoder)
   end
 
   test "custom multibyte encoder" do
-    with_custom_encoder(AsciiOnlyEncoder) do
-      assert_equal "s?me string 123", "søme string 123".mb_chars.to_s
-    end
+    assert_deprecated ActiveSupport.deprecator do
+      with_custom_encoder(AsciiOnlyEncoder) do
+        assert_equal "s?me string 123", "søme string 123".mb_chars.to_s
+      end
 
-    assert_equal "søme string 123", "søme string 123".mb_chars.to_s
+      assert_equal "søme string 123", "søme string 123".mb_chars.to_s
+    end
   end
 end

activesupport/test/multibyte_test_helpers.rb

@@ -7,7 +7,9 @@ module MultibyteTestHelpers
   BYTE_STRING = "\270\236\010\210\245".b.freeze
 
   def chars(str)
-    ActiveSupport::Multibyte::Chars.new(str)
+    assert_deprecated ActiveSupport.deprecator do
+      ActiveSupport::Multibyte::Chars.new(str)
+    end
   end
 
   def inspect_codepoints(str)