Overriding include_* methods in serializers

Hey @david,

Just a quick question on the thinking behind this deprecation

Deprecation notice: add_to_serializer should not be used to directly override include_*? methods

Context: DEV: Improve add_to_serializer include_* options (#21220) · discourse/discourse@26b7f8a · GitHub

I understand the desire to move folks to the include_condition argument in a standard use of the add_to_serializer method, i.e. to add their own methods to serializers.

However there are some instances in which a plugin may want to add an include_* method to a serializer that don’t match that case, i.e. when you’re not determining whether your own custom attribute is included, but you’re overriding an include_* method in a core serializer, e.g.

Core method: discourse/app/serializers/site_serializer.rb at main · discourse/discourse · GitHub

I appreciate that that particular use could be re-thought to not require an override of the site serializer method, or the override could be achieved in other ways, but I’m wondering whether there’s an inherent downside in allowing such use of the add_to_serializer method in this way, and whether the deprecation will lead to a removal of the use of the method in this way.

4 Likes

Yeah, that would be my recommendation.

We’ve recently introduced a new system for ‘plugin modifiers’, which are super cheap extension points which are similar to DiscourseEvent but they take an input value and return a value. So in your case, you could make a core PR to add a DiscoursePluginRegistry.apply_modifier call in the relevant include_ method, and then you can use register_modifier in your plugin to override the value.

It’s likely we’ll eventually block it completely, yes. Plus you really don’t want to be using a method that’s printing deprecation noise in the logs.

If you really must override a method without co-operation from core, then modify_class would seem to be the better choice. The main reason we have a dedicated add_to_serializer is because it auto-defines an include_* method so that it only applies when the plugin is enabled.

That means that the code snippet you linked is currently defining two methods. include_wizard_required? and include_include_wizard_required? :sweat_smile:

11 Likes

That Readme says that it “operates as a stack (first in, first out)”, but that’s a queue. A stack is first in, last out. (I can’t figure out how even to copy the text on my phone).

4 Likes

good catch. yea stack is LIFO and queue is FIFO

Key features of these modifiers:

  • Operate in a stack (first registered, first called)
  • Automatically disabled when the plugin is disabled
  • Pass the cumulative result of all block invocations to the caller
1 Like

It’s more like a ‘middleware stack’: a series of methods which are executed in order, each of which pass their result to the input of the next method.

I don’t think trying to apply LIFO/FIFO terminology here will work: nothing is being added/removed from the ‘stack’ - there is no ‘out’.

4 Likes

Oh. So not a stack data structure.

I started to say something about how I got my cs degree in 1987 and didn’t know what people learned now. :joy:

2 Likes

this is sort of my issue. i have a recent almost 10 year gap where i barely went near a computer (after 25+ years working with them) and it feels like a missing huge knowledge base section.

3 Likes

This topic was automatically closed 30 days after the last reply. New replies are no longer allowed.