ActiveSupport::Deprecation does not fallback to configured disallowed_behavior

[grncdr]

Steps to reproduce

# frozen_string_literal: true

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"

  git_source(:github) { |repo| "https://github.com/#{repo}.git" }

  # Activate the gem you are reporting the issue against.
  gem "activesupport", "~> 7.0.0"
end

require "active_support"
require "active_support/core_ext/object/blank"
require "minitest/autorun"

class BugTest < Minitest::Test
  def test_stuff
    warnings = []

    ActiveSupport::Deprecation.disallowed_warnings << /my_gem soon/
    ActiveSupport::Deprecation.disallowed_behavior = ->(msg, callstack, horizon, gem_name) {
      warnings << msg
    }

    deprecator = ActiveSupport::Deprecation.new('soon', 'my_gem')
    deprecator.deprecation_warning(:some_method)

    refute warnings.empty?
  end
end

Expected behavior

I expect the custom disallowed_behavior to be called by #deprecation_warning.

Actual behavior

The gem-default behaviour (:raise) is called instead.

System configuration

Rails version:

6.1.4.4 (but this problem affects all versions of Rails from 6.1 onwards)

Ruby version:

Ruby 3.1 (but this problem also occurs in earlier Ruby versions).

More information

I think this could be fixed by prepending a fallback to ActiveSupport::Deprecation.disallowed_behavior here:

rails/activesupport/lib/active_support/deprecation/behaviors.rb

Lines 71 to 73 in 87d4d0f

	def disallowed_behavior
	@disallowed_behavior ||= [DEFAULT_BEHAVIORS[:raise]]
	end

However, because this class is using a singleton and InstanceDelegator the method might need two definitions, one on the class-level (that falls back to DEFAULT_BEHAVIORS) and one on the instance level (that falls back to the class-level behavior).

[grncdr]

As a work-around, I am doing this in an initializer.

ActiveSupport::Deprecations[:raise] = ->(...) { ... }

[rafaelfranca]

ActiveSupport::Deprecation.disallowed_behavior you are only changing the behavior of the default deprecator. If you create a new instance you need to configure it instead of the default.

[rafaelfranca]

# frozen_string_literal: true

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"

  git_source(:github) { |repo| "https://github.com/#{repo}.git" }

  # Activate the gem you are reporting the issue against.
  gem "activesupport", "~> 7.0.0"
end

require "active_support"
require "active_support/core_ext/object/blank"
require "minitest/autorun"

class BugTest < Minitest::Test
  def test_stuff
    warnings = []

    deprecator = ActiveSupport::Deprecation.new('soon', 'my_gem')
    deprecator.disallowed_behavior = ->(msg, callstack, horizon, gem_name) {
      warnings << msg
    }
    deprecator.disallowed_warnings << /my_gem soon/
    deprecator.deprecation_warning(:some_method)

    refute warnings.empty?
  end
end

[grncdr]

@rafaelfranca I understand the mechanics of why it didn't work as I expected, but I feel like either this behavior is a bug, or the documentation could be improved.

If the behavior is not open to discussion, the documentation for #deprecate_methods could be improved. This is where I got the idea to pass a custom deprecator, and it doesn't mention that this deprecator will not pick up any config from config.active_support.disallowed_deprecation.

I'd still prefer to call this a bug, but if not: would a PR for that documentation change be more likely to be accepted?

[rafaelfranca]

This is not a code bug, but it is a documentation bug. A PR to update the documentation makes sense.

[grncdr]

Ok, I tried updating the documentation, and that clarified what is surprising-enough-to-be-a-bug-IMO about this behavior: the global configuration of ActiveSupport::Deprecation.disallowed_warnings does affect the custom deprecator, while ActiveSupport::Deprecation.disallowed_behavior doesn't.

So one can't simply write something like this:

Note that custom deprecators are not affected by the global config.active_support.deprecation_* settings.

but instead must clarify which global settings will affect the custom deprecator, and which won't. Consider this (accurate) documentation:

Note that custom deprecators are not affected by the global config.active_support.deprecation or config.active_support.disallowed_deprecation settings, but are affected by the global config.active_support.disallowed_deprecation_warnings setting.

This means that deprecations created using a custom deprecator (without it's own disallowed_behavior) will raise exceptions if they match the globally configured disallowed_deprecation_warnings setting.

It might not be a bug, but it sure feels wrong to require such complex explanations to avoid shooting yourself in the foot.

Or maybe it's better to say that a custom deprecator using the global disallowed_warnings matcher list is a bug, and fix that?

[grncdr]

@rafaelfranca any chance this could be re-opened in light of the above? (It's been a few days and I'm not sure if you've seen my last comment).

[ghiculescu]

I agree that's pretty surprising. Just as a guess, do you think the issue is the use of attr_writer vs setting disallowed_behaviour= directly?

Given the inconsistency here I'm going to reopen so we can investigate more.

[grncdr]

I think the first issue is deciding what the intended behavior here is, custom deprecators should either use or not use shared global config in a consistent way and right now they don't. @cpruitt seems to be the original author of this feature from #37940, maybe he can shed some light on how it should work?

---

If custom deprecators are intended to be isolated and not use global config, then line 27 here is the culprit:

rails/activesupport/lib/active_support/deprecation/disallowed.rb

Lines 26 to 38 in a2217ae

	def deprecation_disallowed?(message)
	disallowed = ActiveSupport::Deprecation.disallowed_warnings
	return false if explicitly_allowed?(message)
	return true if disallowed == :all
	disallowed.any? do |rule|
	case rule
	when String, Symbol
	message.include?(rule.to_s)
	when Regexp
	rule.match?(message)
	end
	end
	end

Just changing that to use self.disallowed_warnings would fix the inconsistency by ensuring that custom deprecators are isolated from the global config... but that use of the shared global disallowed_warnings looks very intentional to me.

---

If custom deprecators should use the global config, then I'd say this lazy reader needs to be modified:

rails/activesupport/lib/active_support/deprecation/behaviors.rb

Lines 71 to 73 in f95c0b7

	def disallowed_behavior
	@disallowed_behavior ||= [DEFAULT_BEHAVIORS[:raise]]
	end

If an ActiveSupport::Deprecation instance doesn't have it's own disallowed behaviour it should fall back to the global one. However, the "global" methods are implemented by delegating to a singleton instance, so you'd need an if self == ActiveSupport::Deprecation.instance check to avoid infinite recursion. This would be somewhat more complicated than isolated instances, but it would (IMO) provide a better experience for users of the framework.

---

Personally I prefer the latter, since it was the behaviour I expected in the first place 😉 but I'm happy to contribute either solution.

[ghiculescu]

The latter seems correct to me also (but I'm not on core so don't take my word on it). If it's not too much effort it may be easier to discuss over a PR.
@rafaelfranca what do you think?

[skipkayhil]

Going to close this as well per #44772 (comment)