[policy, no patch] Hook classes should not be marked final

Created on 19 November 2024, about 1 month ago

Problem/Motivation

There has been some discussion on whether Hook classes should be marked final.

I strongly think they should not be, at least until we have an explicit way to replace a modules implementation.

Without final it is easy to override or modify a module's implementation.
This is not strictly supported, but it is a convenience in many cases.

If we mark them final you have to jump through many more hoops to modify the class.

final does not provide any benefit other than enforcing the BC policy.
I think the policy gives enough coverage and not having final let's developers make their own decisions on whether extending something outside of BC is worth the risk / effort.

Steps to reproduce

N/A

Proposed resolution

Have a policy that does not mark hook classes as final.

Remaining tasks

Discuss

User interface changes

N/A

Introduced terminology

N/A

API changes

N/A

Data model changes

N/A

Release notes snippet

N/A

πŸ“Œ Task
Status

Active

Version

11.0 πŸ”₯

Component

documentation

Created by

πŸ‡ΊπŸ‡ΈUnited States nicxvan

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

  • Issue created by @nicxvan
  • πŸ‡ΊπŸ‡ΈUnited States nicxvan
  • πŸ‡ΊπŸ‡ΈUnited States nicxvan
  • πŸ‡¨πŸ‡­Switzerland berdir Switzerland

    I've argued many times against against making too many things final/internal/not an API , but IMHO, it really makes sense in this case.

    hook implementations are explicitly excluded from our BC policy: https://www.drupal.org/about/core/policies/core-change-policies/bc-polic... β†’ . So are constructors, but we still in updates to be as nice as possible to contrib there.

    By making them final, it is very clear that we do not have to worry about BC for constructors, we can change them, re-group them (although I suspect that will break whatever alter/replace logic that we have).

    Do we even know what's going to happen if you extend such a class? Isn't it still going to find the original implementation in the original module? What about other hooks that you don't want to extend? What if two different modules want to customize two hooks on the same class?

    This really, really seems like a can of worms we should not open.

  • πŸ‡ΊπŸ‡ΈUnited States nicxvan

    If you extend it and set it as a decorator the original event listener tags move over as well.

    Also, yes if multiple interact it would probably get messy quick, but in order to prevent that what do we lose?

  • πŸ‡·πŸ‡΄Romania amateescu

    I don't think we lose anything if we make them final. At the moment a hook implementation can be swapped via hook_module_implements_alter(), and when that will be removed its replacement will need to provide the same ability somehow.

    So +1 for final :)

  • πŸ‡¨πŸ‡­Switzerland berdir Switzerland

    IMHO decorators is an antipattern when not explicitly supported as a concept by an API/interface. It requires a base class that by default calls the inner class or it doesn't work and you can't actually have multiple decorators, so you can just as well just replace the class you want to customize. Decorating a class breaks if you add a new method and have no base class to cover that addition. Or put in other words, decorating should _not_ require to extend the default implementation, otherwise you are not actually decorating it.

    If hook implementations are complex enough that it's worth reusing/extending it instead of copy & paste then it that code should probably live in a service and be an actual API on its own. Many things work like that already,

    I've literally been first in line to argue against 🌱 Use final to define classes that are NOT extension points Active because I think plugins and many other things are useful to extend, but hooks are a very different scenario.

  • πŸ‡¨πŸ‡¦Canada Charlie ChX Negyesi 🍁Canada

    I was initially against the move to PHP5 because I felt private/protected limits extensibility needlessly and I felt the underscore in classic var $_foo expressed our intention this should not be used is enough. It only took 18 years for the PHP team to agree with me and drop setAccessible from reflection in PHP8, essentially turning private/protected properties into advisory.

    Similarly, final should never be used. Instead, there should be some advisory saying "warning, this is not part of BC and if you extend it then you are on your own". Guess what? That advisory already exists at https://www.drupal.org/about/core/policies/core-change-policies/bc-polic... β†’ , six years ago larowlan noted hook implementations are not part of BC, it is well established policy. So final achieves absolutely nothing except limits extension. It's a language misfeature and it should never be used. It's really that simple.

    In this specific case I imagine classes would extend the hook class to reach the helper methods of the class and mark themselves as a decorator at the same time. Concerns raised:

    1. Isn't it still going to find the original implementation in the original module? -- no it won't because it won't be an event listener, the decorator moves service tags (this got documented in Symfony last month).
    2. What if two different modules want to customize two hooks on the same class? -- be my guest, decorators work for that.
    3. What if the base class adds a method which the decorator doesn't call? -- these classes are not part of BC, that's a you problem. Really, who cares? Crippling runtime just because some distant future might -- or, you know, might not -- break something is just a bad idea. That's my entire point.

    Drupal used to be extensibility first. It's upstream that is downright hostile against extensibility. Don't be like upstream.

  • πŸ‡ΊπŸ‡ΈUnited States nicxvan

    I think the helpers are a good example of something that a lot of hooks will likely start implementing and extending will give you access to them.

Production build 0.71.5 2024