Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change internal mapping for event subscriber registrations #3758

Merged
5 changes: 2 additions & 3 deletions core/lib/spree/core/engine.rb
Expand Up @@ -47,12 +47,11 @@ class Engine < ::Rails::Engine
# Setup Event Subscribers
initializer 'spree.core.initialize_subscribers' do |app|
app.reloader.to_prepare do
Spree::Event.require_subscriber_files
Spree::Event.subscribers.each(&:subscribe!)
Spree::Event.activate_autoloadable_subscribers
end

app.reloader.before_class_unload do
Spree::Event.subscribers.each(&:unsubscribe!)
Spree::Event.deactivate_all_subscribers
end
end

Expand Down
29 changes: 15 additions & 14 deletions core/lib/spree/event.rb
@@ -1,13 +1,16 @@
# frozen_string_literal: true

require_relative 'event/adapters/active_support_notifications'
require_relative 'event/subscriber_registry'
require_relative 'event/configuration'
require_relative 'event/subscriber'

module Spree
module Event
extend self

delegate :activate_autoloadable_subscribers, :activate_all_subscribers, :deactivate_all_subscribers, to: :subscriber_registry

# Allows to trigger events that can be subscribed using #subscribe. An
# optional block can be passed that will be executed immediately. The
# actual code implementation is delegated to the adapter.
Expand All @@ -26,8 +29,8 @@ def fire(event_name, opts = {})
end
end

# Loads all Solidus' core and application's event subscribers files.
# The latter are loaded automatically only when the preference
# @deprecated Loads all Solidus' core and application's event subscribers files.
# The latter are loaded automatically only when the preference
# Spree::Config.events.autoload_subscribers is set to a truthy value.
#
# Files must be placed under the directory `app/subscribers` and their
Expand All @@ -36,18 +39,8 @@ def fire(event_name, opts = {})
# Loading the files has the side effect of adding their module to the
# list in Spree::Event.subscribers.
def require_subscriber_files
pattern = "app/subscribers/**/*_subscriber.rb"

# Load Solidus subscribers
# rubocop:disable Rails/DynamicFindBy
solidus_core_dir = Gem::Specification.find_by_name('solidus_core').gem_dir
# rubocop:enable Rails/DynamicFindBy
Dir.glob(File.join(solidus_core_dir, pattern)) { |c| require_dependency(c.to_s) }

# Load application subscribers, only when the flag is set to true:
if Spree::Config.events.autoload_subscribers
Rails.root.glob(pattern) { |c| require_dependency(c.to_s) }
end
Spree::Deprecation.warn("#{self}.require_subscriber_files is deprecated and will be removed in Solidus 3.0.", caller)
subscriber_registry.send(:require_subscriber_files)
end

# Subscribe to an event with the given name. The provided block is executed
Expand Down Expand Up @@ -130,12 +123,20 @@ def suffix
Spree::Config.events.suffix
end

# @deprecated
# @!attribute [r] subscribers
# @return [Array<Spree::Event::Subscriber>] A list of subscribers used to support class reloading for Spree::Event::Subscriber instances
def subscribers
Spree::Deprecation.warn("`#{self}.subscribers` is deprecated. Please use `#{self}.subscriber_registry` instead.", caller)
Spree::Config.events.subscribers
end

# @!attribute [r] subscribers
# @return <Spree::Event::SubscriberRegistry> The registry for supporting class reloading for Spree::Event::Subscriber instances
def subscriber_registry
Spree::Config.events.subscriber_registry
end

private

def normalize_name(name)
Expand Down
9 changes: 6 additions & 3 deletions core/lib/spree/event/configuration.rb
@@ -1,12 +1,15 @@
# frozen_string_literal: true

require 'spree/core/class_constantizer'

module Spree
module Event
class Configuration
def subscriber_registry
@subscriber_registry ||= Spree::Event::SubscriberRegistry.new
end

def subscribers
@subscribers ||= ::Spree::Core::ClassConstantizer::Set.new
Spree::Deprecation.warn("`Spree::Config.events.subscribers` is deprecated. Please use `Spree::Config.events.subscriber_registry`.", caller)
subscriber_registry.send(:registry).keys.map { |module_name| module_name.constantize }
end

attr_writer :adapter, :suffix, :autoload_subscribers
Expand Down
56 changes: 37 additions & 19 deletions core/lib/spree/event/subscriber.rb
Expand Up @@ -19,15 +19,20 @@ module Event
# end
# end
#
# EmailSender.subscribe!
# # Optional, required only when the subscriber needs to be loaded manually.
# #
# # If Spree::Config.events.autoload_subscribers is set to `true` and the module
# # file matches the pattern `app/subscribers/**/*_subscriber.rb` then it will
# # be loaded automatically at boot and this line can be removed:
# EmailSender.activate
module Subscriber
def self.included(base)
base.extend base

base.mattr_accessor :event_actions
base.event_actions = {}

Spree::Event.subscribers << base.name
Spree::Event.subscriber_registry.register(base)
end

# Declares a method name in the including module that can be subscribed/unsubscribed
Expand All @@ -54,32 +59,45 @@ def self.included(base)
# end
# end
def event_action(method_name, event_name: nil)
mattr_accessor "#{method_name}_handler"
mattr_writer "#{method_name}_handler"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, thanks Andrea.


define_method "#{method_name}_handler" do
Spree::Deprecation.warn("#{name}.#{method_name}_handler and #{name}.#{method_name}_handler= from the old events mapping interface are deprecated. Please use the new mapping stored in Spree::Event.subscribers.", caller)

class_variable_get("@@#{method_name}_handler")
end

event_actions[method_name] = (event_name || method_name).to_s
end

# Subscribes all declared event actions to their events. Only actions that are subscribed
# Activates all declared event actions to their events. Only actions that are activated
# will be called when their event fires.
#
# @example subscribe all event actions for module 'EmailSender'
# EmailSender.subscribe!
# @example activate all event actions for module 'EmailSender'
# EmailSender.activate
def activate
Spree::Event.subscriber_registry.activate_subscriber(self)
end

# Deactivates all declared event actions (or a single specific one) from their events.
# This means that when an event fires then none of its unsubscribed event actions will
# be called.
# @example deactivate all event actions for module 'EmailSender'
# EmailSender.deactivate
# @example deactivate only order_finalized for module 'EmailSender'
# EmailSender.deactivate(:order_finalized)
def deactivate(event_action_name = nil)
Spree::Event.subscriber_registry.deactivate_subscriber(self, event_action_name)
end

def subscribe!
unsubscribe!
event_actions.each do |event_action, event_name|
send "#{event_action}_handler=", Spree::Event.subscribe(event_name) { |event|
send event_action, event
}
end
Spree::Deprecation.warn("#{self}.subscribe! is deprecated. Please use `#{self}.activate`.", caller)
activate
end

# Unsubscribes all declared event actions from their events. This means that when an event
# fires then none of its unsubscribed event actions will be called.
# @example unsubscribe all event actions for module 'EmailSender'
# EmailSender.unsubscribe!
def unsubscribe!
event_actions.keys.each do |event_action|
Spree::Event.unsubscribe send("#{event_action}_handler")
end
Spree::Deprecation.warn("#{self}.unsubscribe! is deprecated. Please use `#{self}.deactivate`.", caller)
deactivate
end
end
end
Expand Down
92 changes: 92 additions & 0 deletions core/lib/spree/event/subscriber_registry.rb
@@ -0,0 +1,92 @@
# frozen_string_literal: true

module Spree
module Event
class SubscriberRegistry
def initialize
@registry = {}
@semaphore = Mutex.new
end

def register(subscriber)
registry[subscriber.name] ||= {}
end

def activate_autoloadable_subscribers
require_subscriber_files
activate_all_subscribers
end

def activate_all_subscribers
registry.each_key { |subscriber_name| activate_subscriber(subscriber_name.constantize) }
end

def deactivate_all_subscribers
registry.each_key { |subscriber_name| deactivate_subscriber(subscriber_name.constantize) }
end

def activate_subscriber(subscriber)
return unless registry[subscriber.name]

subscriber.event_actions.each do |event_action, event_name|
@semaphore.synchronize do
unsafe_deactivate_subscriber(subscriber, event_action)

subscription = Spree::Event.subscribe(event_name) { |event| subscriber.send(event_action, event) }

# deprecated mappings, to be removed when Solidus 2.10 is not supported anymore:
subscriber.send("#{event_action}_handler=", subscription)

registry[subscriber.name][event_action] = subscription
end
end
end

def deactivate_subscriber(subscriber, event_action_name = nil)
@semaphore.synchronize do
unsafe_deactivate_subscriber(subscriber, event_action_name)
end
end

private

attr_reader :registry

# Loads all Solidus' core and application's event subscribers files.
# The latter are loaded automatically only when the preference
# Spree::Config.events.autoload_subscribers is set to a truthy value.
#
# Files must be placed under the directory `app/subscribers` and their
# name must end with `_subscriber.rb`.
#
# Loading the files has the side effect of adding their module to the
# list in Spree::Event.subscribers.
def require_subscriber_files
pattern = "app/subscribers/**/*_subscriber.rb"

# Load Solidus subscribers
# rubocop:disable Rails/DynamicFindBy
solidus_core_dir = Gem::Specification.find_by_name('solidus_core').gem_dir
# rubocop:enable Rails/DynamicFindBy
Dir.glob(File.join(solidus_core_dir, pattern)) { |c| require_dependency(c.to_s) }

# Load application subscribers, only when the flag is set to true:
if Spree::Config.events.autoload_subscribers
Rails.root.glob(pattern) { |c| require_dependency(c.to_s) }
end
end

def unsafe_deactivate_subscriber(subscriber, event_action_name = nil)
to_unsubscribe = Array.wrap(event_action_name || subscriber.event_actions.keys)

to_unsubscribe.each do |event_action|
if (subscription = registry.dig(subscriber.name, event_action))
Spree::Event.unsubscribe(subscription)

registry[subscriber.name].delete(event_action)
end
end
end
end
end
end
4 changes: 3 additions & 1 deletion core/lib/spree/testing_support/preferences.rb
Expand Up @@ -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 }.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh right, it's deprecated. 👍

each { |iv| Spree::Config.remove_instance_variable(iv) }
Spree::Config.preference_store = Spree::Config.default_preferences

if defined?(Railties)
Expand Down