Include gem name and deprecation horizon when calling the deprecation behaviors #28800

wvanbergen · 2017-04-19T18:55:59Z

Summary

We are planning to use ActiveSupport::Deprecation in our own code base to mark deprecated behavior. We have some tooling subscribing to the ActiveSupport::Notifications that are generated by the deprecations.

However, for our tooling we would like to be able to make sure we can differentiate between the source of the deprecation. ActiveSupport::Deprecation already has useful context for this: gem name, and deprecation horizon. Unfortunately, these pieces of metadata are not made available to the behavior lambdas.

This PR exposes them as extra arguments, and makes sure those arguments are passed on to the ActiveSupport::Notification we create. As part of this, we also dynamically generate the notification name based on the gem name. It was hardcoded to deprecations.rails before. All the other predefined behaviors are unaffected by this.

cc @rafaelfranca @casperisfine

kaspth

Since this changes the number of arguments that can be passed to a custom installed behavior, we'd need to inspect the arity and coerce those that don't support the more elaborate version. Perhaps something like this:

def behavior=(behavior)
  @behavior = Array(behavior).map { |b| DEFAULT_BEHAVIORS[b] || arity_coerce(b) }
end

private
  def arity_coerce(behavior)
    if behavior.arity >= 2
      behavior
    else
      -> message, callstack, _, _ { behavior.call(message, callstack) }
    end
  end

kaspth · 2017-04-19T19:02:22Z

activesupport/test/deprecation_test.rb

@@ -119,7 +119,7 @@ def test_raise_behaviour
    callstack = caller_locations

    e = assert_raise ActiveSupport::DeprecationException do
-      ActiveSupport::Deprecation.behavior.first.call(message, callstack)
+      ActiveSupport::Deprecation.behavior.first.call(message, callstack, "horizon", "gem")


Alludes to backwards incompatibility: custom installed behaviors can raise ArgumentError's now.

kaspth · 2017-04-19T19:06:54Z

activesupport/test/deprecation_test.rb

+    behavior = ActiveSupport::Deprecation.behavior.first
+
+    begin
+      events = []


Why define this as an array when we expect there to only be one? If it wasn't the right message, we'd eventually find out in the latter tests.

E.g. we could opt for _, event = ActiveSupport::Notifications.subscribe("deprecation.my_gem.custom", &:itself).

This doesn't work because the itself method doesn't have the correct arity.

I also kind of like the explicitness of the array approach. Less obscure on how it works, and makes sure we're not actually overriding another earlier deprecation notification without noticing.

kaspth · 2017-04-19T19:10:07Z

activesupport/lib/active_support/deprecation/reporting.rb

@@ -5,8 +5,6 @@ class Deprecation
    module Reporting
      # Whether to print a message (silent mode)
      attr_accessor :silenced
-      # Name of gem where method is deprecated
-      attr_accessor :gem_name


This module looks like it's public API so we can't just yank this.

Hardly it will break any behavior unless someone is including Reporting in their own class, what I'd not recommend anyway since this method depends on the others in Deprecation. I think this change is fine since the public API (ActiveSupport::Deprecation) did't changed.

I'll just revert this change. I noticed two other accessors defined on other modules as well. I feel they should be part of the main module given that it directly uses them in the initializer, but it's kind of out of scope for this PR.

maclover7

Agree with what @kaspth said -- we have to be careful with backwards compatibility here. Can you add a CHANGELOG.md entry, since we are modifying public API?

rafaelfranca · 2017-04-19T19:47:07Z

activesupport/lib/active_support/deprecation.rb

@@ -29,6 +29,9 @@ class Deprecation
    # The version number in which the deprecated behavior will be removed, by default.
    attr_accessor :deprecation_horizon

+    # Name of gem where method is deprecated


Missing . in the end of the text (in the method below too)

rafaelfranca · 2017-04-19T19:50:25Z

activesupport/lib/active_support/deprecation/behaviors.rb

-        ActiveSupport::Notifications.instrument("deprecation.rails",
-                                                message: message, callstack: callstack)
+      notify: ->(message, callstack, deprecation_horizon, gem_name) {
+        notification_name = "deprecation.#{gem_name.underscore.tr('/', '.')}"


Maybe _ instead of .? We don't want to mix the gem namespace with the notification namespace. deprecation.action_controller.test_helper seems more like a test_helper notification namespace instead of the gem namespace.

rafaelfranca · 2017-04-19T19:52:22Z

activesupport/lib/active_support/deprecation/reporting.rb

@@ -5,8 +5,6 @@ class Deprecation
    module Reporting
      # Whether to print a message (silent mode)
      attr_accessor :silenced
-      # Name of gem where method is deprecated
-      attr_accessor :gem_name


Hardly it will break any behavior unless someone is including Reporting in their own class, what I'd not recommend anyway since this method depends on the others in Deprecation. I think this change is fine since the public API (ActiveSupport::Deprecation) did't changed.

wvanbergen · 2017-04-19T22:18:26Z

Thanks for the fast reviews! Pushed some updates.

…vier handler, and make sure they are used for the ActiveSupport::Notifications message.

rafaelfranca · 2017-04-19T23:21:07Z

❤️ 💚 💙 💛 💜

The callback parameters need to reflect changes after rails#28800

Edouard-chin · 2018-03-22T02:10:29Z

activesupport/lib/active_support/deprecation/behaviors.rb

-        ActiveSupport::Notifications.instrument("deprecation.rails",
-                                                message: message, callstack: callstack)
+      notify: ->(message, callstack, deprecation_horizon, gem_name) {
+        notification_name = "deprecation.#{gem_name.underscore.tr('/', '_')}"


Sorry for digging this up after one year, adding the gem_name and the deprecation_horizon to the event is a great add however generating dynamically the notification name has one major disadvantage because that means we need to know in advance which gems have deprecations in order to keep track of them.

class MySubscriber < ActiveSupport::Subscriber attach_to :rails # Prior to this PR, any gems using AS deprecations could be tracked thanks to this subscriber. # Now we can't because we'd need to attach the subscriber to other namespace which we don't know, thus the concept of "notifying" when a deprecation occur (no matter inside which gem) no longer exists def deprecation(event) ... end end

My suggestion would be to use a static namespace like before (maybe change the rails namespace because it's not super accurate since any gems can trigger deprecation)

I was not a big fan of this, but this was required for backwards compatibility. I would have preferred to simply include the gem name as a parameter rather than in the event name.

I think you can use regular expressions for subscriptions, at least with the lower level API ActiveSupport::Notifications::Fanout API. That may not be propagated to ActiveSupport::Subscriber though.

Ah ok I see thanks for the explanation, and TIL indeed the lower level API Fanout can be passed a regex. I'm going to open a PR to make it possible for AS::Subscriber as well

we need to know in advance which gems have deprecations in order to keep track of them.

That was exactly the reason why we did this. We should not thread deprecation in Rails in the same way we treat deprecation in other gems. At the time of a Rails upgrade, we don't care if the gem X also has deprecations, only if Rails has deprecation. Because of this we need to have different subscriber to each gem so we can chose the behavior we want based on the gem.

This was updated in rails/rails#28800

wvanbergen force-pushed the deprecation_includes_gem_name_and_version branch from fa0d17a to e55f7ce Compare April 19, 2017 19:01

kaspth reviewed Apr 19, 2017

View reviewed changes

maclover7 suggested changes Apr 19, 2017

View reviewed changes

maclover7 added activesupport needs work labels Apr 19, 2017

rafaelfranca requested changes Apr 19, 2017

View reviewed changes

wvanbergen force-pushed the deprecation_includes_gem_name_and_version branch from e55f7ce to 448d44d Compare April 19, 2017 22:18

wvanbergen force-pushed the deprecation_includes_gem_name_and_version branch from 448d44d to 14eeee3 Compare April 19, 2017 22:22

Send deprecation horizon and gem name as arguments to deprecation hea…

0bdf6d7

…vier handler, and make sure they are used for the ActiveSupport::Notifications message.

wvanbergen force-pushed the deprecation_includes_gem_name_and_version branch from 14eeee3 to 0bdf6d7 Compare April 19, 2017 22:23

rafaelfranca approved these changes Apr 19, 2017

View reviewed changes

rafaelfranca merged commit 2b18153 into rails:master Apr 19, 2017

maclover7 removed the needs work label Apr 19, 2017

kirs deleted the deprecation_includes_gem_name_and_version branch April 20, 2017 13:12

fragoulis mentioned this pull request Feb 23, 2018

Correct method documentation #32093

Merged

fragoulis added a commit to fragoulis/rails that referenced this pull request Feb 23, 2018

Correct ActiveSupport::Deprecation::Behavior#behavior= documentation

d7f11ce

The callback parameters need to reflect changes after rails#28800

Edouard-chin reviewed Mar 22, 2018

View reviewed changes

Hamms added a commit to code-dot-org/code-dot-org that referenced this pull request Oct 19, 2020

Update the arguments passed to the Rails Deprecation warning

4020927

This was updated in rails/rails#28800

Hamms added a commit to code-dot-org/code-dot-org that referenced this pull request Nov 5, 2020

Update the arguments passed to the Rails Deprecation warning

2e9aacd

This was updated in rails/rails#28800

jonathanhefner mentioned this pull request Jul 5, 2022

Fix NoMethodError on custom ActiveSupport::Deprecation behavior #45521

Merged

Include gem name and deprecation horizon when calling the deprecation behaviors #28800

Include gem name and deprecation horizon when calling the deprecation behaviors #28800

Uh oh!

Conversation

wvanbergen commented Apr 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Uh oh!

kaspth left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wvanbergen Apr 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maclover7 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wvanbergen commented Apr 19, 2017

Uh oh!

rafaelfranca commented Apr 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wvanbergen Mar 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

wvanbergen commented Apr 19, 2017 •

edited

Loading

kaspth left a comment •

edited

Loading

wvanbergen Apr 19, 2017 •

edited

Loading

wvanbergen Mar 22, 2018 •

edited

Loading