[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

For what it's worth, I'm the committer who most often pushes back against marking things final, but the reasons I've done so are:

1. We haven't had good core best practices about its usage.
2. People often think that they can use final to get around providing BC for something, but in practice they're often overlooking some aspect of the API and it's not accomplishing the goal they want.
3. Misuse of overuse of final makes it more likely that people just duplicate code instead, which leads to lower code quality, higher regression rates, longer upgrade times, etc.
4. As a rule, core-focused contributors tend to underestimate the creativity of what contrib developers can accomplish by extending core code in interesting ways.

All that said, calling final a "misfeature" is unhelpful. We might as well say that everything should just be marked public. Limiting API surface in sensible, architecturally sound is a good thing: it explicitly tells the developer "this is not intended for extension". And a nice, clean, fast, language-level fatal is vastly preferable to something based on some phpdoc-based solution. Moving things from the language level back into phpdoc is going backwards.

This issue has also vastly, vastly exploded from its original scope about hook implementations into somehow being a general policy? No. Release manager saying definitely -1 here. There are very valid reasons to use final and we should do more of it. What we need is clearer standards of when to use it, and better shared awareness of the gotchas and tradeoffs.

For what it's worth, I think even hook implentations should have been final, although I understand thanks to @larowlan that the motivation here is actually probably to enable the new hook_module_implements_alter() equivalent stuff or suchlike.

By the way, I'm not opposed to discussing the use of @final. What I'm concerned with is setting some sort of policy forbidding the actual language feature, which is useful and should be evaluated in context.

Doorstops.

We might as well say that everything should just be marked public

The fundamental difference is how protected properties can have logic hardwired to them. Once Drupal core requires PHP 8.4 and property hooks become available this is a proposal I would wholeheartedly support, however.

Regarding final

the actual language feature, which is useful

Useful for what? For example: it explicitly tells the developer "this is not intended for extension". -- yes, @final does exactly that.

Useful for what? For example: it explicitly tells the developer "this is not intended for extension". -- yes, @final does exactly that.

Others have repeatedly answered this question for you on the issue, so please stop repeating it. Thanks.

Okay, I had unfollowed this momentarily because the discussion is going down some very unhealthy patterns, but I think we can make better progress by at least splitting the scope:

1. One policy issue to explore the use of @final and a policy for it, using other projects as a model
2. One issue to discuss whether to apply the above to hook implementations instead of ever using final (enforce for core, recommend for contrib)
3. One issue to argue against using final at all. (For me and as far as I can see every commenter other than chx, that's a wontfix, but let's at least split that discussion off so as we don't derail this one. At a minimum, we can think of the @final issue as a blocker.)

I'll again unfollow the issue, but hopefully that can help keep the discussion on track. @ghost of drupal past, please avoid rhetoric and pay attention to the feedback you're getting from numerous other contributors here.

I tried. xjm returned and killed the issue because she hates me, sorry it's a fact, it's been six years and even that's not enough.

@chx: my brother, you’re not helping these issues with your drama. There are some possibly effective and productive arguments against what xjm has written, but “she killed it because she hates me” is not one of them. You’re poisoning the well for everyone who agrees with you and wants to see this happen. Now, in addition to whatever technical arguments we might make, we’ll have to overcome the (righteous) disdain for your tantrums.

If you get discouraged, disappointed, burned out, frustrated, etc, please just ignore and “walk away”. But your angry and unhinged public comments make it so much harder to get anything done.

my brother, you’re not helping these issues with your drama.

And derailing the issue is? We were near agremeent.

Who cares about that. Drupal has an AI initiative before it has a documentation team lead. AI will certainly solve all the problems.

I completely agree this is the wrong priorities (and not the only wrong priorities, countless examples in the past decade and half) but core committers have no control over either the 'AI initiative' or whether there's a documentation team lead or not, those are things controlled by corporate funders and the DA budget.

For me it makes sense to apply @final to all the places we think should use it (for me this is seems to be pretty obviously hooks, controllers, plugin implementations that we don't want to be responsible for changes to but very few of us can hand on heart say they haven't subclassed one in custom code). Also once that decision is taken, it's easy to add @final to new code instead of final unless there's a good reason.

Banning final is a completely separate discussion for me - e.g. for a start to actually do that we'd need to remove the existing (very infrequent) final usages in core.

Updated the title.

I'm happy to move the discussion around removing / banning final to a different issue and keep this one as just allowing using @final and defining where to use it.

it explicitly tells the developer "this is not intended for extension". And a nice, clean, fast, language-level fatal is vastly preferable to something based on some phpdoc-based solution. Moving things from the language level back into phpdoc is going backwards.

I believe this was addressed by the bit where @final does all of this beyond the fatal.

As @ghost mentioned we bumped up against this both in symfony containers and in the hook conversion with rector a fair bit. You get clear warnings in IDEs with @final that you should not be extending it, if you're not using IDE's you already need tooling and it can be added.

I'll create an issue for the final discussion to separate that out.

Also from a policy standpoint:

Limiting API surface in sensible, architecturally sound ways is a good thing

I think @final achieves the same thing as final while still giving the open source community the flexibility to decide what to do for themselves.

I'll review the issue summary in a bit.

The one I created might be a duplicate.

I took a pass at narrowing the issue summary and the proposed text here.

Added a touch of reality to the IS, 📌 [WIP] Decouple identifier management from database Connection, introduce an IdentifierHandler class Postponed is still just a proposal for now.

I don’t understand the point of trying to craft a policy or heuristic for when to use @final vs final. As I understand it, the desire is to clearly mark classes as “do not extend”. Either a class is intended to be extended or it’s not.

Some (many?) of us hate final as the way to communicate that, since it locks people out, goes against the spirit of open source, and makes it impossible to do “interesting” things when necessary, including in various scenarios when you want to mock things for testing.

The proposal was to use @final to communicate “do not extend” in all cases, since it still clearly identifies things that are not meant to be extended, invites documentation about why not or what to do instead, but it doesn’t actually prevent it if people want or need to “break glass in case of emergency”…

That’s why the scope expanded in this issue to encompass a complete solution. If we break up the scope into the 3 issues proposed in #44, it doesn’t make sense to me. I don’t see how we can “invent a policy for when @final is okay”, separately from “mark things that aren’t expected to be extended”, which is identical to our current “policy” for using final.

This was envisioned as a policy issue, not an implementation. If we decide that @final is a better tool for the job, we’d open follow up issues to start using it, convert existing final usages, etc.

Would someone who wants to keep using a mix of both final and @final to give some indication of how we could possibly tell the difference from a policy perspective? To me, we need a tool to mark classes as “do not extend”. We’ve currently been using final, and there’s a movement to stop doing that and use @final instead. But I can’t yet fathom that we’d have 2 sets of “do not extend” and why we would use different tools to communicate that for the 2 “different” sets. Please explain. Thanks!

To be clear I 100% agree with @dww in 54.

I created the other issue just so we could maybe more quickly start using @final, but maybe the discussing should remain here.

Yeah, or we start over with a clean issue, free of confusion, scope changes, and tantrums, so there’s a clean comment history to read.

which is identical to our current “policy” for using final.

Policy is in scare quotes here and that's correct though. The only places we use it currently are in low level internal code, usually experimental or experimental-ish APIs to prevent having to provide a BC layer. We're not using it for hooks, controllers, event listeners, and all the other places that are considered @internal but only if you read the BC policy. Nor were we likely to. So I don't see where a complex heuristic comes into this for the vast majority of places we'd use it.

Indeed, I meant no offense. I only mean that we don’t have a formal “policy” around final, so I didn’t want to really call it that. 😅

But that’s not the main point of #54, nor what I wanted to solicit a response about. Here’s the TL;DR:

As I understand it, the desire is to clearly mark classes as “do not extend”. Either a class is intended to be extended or it’s not.

and

To me, we need a tool to mark classes as “do not extend”. We’ve currently been using final, and there’s a movement to stop doing that and use @final instead. But I can’t yet fathom that we’d have 2 sets of “do not extend” and why we would use different tools to communicate that for the 2 “different” sets.

Right but my argument is that we're not actually using final in many places at all.

It's mainly used in package_manager and navigation - both still experimental modules.

Config actions/recipes API - was added as an 'experimental subsystem', we probably need to revisit whether that's still the case now it's been in for a while.

All of those usages need to be revisited as those things get marked stable.

Then there are some other final scattered about but not many. Some of those are places like SimpleAnnotationReader which are forked parts of dependencies that we want to remove later anyway - I would personally leave that one as final because... why...

So it seems very uncontroversial to me to add final to a load of new places that aren't using it. Some specific issues could be opened to replace final with @final, but in some cases to remove it if an API has become stable. Actually banning it means switching every current usage over and adding a phpcs rule or similar which is a lot of additional work (and discussion) to do.

What if the policy is this:

When a class is not intended to be an extension point use @final.

If the class is in an experimental module or experimental subsystem then final may be used instead.

@final is still preferred over final.

Re: #59:

So it seems very uncontroversial to me to add final to a load of new places that aren't using it

Typo? Did you mean "to add @final to a load of new places..."?

Re: the idea that final is okay in experimental modules... not saying this really happens much, but isn't the time when new core code is still experimental a great opportunity to try to extend and improve it in contrib? How much experimentation is possible if things are locked down as final? If I want to make an even better package_manager in contrib, my hands are pretty tied by tedbow and phenaproxima's prolific use of final.

Here's an example of an MR to OO-ify one of the most complex functions in all of core: 📌 Convert update_calculate_project_update_status() into a class Needs work . That MR, as currently written, is full of final and private. I haven't yet picked that apart, but I'm strongly opposed. If I wanted to extend core's Update Status in contrib, an MR like that would mean I basically have to copy/paste most of the relevant code. No thanks.

I'm okay with having to debate whether something is meant to be an extension point on a case by case basis. I just don't want to have to debate the mechanism to declare it's not meant to be extended. I want to save all the effort, from both committers, subsys maintainers, and contributors. We waste^H^H^H^H^H spend enough time arguing about things as it is. 😅 I'd love it if we could agree with what I wrote in #33:

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. 🙏

Typo? Did you mean "to add @final to a load of new places..."?

.... yes.

If I want to make an even better package_manager in contrib, my hands are pretty tied by tedbow and phenaproxima's prolific use of final.

There's already a package_manager in contrib, it's in the automatic_updates module, except the 4.0.x branch which removed that sub-module because anincompatible version is now in core and nothing can work with both the contrib and core versions from 11.2 onwards.

package_manager in core 11.2.x completely broke the 3.0.x branch of automatic_updates after 📌 Clarify/replace 'stage' terminology in Package Manager Active and also the 2.0.x branch of project_browser for the same reason. They both had to come up with new branches that require 11.2.x and add conflicts in their older branches. Hopefully that will mean a smooth upgrade path but we won't see if it really, genuinely works until 11.2.0 is out and sites with those modules installed try to update.

Beyond that package_manager has had a small number of stable blockers ( 🌱 Drupal 10 Core Roadmap for Automatic Updates Active ) for about a year now (the above issue was one of them, not loads of others) so theoretically could have been table in 11.1 if there was more focused attention on clearing all of those out.

Given all that, I cannot fathom how it would be a good use of anyone's time to make a third version of package_manager in contrib. If anyone tried to use that module it could also spill over into the core, automatic updates and project browser issue queues when everything breaks and they don't know why. The whole point of the module being experimental alpha with @internal and @final everywhere is to say to people 'please don't try to integrate with this'. It is a real shame that it did not get closer to stability before project browser and automatic updates were included with Drupal CMS, because there is a non-zero chance of Drupal CMS sites (the majority of installs) blowing up when they do their very first minor update due to this. I have spent quite a lot of time over the past 18 months trying to unblock package_manager in core partly due to this reason.

You can say 'this was just a random theoretical example' but it's not because it's one of only three places in core that are using it widely, for very specific reasons to limit the API surface as much as possible when we knew we were going to introduce breaking changes.

Similarly, navigation in 11.2.x has broken gin toolbar and there needs to be a new branch of Gin that requires that while conflicts are added to older versions, this is due to the new top bar being enabled by default which requires fixing ✨ Integrate Gin with the new Navitaion Top Bar Active and removing gin toolbar. Does some of its code being final help with that? No it doesn't but it may have prevented other breakage, or if it didn't, no-one noticed the code was final anyway.

Re: #63: All fair points. Another package_manager contrib was indeed a hypothetical example. 😅 Didn't mean to pour salt on an open wound. 😬 That said, I fail to see how using @final in those cases would make things any worse than they already are. My main point with #63 is a general one that "experimental" code doesn't seem more appropriate for final than stable code, so I don't love the proposal in #60 (which I assume is a genuine attempt to find compromise and to move this proposal forward, not a strongly-held desire to use final at all).

I'm less concerned about un-finalizing the existing usages, other than for consistency. I'm mostly advocating for using @final not final for everything new, and having a clear policy that doesn't involve arguing about 2 competing mechanisms to accomplish the same thing. I care less about phasing out final in the small handful of existing cases (although I don't think it would be difficult if/when we decided to do it).

p.s. I feel your pain about the priorities of the corporate funders. That explains both the AI Initiative and the lack of support for package_manager / update_manager, etc. It's too bad the people who know the system the best don't have more control over where the development $$ (and therefore, effort) flows, and that those decisions are left to the business people who want to chase the shiny and new (and speculatively profitable), not necessarily the important. Welcome to late stage capitalism, where the golden rule is that those with the gold get to rule. I know I'm not saying anything you don't already know (and mostly/entirely) agree with, in this paragraph. 😅 ✊

Solidarity,
-Derek

p.p.s. As a parallel example, we have 🌱 [policy, no patch] Use declare(strict_types=1) in new code Active . We already add strict_types to all new code, yet the vast majority of core files do not yet declare strict types. I don't think it's the end of our world if we have a policy (as this one was previously worded) to "use @final, not final", even if there are still a handful of legacy final usages sprinkled around core.

#60 (which I assume is a genuine attempt to find compromise and to move this proposal forward, not a strongly-held desire to use final at all).

You'd be correct, I'd rather a world where we don't mark things final at all and only use @final.

@final provides the noticed without the lockdown that is sometimes needed.

One of the consequences of not having a policy is that people that see code in experimental modules see things marked with final and will likely replicate that too.

One thing that's starting to bother me about the terminology of "break glass when in need" is that we're not actually making people brake any glass :)

Unlike the final keyword where you really can't extend it or get errors, the @final annotation only warns you in your IDE and some inspections but does not stop you from extending at all. So it's more like a "Do not trespass" sign on a bank vault that's wide open. It doesn't require much effort to break into said vault.

This leads me to reconsider the benefit of @final. One could argue that "breaking glass" would be writing a small patch that removes the final keyword from whatever core or contrib class you wish to extend. At that point it's both a deliberate action and plainly obvious from the patches folder that someone did, in fact, break glass.

I'm afraid that allowing people to extend @final classes at their own risk will cause confusion among people brought in to debug why their project is acting so weird. It won't be that obvious from the code base that a final class was extended.

Either way, just my 2c. I still stand by #35: If the majority is in favor of @final, I won't go lying on the train tracks so to speak. But now that I've given it some more thought I'm starting to lean towards the keyword again. We're simply not allowing people to "break glass" when in need, IMO we're just removing the glass altogether.

One could argue that "breaking glass" would be writing a small patch that removes the final keyword from whatever core or contrib class you wish to extend. At that point it's both a deliberate action and plainly obvious from the patches folder that someone did, in fact, break glass.

This is my preferred way, as I definitely know then that "from here on, I'm on my own" and this way is my "spirit of open source", because
a) I can read the source code and
b) I am legally allowed to change the code

I've caught myself a couple of times, "accidentally" using services and classes that are marked as "@internal" (Drupal\Core\Render\Markup, anyone?) so I can already see myself "accidentally" extending "@final" classes.

When overseeing this sort of declaration, getting blocked of a problem in there, looking for a solution, proposing the solution by creating an d.o. issue and then it being immediately closed by a maintainer as "won't fix", because "well you should've taken care of looking for the @final annotation!" I'd not be angry but certainly will think twice before my next attempts of contribution.

The thing is final isn't collaborative.

As mentioned ghost was trying to set up entities to allow dependency injection. Symfony users final in so many places that we can't. Are you suggesting we should just patch symgony to take final and private off the classes we would need?

I mean Drupal likely would release a feature that depended on code marked @final upstream but setting up a proof of concept isn't really viable in that situation, with that poc is easier to open an upstream issue showing why removing @final is useful.

We had the same issue with the hook conversion for core. I even opened an issue in rector to ask them to remove final so we could extend better printer to change the array printing and it was closed within a day worth the above to copy the giant file ourselves.

We opted to use sed to remove final, but I strongly feel it goes against the open source ethos and@final gives us a strong flag that you're on your own.

proposing the solution by creating an d.o. issue and then see it immediately closed by a maintainer as "won't fix", because "well you should've taken care of looking for the @final annotation!" I'd not be angry but certainly will think twice before my next attempts of contribution.

How does final help this situation? You can still miss the final unless you mean the suggestion includes code, but then you just can't even try final prevents all extension.

I mean Drupal likely would not release a feature that depended on code marked @final upstream

No we've done that multiple times, although often the @final or @internal has been retrospective after we started using it, one I can think of is #3247384: Start using the class Symfony\Component\Validator\ValidatorBuilder instead of copied classes → but I'm sure there are plenty more.

I mean this is also a reason we cannot add final to most of core except perhaps in a major release because it would break existing overrides.

No offense, but I feel this issue involves too much subjectiveness which makes it too hard to ever come to achieve any objective agreement. +1 for #44.

We've already done 44 this is issue number 1 in that list technically.

We've created a discussion history here that isn't collaborative. The original scope of this issue was thrown away and has gotten into a mess of subjective arguments and, even worse, includes more negative personal tone than objectiveness. It's gone into a debate now rather than a discussion.

This is a public issue and not a private thread. We should try our best to communicate inclusiveness, inviting others sharing their thoughts and experience. Looking at the comment history here I can imagine some reading this will be intimidated and stay silent - not good.

As a starting point, the proper way would be to

• Restore the original scope of this issue (or even better, close this one and start a new one with the original scope). Keep the room to find possible ways for this particular scope, fix it or close it.
• Creating follow-ups when needed as already described in #44.

How does final help this situation? You can still miss the final unless you mean the suggestion includes code, but then you just can't even try! final prevents all extension. Are you talking about in contrib? Again, if marked final you can't even work around it. If the contrib maintainer would close an issue because it extended @final code they surely wouldn't accept code that would require patching out final. If I'm misunderstanding something please clarify.

If you miss @final, nothing will stop you from extending the class. If you miss final, PHP will stop you.

But my point wasn't necessarily about people making mistakes, it was about showing intent and requiring intent when people do want to break glass.

There's a few cases where we might want to use some concept of final. This issue, right now, isn't about when that is but what we can do when it is the case. Although the scope has changed a lot already. So with that scope in mind, there's only a couple of situations where we might encounter final:

1. In contrib
2. In core
3. In one of our dependencies

For both 1. and .2 my comment from #67 still stands: If you have a project where you want to extend a final class and fully well know that it's an edge case or project-specific requirement, by all means write a local patch that merely removes the final keyword and be done with it. Intent is clear, and you have made a manual, small effort to break glass and know you're on your own from here on out.

For 3. I would expect core/contrib not do anything funky there other than perhaps decorate the class, but according to #70 that expectation might be wrong?

Now, for the elephant in the room: @ghost of drupal past's effort to make entities use DI. Here you obviously wouldn't write a local patch, but instead go to the issue queue and argue your case that you are indeed blocked by the final keyword and think you could make a great contribution if it were removed.

At worst, it starts a discussion on why that piece was marked final, you get more info and nothing happens. At best, the final keyword is removed or the final class is refactored to allow for outside influence without extending said class.

Either way, I don't think there are that many efforts where a big change is blocked by the final keyword compared to the amount of times people are deliberately breaking glass, knowing what they're doing is risky business and accepting said risk. So I can see the amount of local patches far exceeding the amount of times we'd have to open an issue to discuss the removal of final somewhere.

So I'm erring on the side of final over @final again because in most cases it requires the person breaking glass to show intent, instantly making it clear to everyone on their project that something funky was done and support was forfeited.

If there is zero difference between @final and final then settling on @final should not be controversial.

You're missing s few critical points though.

1 if contrib needs something that is final they are cannot, contrib cannot require projects using them to patch core.
2 ghost wasn't blocked by drupal using final but symfony

The other big issue with final is it adds more examples of open source preventing people from using open source code.

You cannot limit the people trying to use the code to end users, if that were the case then yes we could use unfinalize and be done on individual projects, but that isn't the case here.

Why does glass have to be broken. We don't even put @internal in all internal classes.

1 if contrib needs something that is final they are cannot, contrib cannot require projects using them to patch core.

Then they would have a valid case to open a core issue asking to not mark said class as final and we get to my point about a discussion taking place: "Why does your module need to extend a class that we don't wish to offer support for?" If it's a good case, we can consider removing final or opening up our code for better alterability.

2 ghost wasn't blocked by drupal using final but symfony

What was the outcome of the issue he opened in the Symfony issue queue?

The other big issue with final is it adds more examples of open source preventing people from using open source code.

To counter with a bit of an exaggeration myself: I could mark half the classes in my module as final and people would still be able to use it just fine.

Why does glass have to be broken. We don't even put @internal in all internal classes.

But @final =/= @internal?

For internal code it could very well be that we allow internal extensions of it, but still do not wish to offer public API support for it. The difference is that we do not have a language construct to enforce that so we fall back to the second best thing. For final code we do have a construct, so that should be the default.

Again, if the decision falls to use @final I will follow it, but so far I haven't seen that many strong arguments for it. Both @final and final would have the same implications across the board, except one requires more deliberate action to get around. I find that virtual "I hereby acknowledge I'm about to fuck shit up" checkbox quite useful.

Let me put it differently: If I were using a contrib module that extended a @final class and after a while my site broke out of the blue I'd be pissed. But if said module could not be installed without patching core (until the core issue re final is resolved), at least I'd know exactly what I'd be getting myself into.

But @final =/= @internal?

It doesn't but to take something like field formatters.

They're @internal because they're plugins and we don't expect anyone to instantiate them and call their methods directly etc.

But also they're @final because we don't recommend subclassing a core plugin implementation to provide your own implementation.

However, in a custom module, I would definitely subclass a field formatter if I knew it would mean I only need to override one short method compared to copying and pasting six methods to change one - because I'd rather deal with occasional breaks to that one method than fork the entire class. But when I do this, I do so knowing that decision has implications. For me that's a good use for @final, because people who don't read and memorise the finer details of the bc policy will be warned but not locked out.

If we marked all core field formatters final in 11.2.x, dozens/hundreds of contrib and custom modules would explode.

What was the outcome of the issue he opened in the Symfony issue queue?

Completely different example, but Symfony made it impossible for us to use the validation component without subclassing @internal classes, and rejected the (very small) change we proposed that would have allowed us to avoid this:
https://github.com/symfony/symfony/pull/44911

I return, again, to my request that this issue be split into three scopes as per #44.