Change internal mapping for event subscriber registrations #3758

spaghetticode · 2020-09-11T15:27:42Z

The previous way of registering subscribers could generate some issues with rails 6 applications, due to how Zeitwerk (re)loads classes in initializers. The previous approach was also scattering the information into every single subscriber, while now it's centralized in a single place.

Currently, Spree::Event.subscribers includes only the list of subscriber classes, while the rest of the information is delegated to each subscriber. This is how information can be accessed with the current interface:

# list of subscribers:
$ Spree::Event.subscribers
 => #<Spree::Core::ClassConstantizer::Set:0x00007ff62dd69a18 @collection=#<Set: {"AppEventSubscriber", "Spree::MailerSubscriber"}>> 

# internal mapping for AppEventSubscriber:
$ AppEventSubscriber.event_actions
 => {:app_event=>"app_event"} 

# mapping to the ActiveSupport::Notifications subscription:

```ruby
AppEventSubscriber.app_event_handler
 => #<ActiveSupport::Notifications::Fanout::Subscribers::Timed:0x00007ff67e582880 @pattern="app_event.spree", @delegate=#<Proc:0x00007ff67e582df8@/Users/andrea/nebulab/solidus_event_subscribers_autoload/vendor/gems/solidus/core/lib/spree/event/adapters/active_support_notifications.rb:18>, @can_publish=false>

After merging this PR, the functionality provided by Spree::Event.subscribers will be managed by the new Spree::Event.subscriber_registry. The registry stores information with the format:

{ subscriber_class_name => { subscriber_method => ActiveSupportNotifications_subscription } }

Real world example:

pp Spree::Event.subscriber_registry
#<Spree::Event::SubscriberRegistry:0x00007fce3e80d798
 @registry=
  {"Spree::MailerSubscriber"=>
    {:order_finalized=>
      #<ActiveSupport::Notifications::Fanout::Subscribers::Timed:0x00007fce3ec86e78
       @can_publish=false,
       @delegate=
        #<Proc:0x00007fce3ec86f40@/Users/andrea/nebulab/solidus/core/lib/spree/event/adapters/active_support_notifications.rb:18>,
       @pattern="order_finalized.spree">,
     :send_reimbursement_email=>
      #<ActiveSupport::Notifications::Fanout::Subscribers::Timed:0x00007fce3ec86a18
       @can_publish=false,
       @delegate=
        #<Proc:0x00007fce3ec86ab8@/Users/andrea/nebulab/solidus/core/lib/spree/event/adapters/active_support_notifications.rb:18>,
       @pattern="reimbursement_reimbursed.spree">},
   "AppSubscriber"=>
    {:app_event=>
      #<ActiveSupport::Notifications::Fanout::Subscribers::Timed:0x00007fce3ec851e0
       @can_publish=false,
       @delegate=
        #<Proc:0x00007fce3ec85280@/Users/andrea/nebulab/solidus/core/lib/spree/event/adapters/active_support_notifications.rb:18>,
       @pattern="app_event.spree">}},
 @semaphore=#<Thread::Mutex:0x00007fce3e80d6a8>

This PR requires a small change in solidus_support too, see solidusio/solidus_support#53

These changes should be transparent to stores as they only modify how event subscribers are mapped and managed internally in Solidus.

Many thanks to @cedum and @DanielePalombo who helped in moving forward the PR.

Checklist:

I have followed Pull Request guidelines
I have added a detailed description into each commit message
I have added tests to cover this change (if needed)

core/spec/lib/spree/event_spec.rb

core/lib/spree/testing_support/preferences.rb

core/spec/lib/spree/event_spec.rb

core/lib/spree/event/subscriber.rb

The new method `Spree::Event.subscribe_subscribers!` is introduced in Solidus with solidusio/solidus#3758 as a better way to subscribe event subscribers. Under the hood, event subscribers use new interal mappings. We need to keep around the old interface as well until it won't be supported anymore in Solidus.

The new method `Spree::Event.subscribe_subscribers!` is introduced in Solidus with solidusio/solidus#3758 as a better way to subscribe event subscribers: under the hood, event subscribers use new interal mappings. Also, we need to keep around the old interface as well until it won't be supported anymore in Solidus.

kennyadsl

Left some comments, I think we are close to a solution, thanks @spaghetticode !

kennyadsl · 2020-09-25T10:22:26Z

core/lib/spree/event/subscriber.rb

-            send event_action, event
-          }
+          subscription = Spree::Event.subscribe(event_name) { |event| send event_action, event }
+          # old mappings, to be removed when Solidus 2.10 is not supported anymore:


Can we use the word deprecated instead of old? Just because it will be easier to find it if we perform a search of all the deprecated code within the codebase with a text editor.

kennyadsl · 2020-09-25T10:23:38Z

core/lib/spree/event/subscriber.rb

+          subscription = Spree::Event.subscribe(event_name) { |event| send event_action, event }
+          # old mappings, to be removed when Solidus 2.10 is not supported anymore:
+          send "#{event_action}_handler=", subscription
+          # new mappings:


I think we can get rid of this comment, probably improving a little bit the one with the deprecated code to explain that this other way is the new way of doing things.

kennyadsl · 2020-09-25T10:28:31Z

core/lib/spree/testing_support/preferences.rb

@@ -14,7 +14,9 @@ module Preferences
      #
      # @deprecated
      def reset_spree_preferences(&config_block)
-        Spree::Config.instance_variables.each { |iv| Spree::Config.remove_instance_variable(iv) }
+        Spree::Config.instance_variables.
+          reject { |iv| iv == :@events_configuration }.


I'm not sure about this. What if some specs need to reset that configuration? It's applicable? How do people know that all the preferences except that one will be reset when calling reset_spree_preferences in their code?

This helper method is deprecated, so it won't be around for long. We don't use it anymore in our codebase, but we have some specs that verify its behavior. I'm not sure if creating a better workaround for making it work properly with the new event mappings is worth the effort at this point.

Oh right, it's deprecated. 👍

kennyadsl · 2020-09-25T10:35:29Z

core/lib/spree/core/engine.rb

-          Spree::Event.subscribers.each_key do |klass|
-            klass.constantize.subscribe!
-          end
+          Spree::Event.subscribe_subscribers!


The more I look into this and the more I think we need another class called Spree::Event::Subscribers that acts as a collection of subscribers. No matter if it's hash inside but the interface will probably look better, having something like:

Spree::Event.subscribers.subscribe_all.

WDYT?

sure, makes sense! I'll work on your suggestion 👍

kennyadsl · 2020-09-25T10:38:00Z

core/lib/spree/event/subscriber.rb

@@ -54,7 +54,14 @@ def self.included(base)
      #     end
      #   end
      def event_action(method_name, event_name: nil)
-        mattr_accessor "#{method_name}_handler"
+        mattr_writer "#{method_name}_handler"


Why don't we need to deprecate the writer as well?

We still use the writer internally to keep compatibility, if somebody is still using the reader in their codebase (we don't use the reader anymore). So, if we deprecate the writer, then we'll need to silence the deprecation when using it. So, deprecating the writer would be impractical at this point.

But I think we don't really need to deprecate the writer as well. The only reason for some developers to be using the writer in their codebase is to set a value to be accessed from the reader. When this happens, they will see the deprecation when the matching reader is called.

Makes sense, thanks Andrea.

core/lib/spree/event/subscribers.rb

core/spec/lib/spree/event/subscribers_spec.rb

kennyadsl · 2020-10-05T14:55:00Z

core/lib/spree/event/subscriber.rb

-        event_actions.keys.each do |event_action|
-          Spree::Event.unsubscribe send("#{event_action}_handler")
-        end
+        Spree::Deprecation.warn("#{name}.unsubscribe! is deprecated. Please move your event subscribers to folder `app/subscribers/*_subscriber.rb` and they will be unloaded automatically. ", caller)


I think the message here is incorrect. You have to delete the subscriber file to unsubscribe, isn't it? Also, I'd keep the method since maybe you don't have control over where the file has been added (core or an extension).

Yes, you're right about the message. And also the remainder of the comment is right. I think we need to keep offering that kind of granularity, even though it will be rarely used.

core/spec/lib/spree/event/subscriber_registry_spec.rb

core/lib/spree/event/subscriber.rb

core/spec/lib/spree/event/subscriber_spec.rb

The class `Spree::Event::SubscriberRegistry` is introduced in order to centralize and handle in a dedicated environment event mappings for subscriber modules. This was previously responsibility of each `Spree::Event::Subscriber` module, but due to some issues with Zeitwerk and the way these modules were activated in rails initializers, that approach is not working properly anymore with Rails 6. This class also manages the activation/deactivation of all event subscribers that happens at boot time. The activation/deactivation code is threadsafe, just in the remote case that somebody tries to use it in a multithreaded context. This does not happen in the standard use-case of this class, as it is being used during the app initialization time.

Subscribers are loaded once at runtime during the app initialization process, so if they get reset by `reset_spree_preferences` then the original value will not come back.

The event config setting `Spree::Event.config.subscribers` (accessible via Spree::Event.subscribers) is now deprecated in favor of the new subscriber registry.

`Spree::Event.subscribers` is deprecated and its functionality is replaced by `.subscriber_registry`. `.subscriber_registry` is not a 100% compatible replacement for `.subscribers`, as the internal mapping system for event subscribers is changing completely, but for existing Solidus stores the change should be transparent enough. `Spree::Event.require_subscriber_files` is deprecated and delegated to the subscriber registry object. The method with the same name defined in `Spree::Event::SubscriberRegistry` is now private, as there is no use in keeping it public.

As `Spree::Event.subscribers` is deprecated and being replaced by `Spree::Event.subscriber_registry`.

`Spree::Event::Subscriber.subscribe!` is deprecated and replaced by `::activate`. `Spree::Event::Subscriber.unsubscribe!` is deprecated and replaced by `::deactivate`. The method names `subscribe` and `unsubscribe` are already used by `Spree::Event`, so the naming was a bit confusing; `activate` and `deactivate` should help in making this a bit more clear. `::activate` and `::deacticate` are now using the new subscriber registry for activation/deactivation, as `Spree::Event.subscribers` is now deprecated.

We're keeping the old event mappings handler methods, as somebody may still be relying on them, but they're now deprecated.

kennyadsl

Found something that looks like a mistake, let me know if I'm missing something instead.

core/lib/spree/core/engine.rb

The methods `Spree::Event.activate_autoloadable_subscribers` and `Spree::Event.deactivate_all_subscribers` encapsulate in a single call all the logic needed to activate event subscribers during the app init process. Those methods will need to be used also in the engine.rb file provided by the gem `solidus_support`.

The new method `Spree::Event.activate_all_subscribers` is introduced in Solidus with solidusio/solidus#3758 as a better way to subscribe event subscribers. Under the hood, event subscribers use new interal mappings. Also, we need to keep around the old interface as well until it won't be supported anymore in Solidus.

spaghetticode · 2020-10-19T16:30:01Z

@kennyadsl thanks for spotting the bug ❤️

I have just tested this PR with a newly generated sandbox app using solidus_support with this branch, and it seems to be working properly when adding a custom autoloadable subscriber.

kennyadsl

Thanks @spaghetticode!

The new method `Spree::Event.activate_all_subscribers` is introduced in Solidus with solidusio/solidus#3758 as a better way to subscribe event subscribers. Under the hood, event subscribers use new interal mappings. Also, we need to keep around the old interface as well until it won't be supported anymore in Solidus.

elia · 2020-12-21T14:11:16Z

I'm getting these deprecations while running a generator (although I don't believe that to be relevant), I think the caller should be refined to point at the right level of the backtrace, instead of showing -e.

DEPRECATION WARNING: `Spree::Event.subscribers` is deprecated. Please use `Spree::Event.subscriber_registry` instead. (called from <main> at -e:1)
DEPRECATION WARNING: `Spree::Config.events.subscribers` is deprecated. Please use `Spree::Config.events.subscriber_registry`. (called from <main> at -e:1)
DEPRECATION WARNING: Spree::MailerSubscriber.subscribe! is deprecated. Please use `Spree::MailerSubscriber.activate`. (called from <main> at -e:1)
DEPRECATION WARNING: `Spree::Event.subscribers` is deprecated. Please use `Spree::Event.subscriber_registry` instead. (called from <main> at -e:1)
DEPRECATION WARNING: `Spree::Config.events.subscribers` is deprecated. Please use `Spree::Config.events.subscriber_registry`. (called from <main> at -e:1)
DEPRECATION WARNING: Spree::MailerSubscriber.subscribe! is deprecated. Please use `Spree::MailerSubscriber.activate`. (called from <main> at -e:1)
DEPRECATION WARNING: `Spree::Event.subscribers` is deprecated. Please use `Spree::Event.subscriber_registry` instead. (called from <main> at -e:1)
DEPRECATION WARNING: `Spree::Config.events.subscribers` is deprecated. Please use `Spree::Config.events.subscriber_registry`. (called from <main> at -e:1)
DEPRECATION WARNING: Spree::MailerSubscriber.subscribe! is deprecated. Please use `Spree::MailerSubscriber.activate`. (called from <main> at -e:1)
DEPRECATION WARNING: `Spree::Event.subscribers` is deprecated. Please use `Spree::Event.subscriber_registry` instead. (called from <main> at -e:1)
DEPRECATION WARNING: `Spree::Config.events.subscribers` is deprecated. Please use `Spree::Config.events.subscriber_registry`. (called from <main> at -e:1)
DEPRECATION WARNING: Spree::MailerSubscriber.subscribe! is deprecated. Please use `Spree::MailerSubscriber.activate`. (called from <main> at -e:1)

Unless I'm mistaken and this is correct for some reason we should probably turn this comment into an issue

cc @spaghetticode

spaghetticode self-assigned this Sep 11, 2020

houndci-bot reviewed Sep 11, 2020

View reviewed changes

core/spec/lib/spree/event_spec.rb Outdated Show resolved Hide resolved

spaghetticode mentioned this pull request Sep 11, 2020

Event subscribers loading Issue (3757) nebulab/solidus#50

Closed

spaghetticode force-pushed the spaghetticode/event-subscribers-mappings branch from 310ef32 to 75e0d99 Compare September 11, 2020 16:17

houndci-bot reviewed Sep 18, 2020

View reviewed changes

core/lib/spree/testing_support/preferences.rb Outdated Show resolved Hide resolved

spaghetticode force-pushed the spaghetticode/event-subscribers-mappings branch from 91eaf64 to a20b488 Compare September 18, 2020 12:48

houndci-bot reviewed Sep 18, 2020

View reviewed changes

core/spec/lib/spree/event_spec.rb Outdated Show resolved Hide resolved

spaghetticode force-pushed the spaghetticode/event-subscribers-mappings branch 4 times, most recently from 2cfe4be to 1304862 Compare September 18, 2020 15:45

houndci-bot reviewed Sep 25, 2020

View reviewed changes

core/lib/spree/event/subscriber.rb Outdated Show resolved Hide resolved

core/lib/spree/event/subscriber.rb Outdated Show resolved Hide resolved

spaghetticode force-pushed the spaghetticode/event-subscribers-mappings branch 2 times, most recently from a090359 to 1bbe5b1 Compare September 25, 2020 07:55

kennyadsl added this to the 2.11 milestone Sep 25, 2020

spaghetticode mentioned this pull request Sep 25, 2020

Use Spree::Event. activate_all_subscribers when available solidusio/solidus_support#53

Merged

spaghetticode added the Needs Core Team Review label Sep 25, 2020

spaghetticode marked this pull request as ready for review September 25, 2020 09:38

kennyadsl reviewed Sep 25, 2020

View reviewed changes

kennyadsl added the changelog:solidus_core Changes to the solidus_core gem label Sep 25, 2020

spaghetticode force-pushed the spaghetticode/event-subscribers-mappings branch from 1bbe5b1 to c49b9f5 Compare October 2, 2020 12:53

houndci-bot reviewed Oct 2, 2020

View reviewed changes

core/lib/spree/event/subscribers.rb Outdated Show resolved Hide resolved

spaghetticode force-pushed the spaghetticode/event-subscribers-mappings branch 3 times, most recently from 2c48a9f to be6e796 Compare October 2, 2020 15:32

houndci-bot reviewed Oct 2, 2020

View reviewed changes

core/spec/lib/spree/event/subscribers_spec.rb Outdated Show resolved Hide resolved

spaghetticode force-pushed the spaghetticode/event-subscribers-mappings branch from be6e796 to b2a8847 Compare October 2, 2020 15:34

kennyadsl reviewed Oct 5, 2020

View reviewed changes

spaghetticode force-pushed the spaghetticode/event-subscribers-mappings branch from b2a8847 to 0fa6598 Compare October 16, 2020 08:31

houndci-bot reviewed Oct 16, 2020

View reviewed changes

core/spec/lib/spree/event/subscriber_registry_spec.rb Show resolved Hide resolved

core/lib/spree/event/subscriber.rb Outdated Show resolved Hide resolved

spaghetticode force-pushed the spaghetticode/event-subscribers-mappings branch 2 times, most recently from 5d36a67 to 465a8bf Compare October 16, 2020 10:55

houndci-bot reviewed Oct 16, 2020

View reviewed changes

core/spec/lib/spree/event/subscriber_spec.rb Outdated Show resolved Hide resolved

spaghetticode force-pushed the spaghetticode/event-subscribers-mappings branch 3 times, most recently from 880bcde to 996bd4f Compare October 16, 2020 15:05

DanielePalombo added 2 commits October 16, 2020 17:46

Do not clear subscribers when resetting preferences

c4af8bf

Subscribers are loaded once at runtime during the app initialization process, so if they get reset by `reset_spree_preferences` then the original value will not come back.

spaghetticode force-pushed the spaghetticode/event-subscribers-mappings branch from 996bd4f to 6816374 Compare October 16, 2020 15:54

DanielePalombo and others added 5 commits October 16, 2020 18:22

Deprecate Spree::Event.config.subscribers

5d81ecc

The event config setting `Spree::Event.config.subscribers` (accessible via Spree::Event.subscribers) is now deprecated in favor of the new subscriber registry.

Use subscriber registry for registering event subscribers

026af1b

As `Spree::Event.subscribers` is deprecated and being replaced by `Spree::Event.subscriber_registry`.

Deprecate old event subscriber internal mappings methods

63aeaef

We're keeping the old event mappings handler methods, as somebody may still be relying on them, but they're now deprecated.

spaghetticode force-pushed the spaghetticode/event-subscribers-mappings branch from 6816374 to 6e96808 Compare October 16, 2020 16:24

spaghetticode removed the Needs Work label Oct 16, 2020

kennyadsl requested changes Oct 19, 2020

View reviewed changes

core/lib/spree/core/engine.rb Outdated Show resolved Hide resolved

spaghetticode force-pushed the spaghetticode/event-subscribers-mappings branch 2 times, most recently from c49b9f5 to b2a5a03 Compare October 19, 2020 15:42

kennyadsl approved these changes Oct 21, 2020

View reviewed changes

jarednorman approved these changes Oct 23, 2020

View reviewed changes

kennyadsl merged commit 8c0a907 into solidusio:master Oct 23, 2020

kennyadsl deleted the spaghetticode/event-subscribers-mappings branch October 23, 2020 08:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change internal mapping for event subscriber registrations #3758

Change internal mapping for event subscriber registrations #3758

spaghetticode commented Sep 11, 2020 •

edited

kennyadsl left a comment

kennyadsl Sep 25, 2020

kennyadsl Sep 25, 2020

kennyadsl Sep 25, 2020

spaghetticode Sep 25, 2020

kennyadsl Sep 25, 2020

kennyadsl Sep 25, 2020

spaghetticode Sep 25, 2020

kennyadsl Sep 25, 2020

spaghetticode Sep 25, 2020

kennyadsl Sep 25, 2020

kennyadsl Oct 5, 2020

spaghetticode Oct 7, 2020

kennyadsl left a comment

spaghetticode commented Oct 19, 2020

kennyadsl left a comment

elia commented Dec 21, 2020

Change internal mapping for event subscriber registrations #3758

Change internal mapping for event subscriber registrations #3758

Conversation

spaghetticode commented Sep 11, 2020 • edited

kennyadsl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kennyadsl left a comment

Choose a reason for hiding this comment

spaghetticode commented Oct 19, 2020

kennyadsl left a comment

Choose a reason for hiding this comment

elia commented Dec 21, 2020

spaghetticode commented Sep 11, 2020 •

edited