FIX: Don’t list values from disabled plugins

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

@@ -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

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|

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

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