From 5ec227334a8b3e2016db19029458833d7b350343 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lo=C3=AFc=20Guitaut?= Date: Tue, 9 Jul 2024 17:56:22 +0200 Subject: [PATCH] =?UTF-8?q?FIX:=20Don=E2=80=99t=20list=20values=20from=20d?= =?UTF-8?q?isabled=20plugins?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Currently, when a plugin registers a new reviewable type or extends a list method (through `register_reviewble_type` and `extend_list_method` respectively), the new array is statically computed and always returns the same value. It will continue to return the same value even if the plugin is disabled (it can be a problem in a multisite env too). To address this issue, this patch changes how `extend_list_method` works. It’s now using `DiscoursePluginRegistry.define_filtered_register` to create a register on the fly and store the extra values from various plugins. It then combines the original values with the ones from the registry. The registry is already aware of disabled plugins, so when a plugin is disabled, its registered values won’t be returned. --- app/models/reviewable.rb | 7 ++-- lib/discourse_plugin_registry.rb | 8 +++-- lib/plugin/instance.rb | 22 +++++++++--- spec/lib/plugin/instance_spec.rb | 57 ++++++++++++++++++++++++++------ 4 files changed, 71 insertions(+), 23 deletions(-) diff --git a/app/models/reviewable.rb b/app/models/reviewable.rb index f46905217e3..d7ec2260fca 100644 --- a/app/models/reviewable.rb +++ b/app/models/reviewable.rb @@ -66,14 +66,11 @@ class Reviewable < ActiveRecord::Base end def self.valid_type?(type) - return false if Reviewable.types.exclude?(type) - type.constantize <= Reviewable - rescue NameError - false + type.to_s.safe_constantize.in?(types) end def self.types - %w[ReviewableFlaggedPost ReviewableQueuedPost ReviewableUser ReviewablePost] + [ReviewableFlaggedPost, ReviewableQueuedPost, ReviewableUser, ReviewablePost] end def self.custom_filters diff --git a/lib/discourse_plugin_registry.rb b/lib/discourse_plugin_registry.rb index cd4dd47b589..ee2c9786c2a 100644 --- a/lib/discourse_plugin_registry.rb +++ b/lib/discourse_plugin_registry.rb @@ -4,6 +4,8 @@ # A class that handles interaction between a plugin and the Discourse App. # class DiscoursePluginRegistry + @@register_names = Set.new + # Plugins often need to be able to register additional handlers, data, or # classes that will be used by core classes. This should be used if you # need to control which type the registry is, and if it doesn't need to @@ -15,7 +17,7 @@ class DiscoursePluginRegistry # - Defines instance method as a shortcut to the singleton method # - Automatically deletes the register on registry.reset! def self.define_register(register_name, type) - @@register_names ||= Set.new + return if respond_to?(register_name) @@register_names << register_name define_singleton_method(register_name) do @@ -36,13 +38,13 @@ class DiscoursePluginRegistry # - Defines instance method as a shortcut to the singleton method # - Automatically deletes the register on registry.reset! def self.define_filtered_register(register_name) + return if respond_to?(register_name) define_register(register_name, Array) singleton_class.alias_method :"_raw_#{register_name}", :"#{register_name}" define_singleton_method(register_name) do - unfiltered = public_send(:"_raw_#{register_name}") - unfiltered.filter { |v| v[:plugin].enabled? }.map { |v| v[:value] }.uniq + public_send(:"_raw_#{register_name}").filter_map { |h| h[:value] if h[:plugin].enabled? }.uniq end define_singleton_method("register_#{register_name.to_s.singularize}") do |value, plugin| diff --git a/lib/plugin/instance.rb b/lib/plugin/instance.rb index 058248f1241..10bbd34c802 100644 --- a/lib/plugin/instance.rb +++ b/lib/plugin/instance.rb @@ -831,14 +831,28 @@ class Plugin::Instance end def register_reviewable_type(reviewable_type_class) - extend_list_method Reviewable, :types, [reviewable_type_class.name] + return unless reviewable_type_class < Reviewable + extend_list_method(Reviewable, :types, reviewable_type_class) end def extend_list_method(klass, method, new_attributes) - current_list = klass.public_send(method) - current_list.concat(new_attributes) + register_name = [klass, method].join("_").underscore + DiscoursePluginRegistry.define_filtered_register(register_name) + DiscoursePluginRegistry.public_send( + "register_#{register_name.singularize}", + new_attributes, + self, + ) - reloadable_patch { klass.public_send(:define_singleton_method, method) { current_list } } + original_method_alias = "__original_#{method}__" + return if klass.respond_to?(original_method_alias) + reloadable_patch do + klass.singleton_class.alias_method(original_method_alias, method) + klass.define_singleton_method(method) do + public_send(original_method_alias) | + DiscoursePluginRegistry.public_send(register_name).flatten + end + end end def directory_name diff --git a/spec/lib/plugin/instance_spec.rb b/spec/lib/plugin/instance_spec.rb index 7bf68f1d8b9..2610e269076 100644 --- a/spec/lib/plugin/instance_spec.rb +++ b/spec/lib/plugin/instance_spec.rb @@ -599,25 +599,60 @@ TEXT end end - describe "#register_reviewable_types" do - it "Overrides the existing Reviewable types adding new ones" do - current_types = Reviewable.types - new_type_class = Class + describe "#register_reviewable_type" do + subject(:register_reviewable_type) { plugin_instance.register_reviewable_type(new_type) } - Plugin::Instance.new.register_reviewable_type new_type_class + context "when the provided class inherits from `Reviewable`" do + let(:new_type) { Class.new(Reviewable) } - expect(Reviewable.types).to match_array(current_types << new_type_class.name) + it "adds the provided class to the existing types" do + expect { register_reviewable_type }.to change { Reviewable.types.size }.by(1) + expect(Reviewable.types).to include(new_type) + end + + context "when the plugin is disabled" do + before do + register_reviewable_type + plugin_instance.stubs(:enabled?).returns(false) + end + + it "does not return the new type" do + expect(Reviewable.types).not_to be_blank + expect(Reviewable.types).not_to include(new_type) + end + end + end + + context "when the provided class does not inherit from `Reviewable`" do + let(:new_type) { Class } + + it "does not add the provided class to the existing types" do + expect { register_reviewable_type }.not_to change { Reviewable.types } + expect(Reviewable.types).not_to be_blank + end end end describe "#extend_list_method" do - it "Overrides the existing list appending new elements" do - current_list = Reviewable.types - new_element = Class.name + subject(:extend_list) do + plugin_instance.extend_list_method(UserHistory, :staff_actions, %i[new_action another_action]) + end - Plugin::Instance.new.extend_list_method Reviewable, :types, [new_element] + it "adds the provided values to the provided method on the provided class" do + expect { extend_list }.to change { UserHistory.staff_actions.size }.by(2) + expect(UserHistory.staff_actions).to include(:new_action, :another_action) + end - expect(Reviewable.types).to match_array(current_list << new_element) + context "when the plugin is disabled" do + before do + extend_list + plugin_instance.stubs(:enabled?).returns(false) + end + + it "does not return the provided values" do + expect(UserHistory.staff_actions).not_to be_blank + expect(UserHistory.staff_actions).not_to include(:new_action, :another_action) + end end end