The different approaches to modifying server classes and modules

Development
1

@merefield @fzngagan @muhlisbc Ok, so there are a variety of different ways to modify Discourse server modules and classes.

I’ve been considering all of this recently for the Multilingual plugin and now thinking about them when looking at @muhlisbc’s additions to the Question Answer plugin: https://github.com/paviliondev/discourse-question-answer/pull/30/files.

Interested in any of your thoughts on the below.

The goals

The goals with such modifications are to:

  1. Allow the modification to be disabled via the enabled / disabled setting.

  2. Default to the core functionlity as far as possible.

  3. Make them as clear and simple as possible (i.e. plugin structure)

The approaches

There are only really two approaches that come close to fulfilling these goals, the server plugin api and module prepending.

Server plugin api

The methods in https://github.com/discourse/discourse/blob/master/lib/plugin/instance.rb are the Discourse server plugin api.

The upsides of this approach are that:

  1. it’s supported by the Discourse team; and

  2. it’s subject to the enabled setting

The downsides of this approach are:

  1. it doesn’t allow you to refer to the super method (i.e. the method you’re overriding).

  2. In larger plugins it leads to a long plugin.rb file that’s hard to follow.

Module prepending

Module prepending is described in some detail here: https://meta.discourse.org/t/tips-for-overriding-existing-discourse-methods-in-plugins/83389.

The upsides of this approach are:

  1. It allows for modifications to be broken into distinct modules and put in seperate files for a clearer structure

  2. It allows reference to the super method, to preserve core functionality as needed.

The downsides of this approach are:

  1. It’s not supported by the Discourse team

  2. It’s not automatically subject to the plugin enabled setting

This second downside can be overcome however by putting the module prepension in the plugin.rb file, and then making it subject to the plugin enabled setting.

Module prepension, subject to the plugin-enabled setting is currently my preferred approach.

Approaches to avoid

I would note that these other approaches should be avoided, as they either are unreliable ways to modify classes (i.e. don’t work in all situations in which the code might be called), are messy structurally, and / or don’t allow for use of the core code.

  1. Class or module eval

    Post.class_eval do
   def method
   end
end

  2. Aliasing methods, e.g.

     class Post
   alias_method :method_old, :method
 end

  3. Simply overdding a method by making a new declaration

    class Post
   def method
   end
end
2

My 2 cents on this

If adding a new method, best to go with the plugin api methods.

If modifying a method, module prepension subject to plugin enabled should be preferred.

Lastly, breaking up different modifications into multiple files and keeping plugin.rb as short as possible.

3

Yes, I think i’ve just found a case where converting add_to_serializer statement to a .class_eval in a separate file is failing for me.

4

Yeah I would avoid using class_eval. It feels convenient, but it can come back to bite you.

5

@angus

There’s a caveat here. The plugin.rb file is loaded only once, so the module prepension done based on the initial state of the plugin enabled setting. Disabling the plugin would require a server restart apparently?

Alternatively, we could check for plugin enabled condition in the overridden methods themselves which ensures that functionality is enabled subject to the setting even if the setting is toggled.

Thoughts?

6

Yeah, you’re right. I actually put the enabled site setting inside the extension module normally.