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

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.

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?

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 :)

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.

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.

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.

I've been wondering for a while and and what I should add here and started several times and gave up again.

This issue is specifically about hook classes and not final in general. I linked to a general issue, where I'm also against proposals that we should make everything final by default.

To be honest, I think a lot of #8 is unnecessarily dramatic. We're talking about hooks, hooks until a few weeks ago were functions. You can not subclass/extend a function. There is no regression in extensibility if we make Hooks classes final. Hooks can be altered, you can very easily skip a hook from being called and implement your own. The only thing that final does is that you have to copy the current code instead of subclassing it. looking through some hook classes, the vast majority of them *are* small. Most exceptions are either hooks ore declaration hooks that have alters, those you can just alter again.

📌 Split File hooks into separate classes Active was now committed without final, but it *was* committed with private properties. That's pointless, because it actually means you can't subclass it either as you can't use those properties. If you want to fight something, fight private properties IMHO. I'm pretty sure DependencySerializationTrait still can't deal with that, so if you use them on forms, plugins or similar things you might hit some very, very weird serialization issues. AFAIK our policy is still that properties should be protected by default?

Yes, hooks are excluded from BC. So are constructors, but we still almost always do the BC dance with them, often even with change records and tests. Having a final keyword makes it very clear that we don't have to do any of that. What's excluded are hooks themself, the class the hook resides in is then already a grey zone again. With final we can add DI, we can regroup them, remove them and it's very, very clear that we don't have to think for a second about BC.

I honestly don't understand why there are concerns about this.

Thanks @berdir for taking the time to write down your thoughts. I very much agree, I do not have anything to add.

Let's widen this.

Maybe let's not?

Berdir makes an excellent point in #8:

hooks until a few weeks ago were functions. You can not subclass/extend a function.

If we widen this issue to a policy about final in general, regardless of context, we lose that important piece of information. Reverting the IS to focus on the original discussion. If you want to discuss final in general maybe a meta issue is needed where we can have child issues for every type of system we're discussing.

P.S.: I'm not a fan of marking everything as final but some things really ought to be. Hooks are a great example of that. Keep in mind final does not block decorating, but it will block people from extending your internal class and then complaining when you broke their extension.

A great, very recent, example of that is #2719721-71: BreadcrumbBuilder::applies() mismatch with cacheability metadata → where it was unclear to people whether or not tagged services are deemed internal. Had we marked the breadcrumb builders as final, that would have been obvious from the get go. Even for maintainers it's sometimes hard to know what is and isn't internal without checking the docs, being able to mark internal things as final will drastically reduce the room for doubt.

Sure , make Drupal unextensible, who cares, already Symfony does that and we knwo Symfony is the best codebase there can be, am I right

For anyone not wanting final on hooks classes, it would probably help to provide an example of how one might want to extend a hooks class.

Might be worth also considering how this would look if ✨ Support hooks (Hook attribute) in components Active got in.

it would probably help to provide an example of how one might want to extend a hooks class.

this has been answered in #8

In this specific case I imagine classes would either or both a) extend the hook class to reach the helper methods of the class b) perhaps tweak or completely override a hook method in both cases they would mark themselves as a decorator at the same time.

.
yes b) is being addressed in 📌 Hook ordering across OOP, procedural and with extra types Active

Also, on a much wider scope: why does this matter? You can't predict the future, you know final closes the door and gives you nothing in return so why do it?

Looking back at amount of wasted time of community on issues like "make this place extensible" and sometimes unexpected future of new code I strongly against using final

Moreover making open source it looks a nonsense to spend time on closing/protecting it

From documentation POV it's more handy (both for human and AI) to have short comment with @internal and reason why this point is not supposed to be extensible

Guzzle already has the solution.

/**
 * @final
 */
class Client implements ClientInterface, \Psr\Http\Client\ClientInterface

For more modern code we could even create an attribute. If we are really enterprising we could create an attribute through Psr and then world peace is just one small step away after that.

Oh I like that! It let's you define, gives the dev a clear notice but they can do what they choose at that point!

posted in #core-development. @dww was a +1

I'm a +1 because I never know when to use final myself

Going to mark it, guess if it gets approved a follow up to convert current finals can be opened.

Might as well comment here for posterity, since Slack is ephemeral.

Yes, I hate final. I know we’re all excited about limiting our API surface, and I know the lengths we go jumping through hoops to provide BC in some questionable circumstances. From the standpoint of maintainers, the appeal of final is really juicy. But using a language “feature” to completely prevent extending a class is a real burden in various scenarios. Both because of “interesting” requirements when building real things, and when trying to write tests

I love the @final annotation as a solution. It’s totally clear, unambiguous, it invites documentation, but it still lets you extend if you need to. You know you’re on your own, but you can do it.

A few questions come to mind

• The BC policy will need a change, it currently states that "All classes, methods, functions, etc. in the Drupal 8 and above API are designated as either public or internal".
• Is there other documentation that would require a change?
• Has anyone checked to make sure the @final will be handled correctly by api.drupal.org? Core has started to use some features which are effectively unreadable on api.d.o
• There is a defined order for the tags. where will this one go?
• There is no section for @final in the coding standards tag reference → . It would be sensible to add something

We would need a new section in the bc policy probably.

@final implies you shouldn't subclass something, but it doesn't imply that you shouldn't call methods on the class. So a value object you're expected to interact with could be @final, but also a controller that you're not expected to interact with at all could be both @final and @internal.

I think it's likely that @final will 'just work' on api.drupal.org because it's a normal phpdoc annotation rather than an attribute.

I would put @final to the end I know the documentation currently calls for annotations to go there but since annotations are being removed, this is not a biggie. Doctrine can pick them up even if there is something after.

Dig some more searching and there are similar issues to add @internal, to some of the same classes as stated in the issue summary. That needs some clarification.

#2873395: [meta] Add @internal to core classes, methods, properties →
#2873716: Add @internal to core classes, methods, properties to Plugins →
🌱 [meta] Document @internal APIs both explicitly in phpdoc and implicitly in d.o documentation Active

📌 [PP-1] Make doctrine/lexer:^3.0 compatible with \Drupal\Component\Annotation\Doctrine. Active is a great example of why using final is bad.

My 0.02.

I am +1 on using the @final annotation more widely - it has no runtime impact while at the same time static analysis catches it and we have ways to manage it without hard failures.

I am -1 on forbidding final keyword as a general rule. While I agree it's frequently misused (see this https://github.com/paratestphp/paratest/issues/939 for example - it's making paratest practically a no-go for Drupal), still using it is a matter of deliberate choice that should be agreed upon before commiting to the codebase. And IMHO there are cases where a class is just a front runner for other code that can be varied in a composition model (for example, Drupal's Database class).

Could you please elaborate on what advantage final offers that @final does not?

Not in general terms. What I am saying is that the use of final should be pondered on a case by case basis. And because of that, that we should not rule it out in principle.

For example, for me Drupal\Core\Database\Identifier\Table in 📌 [WIP] Decouple identifier management from database Connection, introduce an IdentifierHandler class Postponed is OK to be final because it minimally extends a Base class, it should be used only to handle a database table, and its values are managed by a factory that feeds them to the object based on the database backend (varies). You want to do something like a Table? Extend a new class from the base one.

OTOH, use of final in https://github.com/paratestphp/paratest/issues/939 makes no sense to me because how it's been used prevents any overriding of anything... it preaches composition but it does not have the logic behind. So you have only a wall there. Same applies to a lot of recent developments in the PHPUnit project.

For sure this is a case where less is more; but less does not need to be zero...

The problem is we don’t want to keep debating this case by case. The proposal is that @final gives us everything we actually want to use final for, with none of the downsides. So we should never use final and always use @final. Then it’s totally clear, we don’t waste time continuing to argue about this in individual issues, and everyone gets what they want. 🙏

Yes. The core tenet of the proposal is this: there's no known advantage of final over @final while there are certainly downsides so let's always use @final.

While I am still a believer that the final keyword has its place, there is no point in being stubborn for the sake of being stubborn if a perfectly suitable alternative like @final presents itself. So consider me on board with the latest proposal.

I reckon the next step here is to clarify the documentation changes. Since there is no proposed text or what documents need to change I added a start to the proposed resolution section.

What I see now is that the current API definition defines 'Public' and 'Internal'. The text for what 'final' means needs to be added.

Also, the proposed resolution includes several APIs that are currently considered 'Internal APIs'. What changes are need to identify what is 'internal' and what is 'final' or what is both 'internal' and 'final'.

Setting this to needs work for the above.

I presume something like this would go under the guidelines section: "Unless a class is explicitly intended to be extended mark it with @final to signal it is not supposed to be extended. Any bug reports regarding the extension of a class marked by @final will be closed. Nonetheless, core does not use the PHP final keyword so developers can still extend such classes at their own risk. It's a "break glass in emergency" solution."

I could live with #37.

Seems to check all the boxes:

• Document as "Use at your own risk"
• We won't stop you from using it at your own risk
• You will get zero support when ignoring @final