FEATURE: Add cops for plugins

This patch adds some new rules to have saner defaults for our plugins.
These rules should help in particular with disabled plugins.

The new cops are:

* Discourse/Plugins/CallRequiresPlugin: checks `requires_plugin` is
  called in controllers.
* Discourse/Plugins/DiscourseEvent: checks `on` is called instead of
  `DiscourseEvent.on`.
* Discourse/Plugins/NamespaceConstants: checks constants are not defined
  outside the plugin namespace.
* Discourse/Plugins/NamespaceMethods: checks methods are not defined
  outside the plugin namespace.
* Discourse/Plugins/NoMonkeyPatching: checks existing classes are not
  patched in `plugin.rb` using `class_eval`.
* Discourse/Plugins/UseRequireRelative: checks `load` is not used to
  load dependencies.

.rubocop.yml

@@ -1,2 +1,9 @@
 inherit_from:
   - default.yml
+
+AllCops:
+  inherit_mode:
+    merge:
+      - Exclude
+  Exclude:
+    - "**/spec/fixtures/**/*"

config/default.yml

@@ -62,3 +62,31 @@ Discourse/NoMixingMultisiteAndStandardSpecs:
   Patterns:
     - _spec.rb
     - '(?:^|/)spec/'
+
+Discourse/Plugins/CallRequiresPlugin:
+  Enabled: true
+  Include:
+    - 'app/controllers/**/*'
+
+Discourse/Plugins/UsePluginInstanceOn:
+  Enabled: true
+
+Discourse/Plugins/NamespaceMethods:
+  Enabled: true
+  Exclude:
+    - '**/spec/**/*'
+    - '**/tasks/**/*.rake'
+    - '**/db/fixtures/**/*'
+
+Discourse/Plugins/NamespaceConstants:
+  Enabled: true
+  Exclude:
+    - '**/spec/**/*'
+    - '**/tasks/**/*.rake'
+    - '**/db/fixtures/**/*'
+
+Discourse/Plugins/UseRequireRelative:
+  Enabled: true
+
+Discourse/Plugins/NoMonkeyPatching:
+  Enabled: true

lib/rubocop-discourse.rb

@@ -1,6 +1,8 @@
 # frozen_string_literal: true
 
 require "rubocop"
+require "active_support"
+require "active_support/core_ext/string/inflections"
 require_relative "rubocop/discourse"
 require_relative "rubocop/discourse/inject"
 

lib/rubocop/cop/discourse/plugins/call_requires_plugin.rb

@@ -0,0 +1,71 @@
+# frozen_string_literal: true
+
+module RuboCop
+  module Cop
+    module Discourse
+      module Plugins
+        # Plugin controllers must call `requires_plugin` to prevent routes from
+        # being accessible when the plugin is disabled.
+        #
+        # @example
+        #   # bad
+        #   class MyController
+        #     def my_action
+        #     end
+        #   end
+        #
+        #   # good
+        #   class MyController
+        #     requires_plugin PLUGIN_NAME
+        #     def my_action
+        #     end
+        #   end
+        #
+        class CallRequiresPlugin < Base
+          MSG =
+            "Use `requires_plugin` in controllers to prevent routes from being accessible when plugin is disabled."
+
+          def_node_matcher :requires_plugin_present?, <<~MATCHER
+            (class _ _
+              {
+                (begin <(send nil? :requires_plugin _) ...>)
+                <(send nil? :requires_plugin _) ...>
+              }
+            )
+          MATCHER
+
+          def on_class(node)
+            return if requires_plugin_present?(node)
+            return if requires_plugin_present_in_parent_classes(node)
+            add_offense(node, message: MSG)
+          end
+
+          private
+
+          def requires_plugin_present_in_parent_classes(node)
+            return unless processed_source.path
+            controller_path =
+              base_controller_path(node.parent_class&.const_name.to_s)
+            return unless controller_path
+            Commissioner
+              .new([self.class.new(config, @options)])
+              .investigate(parse(controller_path.read, controller_path.to_s))
+              .offenses
+              .empty?
+          end
+
+          def base_controller_path(base_class)
+            return if base_class.blank?
+            base_path = "#{base_class.underscore}.rb"
+            path = Pathname.new("#{processed_source.path}/../").cleanpath
+            until path.root?
+              controller_path = path.join(base_path)
+              return controller_path if controller_path.exist?
+              path = path.join("..").cleanpath
+            end
+          end
+        end
+      end
+    end
+  end
+end

lib/rubocop/cop/discourse/plugins/namespace_constants.rb

@@ -0,0 +1,35 @@
+# frozen_string_literal: true
+
+module RuboCop
+  module Cop
+    module Discourse
+      module Plugins
+        # Constants must be defined inside the plugin namespace (module or class).
+        #
+        # @example
+        #   # bad
+        #   MY_CONSTANT = :value
+        #
+        #   # good
+        #   module MyPlugin
+        #     MY_CONSTANT = :value
+        #   end
+        #
+        class NamespaceConstants < Base
+          MSG = "Don’t define constants outside a class or a module."
+
+          def on_casgn(node)
+            return if inside_namespace?(node)
+            add_offense(node, message: MSG)
+          end
+
+          private
+
+          def inside_namespace?(node)
+            node.each_ancestor.detect { _1.class_type? || _1.module_type? }
+          end
+        end
+      end
+    end
+  end
+end

lib/rubocop/cop/discourse/plugins/namespace_methods.rb

@@ -0,0 +1,37 @@
+# frozen_string_literal: true
+
+module RuboCop
+  module Cop
+    module Discourse
+      module Plugins
+        # Methods must be defined inside the plugin namespace (module or class).
+        #
+        # @example
+        #   # bad
+        #   def my_method
+        #   end
+        #
+        #   # good
+        #   module MyPlugin
+        #     def my_method
+        #     end
+        #   end
+        #
+        class NamespaceMethods < Base
+          MSG = "Don’t define methods outside a class or a module."
+
+          def on_def(node)
+            return if inside_namespace?(node)
+            add_offense(node, message: MSG)
+          end
+
+          private
+
+          def inside_namespace?(node)
+            node.each_ancestor.detect { _1.class_type? || _1.module_type? }
+          end
+        end
+      end
+    end
+  end
+end

lib/rubocop/cop/discourse/plugins/no_monkey_patching.rb

@@ -0,0 +1,92 @@
+# frozen_string_literal: true
+
+module RuboCop
+  module Cop
+    module Discourse
+      module Plugins
+        # Don’t monkey-patch classes directly in `plugin.rb`. Instead, define
+        # additional methods in a dedicated mixin (an ActiveSupport concern for
+        # example) and use `prepend` (this allows calling `super` from the mixin).
+        #
+        # If you’re just adding new methods to an existing serializer, then use
+        # `add_to_serializer` instead.
+        #
+        # @example generic monkey-patching
+        #   # bad
+        #   ::Topic.class_eval do
+        #     has_many :new_items
+        #
+        #     def new_method
+        #     end
+        #   end
+        #
+        #   # good
+        #   module MyPlugin::TopicExtension
+        #     extend ActiveSupport::Concern
+        #
+        #     prepended do
+        #       has_many :new_items
+        #     end
+        #
+        #     def new_method
+        #     end
+        #   end
+        #
+        #   reloadable_patch { ::Topic.prepend(MyPlugin::TopicExtension) }
+        #
+        # @example for serializers
+        #   # bad
+        #   UserSerializer.class_eval do
+        #     def new_method
+        #       do_processing
+        #     end
+        #   end
+        #
+        #   # good
+        #   add_to_serializer(:user, :new_method) { do_processing }
+        #
+        class NoMonkeyPatching < Base
+          MSG =
+            "Don’t reopen existing classes. Instead, create a mixin and use `prepend`."
+          MSG_CLASS_EVAL =
+            "Don’t call `class_eval`. Instead, create a mixin and use `prepend`."
+          MSG_CLASS_EVAL_SERIALIZERS =
+            "Don’t call `class_eval` on a serializer. If you’re adding new methods, use `add_to_serializer`. Otherwise, create a mixin and use `prepend`."
+          MSG_SERIALIZERS =
+            "Don’t reopen serializers. Instead, use `add_to_serializer`."
+          RESTRICT_ON_SEND = [:class_eval].freeze
+
+          def_node_matcher :existing_class?, <<~MATCHER
+            (class (const (cbase) _) ...)
+          MATCHER
+
+          def_node_matcher :serializer?, <<~MATCHER
+            ({class send} (const _ /Serializer$/) ...)
+          MATCHER
+
+          def on_send(node)
+            if serializer?(node)
+              return add_offense(node, message: MSG_CLASS_EVAL_SERIALIZERS)
+            end
+            add_offense(node, message: MSG_CLASS_EVAL)
+          end
+
+          def on_class(node)
+            return unless in_plugin_rb_file?
+            return unless existing_class?(node)
+            if serializer?(node)
+              return add_offense(node, message: MSG_SERIALIZERS)
+            end
+            add_offense(node, message: MSG)
+          end
+
+          private
+
+          def in_plugin_rb_file?
+            processed_source.path.split("/").last == "plugin.rb"
+          end
+        end
+      end
+    end
+  end
+end

lib/rubocop/cop/discourse/plugins/use_plugin_instance_on.rb

@@ -0,0 +1,42 @@
+# frozen_string_literal: true
+
+module RuboCop
+  module Cop
+    module Discourse
+      module Plugins
+        # Using `DiscourseEvent.on` leaves the handler enabled when the plugin is disabled.
+        #
+        # @example
+        #   # bad
+        #   DiscourseEvent.on(:event) { do_something }
+        #
+        #   # good
+        #   on(:event) { do_something }
+        #
+        class UsePluginInstanceOn < Base
+          MSG =
+            "Use `on` instead of `DiscourseEvent.on` as the latter will listen to events even if the plugin is disabled."
+          NOT_OUTSIDE_PLUGIN_RB =
+            "Don’t call `DiscourseEvent.on` outside `plugin.rb`."
+          RESTRICT_ON_SEND = [:on].freeze
+
+          def_node_matcher :discourse_event_on?, <<~MATCHER
+            (send (const nil? :DiscourseEvent) :on _)
+          MATCHER
+
+          def on_send(node)
+            return unless discourse_event_on?(node)
+            return add_offense(node, message: MSG) if in_plugin_rb_file?
+            add_offense(node, message: NOT_OUTSIDE_PLUGIN_RB)
+          end
+
+          private
+
+          def in_plugin_rb_file?
+            processed_source.path.split("/").last == "plugin.rb"
+          end
+        end
+      end
+    end
+  end
+end

lib/rubocop/cop/discourse/plugins/use_require_relative.rb

@@ -0,0 +1,32 @@
+# frozen_string_literal: true
+
+module RuboCop
+  module Cop
+    module Discourse
+      module Plugins
+        # Use `require_relative` to load dependencies.
+        #
+        # @example
+        #   # bad
+        #   load File.expand_path("../lib/my_file.rb", __FILE__)
+        #
+        #   # good
+        #   require_relative "lib/my_file"
+        #
+        class UseRequireRelative < Base
+          MSG = "Use `require_relative` instead of `load`."
+          RESTRICT_ON_SEND = [:load].freeze
+
+          def_node_matcher :load_called?, <<~MATCHER
+            (send nil? :load _)
+          MATCHER
+
+          def on_send(node)
+            return unless load_called?(node)
+            add_offense(node, message: MSG)
+          end
+        end
+      end
+    end
+  end
+end

lib/rubocop/cop/discourse_cops.rb

@@ -1,4 +1,4 @@
 # frozen_string_literal: true
 
-path = File.join(__dir__, "discourse", "*.rb")
+path = File.join(__dir__, "discourse", "**/*.rb")
 Dir[path].each { |file| require file }

rubocop-discourse.gemspec

@@ -2,7 +2,7 @@
 
 Gem::Specification.new do |s|
   s.name = "rubocop-discourse"
-  s.version = "3.6.1"
+  s.version = "3.7.0"
   s.summary = "Custom rubocop cops used by Discourse"
   s.authors = ["Discourse Team"]
   s.license = "MIT"
@@ -11,6 +11,7 @@ Gem::Specification.new do |s|
   s.files = `git ls-files`.split($/)
   s.require_paths = ["lib"]
 
+  s.add_runtime_dependency "activesupport", ">= 6.1"
   s.add_runtime_dependency "rubocop", ">= 1.59.0"
   s.add_runtime_dependency "rubocop-rspec", ">= 2.25.0"
   s.add_runtime_dependency "rubocop-factory_bot", ">= 2.0.0"

spec/fixtures/controllers/bad_controller.rb

@@ -0,0 +1,5 @@
+# frozen_string_literal: true
+
+class BadController < NoRequiresPluginController
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Discourse/Plugins/CallRequiresPlugin: Use `requires_plugin` in controllers to prevent routes from being accessible when plugin is disabled.
+end