Problem/Motivation

In core we've held off from using final on classes because of the idea that final limits extensibility and a key feature is extensibility.

However there are cases where we've defined specific types of classes in core as not being extensible and @internal by default. These include:

  • Plugins
  • Events
  • Forms
  • Controllers
  • Paramconvertors
  • Access policies (already marked as final)

See https://www.drupal.org/core/d8-bc-policy

However people are extending these in contrib and custom code and then when we make changes to the constructors things break. For example:
https://www.drupal.org/project/drupal/issues/2594425 and then https://www.drupal.org/project/menu_block/issues/2968049

Proposed resolution

Use final instead of an implicit @internal. Put in another way we need to prioritise composition over inheritance. So a plugin that wants to borrow functionality from another plugin could create that plugin in it's constructor and then use that object. See #2968049-18: Dependency Injection issue as an example.

Making this change is likely to shift some costs from future maintenance to the here-and-now but in the long run it means that core upgrades to major and minor releases can be done with more confidence because our conventions have moved from wishful thinking to language constructs.

Remaining tasks

  • Bring in the technical working group as this is likely to involve coding standards.
  • Build core consensus - this is likely to be tricky because there is an idea that the use of final is bad.

User interface changes

None

API changes

None :) everything is already implicitly @internal

Data model changes

None

Release notes snippet

This is Drupal 9 only change.

Comments

alexpott created an issue. See original summary.

alexpott’s picture

Title: Use final to define extension points » Use final to define classes that are NOT extension points
berdir’s picture

Not sure about this. Subclassing/altering plugins is a pretty common thing to do in contrib and custom modules if you want to alter something.

I don't think copying a whole class is something we should promote as an alternative, because then you are stuck on that and will not be able to get improvements/bugfixes, which might well again break something else that expects that to be there. E.g. a views handler that adds a method/property that some preprocess code then expects to be there, same for having it as a composition.

Also, #2977107: Use more specific entity.manager services in module .services.yml files is about services, and #2471663: Undeprecate EntityHandlerBase is about entity handler (base) classes, so wouldn't be covered by this and wouldn't help these issues.

Just my first thoughts on this for now ;)

wim leers’s picture

I don't think copying a whole class is something we should promote as an alternative, because then you are stuck on that and will not be able to get improvements/bugfixes, which might well again break something else that expects that to be there.

That's exactly the point though: the expectation that granular overrides in subclasses need to continue to work is the very problem this is solving. Because it means that contrib/custom modules do break when updating core/contrib modules, even when core/contrib modules are really just fixing bugs for themselves. Quite often bug fixes require code changes that necessitate changes to internal architecture.

If a contrib/custom module wants to reuse more, then they should advocate with the maintainer of the core/contrib module to extract portions into traits, utility services/classes or … even make the plugin non-final.

The concrete example you give (a views handler) may very well already be vetted enough to be a maintainable, supportable, evolvable API. The problem is that we need to first put in conscious thought into designing it in such a way that it can do such things.

I would agree with you if we'd be arguing that every class needs to be final. But this is arguing that most things should be at least initially be final — that's very different.

IOW: only alter (subclass/override) things that have explicitly been marked as a supported API, and enforce this through final, so that contrib/custom developers now explicitly realize that they're signing up for maintaining the whole thing since they have to copy/paste it instead.

This would prevent a significant amount of "oh no core broke my module in a minor core update" consequences and the consequent general fear of updating to core minors that has been accumulating.

alexpott’s picture

Issue tags: +Drupal 9

Tagging this with Drupal 9 as this is something we can obviously only change on a major release.

berdir’s picture

I do get your point, of course it would make it easier for "us", but I think this proposal might be written/defined too much from a core-maintainer perspective, without considering what contrib/custom has to do and what needs to be done to do per-site customizations.

> This would prevent a significant amount of "oh no core broke my module in a minor core update" consequences and the consequent general fear of updating to core minors that has been accumulating.

I doubt that we can really change that, not with extremely generic extension points like hook_form_alter() for example.

I also wouldn't be surprised if we would get an increased amount of "bugfix/feature X isn't working for me" issues, caused by some contrib module replacing a core class and as a result doesn't have that feature/fix. So having an explicit error in such a case would in some ways even be preferable compared to it just silently not existing.

catch’s picture

I also wouldn't be surprised if we would get an increased amount of "bugfix/feature X isn't working for me" issues, caused by some contrib module replacing a core class and as a result doesn't have that feature/fix. So having an explicit error in such a case would in some ways even be preferable compared to it just silently not existing.

Yes this is always the trade-off and why we originally went for @internal instead of using final. With @internal you have a choice to extend/copy/compose, with final we've made the choice for you. When code is copied you miss both refactoring and bug fixes.

I do get your point, of course it would make it easier for "us", but I think this proposal might be written/defined too much from a core-maintainer perspective, without considering what contrib/custom has to do and what needs to be done to do per-site customizations.

So I'd agree with this but there's also people doing minor core updates on specific sites - which is sometimes us as either core/contrib developers, but also site builders who aren't writing code, potentially auto-updates. I don't think this is our biggest problem for minor update reliability but if you update core and nothing else, and your site breaks, core will get the blame and it contributes to distrust of core updates.

I kind of feel like @internal + best effort backwards compatibility (more or less what our current compromise is) provides the most flexibility with the least breakage. Also not everything is method changes, there's also issues like this #2815379: Move message, goto, and email action plugins to Drupal\Core\Action - don't really think final has any impact either way on that, only way to offer proper bc would be to leave the old classes there deprecated.

alexpott’s picture

@catch why won't final work for #2815379: Move message, goto, and email action plugins to Drupal\Core\Action - if you were extending via composition and injecting an instance from the plugin manager then you only need the plugin ID and that's not changing - yes dependencies are changing but that's not the biggest thing especially in that case.

wim leers’s picture

I think this proposal might be written/defined too much from a core-maintainer perspective, without considering what contrib/custom has to do and what needs to be done to do per-site customizations.

It's equally for core maintainers as it is for those who are operating actual Drupal 8 sites: they're the ones who are most adversely affected.

catch’s picture

@alexpott #8 hmm that's probably right, don't think it helps for other examples such as forms/controllers etc though where there's no ID as such.

mikelutz’s picture

I'm for using final on internal classes. It shouldn't be used all the time, but if something is supposed to be internal, it seems reasonable to enforce that with language constructs. It makes for a more stable end product.

If it's something we are going to do in Drupal 9, should we decide soon and do something like this in Drupal 8?

mikelutz’s picture

Version: 9.x-dev » 8.7.x-dev
mikelutz’s picture

Oops, picked a bad class to try it on, lol.

mikelutz’s picture

Version: 8.7.x-dev » 9.x-dev
Status: Active » Postponed

Discussed in #3019442: Drupal 9 readiness meeting / 10 December 2018. It was suggested not to mark classes final in D9, but to consider the implications and possible begin deprecating extending certain classes in D9 and then marking them final in D10.

wim leers’s picture

Version: 9.x-dev » 8.7.x-dev
Status: Postponed » Active

D10 is at least another half decade. #3019442: Drupal 9 readiness meeting / 10 December 2018 is just a placeholder issue without any information right now. Reactivating this issue to ensure we have a clearly documented, very strong rationale for not doing this.

alexpott’s picture

Also the rationale for not doing this needs to somehow get around the fact that if we don’t enforce our conventions via final the free-for-all means that popular modules extend internal classes all the time. If we want reliable unattended updates it feels like something has to give.

wim leers’s picture

Exactly. That's another consequence of this bit I wrote in #4:

This would prevent a significant amount of "oh no core broke my module in a minor core update" consequences and the consequent general fear of updating to core minors that has been accumulating.

If manual updates suffer from this, then automatic updates of course will also suffer from this.

wim leers’s picture

Also note that I am not arguing to suddenly make all classes final in D9. I'm only arguing that new classes should be generally be final, except when they're explicitly designed for reuse.

How to get existing classes to become final is out of scope here IMHO. That is far, far trickier. Let's start with the simple thing first.

alexpott’s picture

Another important point about using final is that then core has to obey it's own rules. I.e. if we want to extend one of the implicitly internal things we do and then it's okay because we maintain all the classes in the chain BUT then custom and contrib pick up our bad habits.

alexpott’s picture

The final discussion has come up on #3009377: Replace drupal_get_user_timezone with a date_default_timezone_get() and an event listener because someone has extended the \Drupal\Core\Session\AccountProxy object / ie. the class behind current user service. The use-case appears to be an new way of extending the authentication API however there are other ways of doing this. For me we should be adding final to such classes to make it clear that we don't think that extending them is something that should be done. And perhaps over time point to documentation of how to implement likely use-cases for doing so. Of course someone in this case could have copied the entire class into theres and I think that that is okay because at that time you are fully aware of the choice you are making and core is not forced into tough / impossible decisions.

xjm’s picture

I've avoided saying anything here because I think @Berdir and @catch articulated already what I think of this issue and I don't really want to discuss it on this issue, but it seems this is not going away, so:

  1. This will not block the release of D9. I'm opposed to even trying to adopt it prior to 9.0.0 shipping. Too disruptive, too much work, not enough time.
  2. In general, most of our @intenral code is not even marked as such yet, so it's vastly premature to say that "@internal isn't working". Let's finish that first, and maybe improve our tooling around it once core actually tells you what code we consider @internal, and then we can discuss further steps.
  3. Controllers and form controllers -- It is necessary to break these in order to change the UI. However, extending them is also a totally reasonable and probably very common workflow, so I don't think final is the right way to go about it at all. I'd much prefer to improve our tooling to communicate the tradeoffs of choosing to extend this code
  4. Other things listed in the IS -- I've heard of exactly 0 cases where someone's site was broken because they were subclassing one of these things. Okay, now I've heard of one from #20. (And did it have an @internal on it explicitly?) So if @Berdir thinks there's a reason for them to not be final, that's enough reason for me for them to not be final. @Berdir maintains enough contrib code for that to be legit.
  5. (Edit) This was mentioned previously relating to automatic updates, but we're not going to offer automatic updates for minor release upgrades anyway, so that's a moot point.
  6. (Edit) Finally, all the things that actually have broken people's sites I summarized previously in: #2909665-27: [plan] Extend security support to cover the previous minor version of Drupal. Notice what's not on the list.

(Edit) Overall, I feel like this is something that is intended to mitigate the annoyances of providing best-effort BC for internal code for core developers, without really helping site owners or module developers, and at the expense of Drupal's flexibility.

alexpott’s picture

alexpott’s picture

dawehner’s picture

We have really strict and quite tricky to understand rules what can change and what can't change in a patch/minor release. Following these rules are hard, but if they are followed they generate a similar kind of benefit what final classes would offer as well. Having to think less about it would

Much like everything there are pros and cons in this world. I'm wondering whether it would be possible to have an experiment, for a limited subset, like for example the book module or some experimental module.

All this aside, I'm wondering whether we could make a recommendation for contrib to make their classes final. Contrib authors have less resources and the probability that an override is the best solution might be even lower than for core.

cilefen’s picture

I’m not sure how much this affects developers in practice—I haven’t looked atound core yet—but AFAIK it isn’t trivial to mock final classes in tests. But you can mock objects out of interfaces.

tstoeckler’s picture

I am very strongly against this for two (separate, albeit related) reasons:

  1. In the comments above "the ability to use fine-grained/minimal overrides" and "the expectation that those overrides do not break on core updates" are being noted as going hand-in-hand, but I don't think that's the case. For contrib modules it certainly is, but for custom modules that is not necessarily the case. I often use these fine-grained overrides in custom code but I am absolutely aware that this might mean that things break and (not always, but sometimes) that is a trade-off I am willing to make if it means I won't have to maintain a hole sloth of duplicated code. I can make that trade-off because I know I will be able to rectify it - most probably in a matter of minutes, not hours - if something breaks. But using final is absolute, I cannot go around it even at my own risk. That's why I would much prefer something like @internal being added in all those places, because then my IDE warns me that I'm doing something unsupported, but I can still do it, if I really want to, and that's quite important to me.
  2. As is already noted above, some of the affected classes, in particular plugins and controllers, are massive in Drupal. Without going into whether that's achievable (or desirable), this would be a different discussion if all of Drupal were following patterns like separation of concerns and the single responsibility principle - or in other words if most classes in Drupal were in the realm of 100 lines of code, not 1000. I think a lot of people are vastly underestimating the maintenance burden this change would incur to especially custom site owners that now would have to maintain massive amounts of duplicated code.
larowlan’s picture

Over in #3027053: [Policy, no patch] Allow experimental modules to use final to limit public API during development phase I said this

Aside from mocking shortcomings, I am in favour of using final to enforce what is and isn't an extension point, along with private as well.

Language features will always be a stronger signal than documentation, convention and policies.

and @WimLeers asked me to comment here as well.

I think #3027053: [Policy, no patch] Allow experimental modules to use final to limit public API during development phase provides a good sandbox to test this in. It is low impact and it effectively communicates the fact that experimental modules are not supposed to be extended because they're experimental.

I am not convinced about controllers, forms and plugins. Extending those is a fairly regular occurrence in client projects. In terms of the constructor signature changes, Search API demonstrated a decent pattern of how to do that safely, I wrote an article about that pattern. That would equally apply to controllers as well.

effulgentsia’s picture

I am not convinced about controllers, forms and plugins.

Same here. I like the idea of trying out using final somewhere, but not as broadly as this issue is currently proposing. +1 to #3027053: [Policy, no patch] Allow experimental modules to use final to limit public API during development phase as the initial step for where to try it out.

However people are extending [@internal classes] in contrib and custom code and then when we make changes to the constructors things break.

I imagine it's not only constructor changes that cause such breaks, but perhaps those are the most common breaking changes that we make in practice. In any case, I like the idea in #13. What if we add a trait like that for triggering an error (not sure if it should be E_USER_DEPRECATED or some other E_USER_* error type) when an @internal class is extended? For contrib, that would cause test failures, so would address the problem this issue is trying to solve in terms of minimizing contrib maintainer surprise at something breaking after a minor core update. But for custom code, per #26.1, it would provide a mechanism to override that error (e.g., by overriding the trait method that throws the error), for the cases where the code maintainer is willing to deal with breaks when they occur. I suppose it would remain possible for contrib to then suppress the error as well, but I wonder if there's something else we can do to deal with that. For example, encourage contrib projects that extend @internal classes and suppress the corresponding error to flag their module as only working with the current core version (e.g., a dependency of drupal:system (<8.7)). Since they don't yet really know if they work with the next core minor. Then, when the next core minor comes out (or during the beta/RC phase), they can run their tests to see if it works, and update that dependency to the following minor (e.g., drupal:system (<8.8)). It means such a contrib maintainer needs to actively check their module against core every 6 months (instead of forgetting about their module for a year or more), but I think that's a reasonable price to pay for extending an internal class, and if you're not able to do that maintenance, then a better option for you is to not extend @internal classes. If contrib maintainers follow this approach, then site builders who use those modules won't find themselves in a situation where they successfully updated to the next minor but then have a broken codebase throwing unexpected errors or otherwise failing in unexpected ways.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

james.williams’s picture

@effulgentsia flagging a contrib module as potentially incompatible with future core minor updates would leave many site builders unnecessarily stuck - as I imagine in the majority of cases, the future core minor update would not break anything (even if we can't be certain of that, sure). Many (most?) contrib modules may not get around to updating that constraint every 6 months.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

xjm’s picture

Adding final is also the type of coding standards change that I'd only want to adopt with a new major, because while it's technically currently treated as an internal BC break for the APIs proposed, it's a big one, and we try to provide a continuous upgrade path even for internal code. We'd have to figure out what our practice is going to be for BC-breaking new coding standards in a major version.

In #3072702: Core extensions should not need to specify the new core_version_requirement in *.info.yml files so that 9.0.x can be installed, it occurred to me that core test classes (non-base-classes) might actually be a place to adopt final consistently. They have to be public with public methods for the test runner to call them, but weird stuff happens if you subclass them (the test methods get run for each subclass test, which we do intentionally in a few test base classes, but it is super weird and not what you want in most cases).

borisson_’s picture

In #3072702: Use new core_version_requirement in all module and theme info and yml files so that 9.0.x can be installed, it occurred to me that core test classes (non-base-classes) might actually be a place to adopt final consistently. They have to be public with public methods for the test runner to call them, but weird stuff happens if you subclass them (the test methods get run for each subclass test, which we do intentionally in a few test base classes, but it is super weird and not what you want in most cases).

That sounds like a really good idea, +1

borisson_’s picture

To elaborate on the comment I wrote yesterday, I think starting with test classes is a good get contributors used to having final classes in the system.
I don't think we will get any other actual gains from that because our test classes aren't that important. If we want to wait for a major release to introduce final classes then it probably means this shouldn't happen before 10.x. I think waiting for that is a very long time. We can start introducing final with new classes/modules that we add to the system without any risk.

catch’s picture

So I did actually extend a bunch of entity type tests in Drupal 7 for the entitycache contrib module, but that was an extremely specific use-case (and the module doesn't exist in 8.x). https://git.drupalcode.org/project/entitycache/blob/7.x-1.x/entitycache....

There is some core code that is definitely never supposed to be extended, like the doctrine annotations fork we just did, where I think we can use final now without this overall issue being resolved. If someone wanted to extend that code we'd want a core issue about it since it's supposed to disappear soon too.

Tests also seem like a good place to introduce it - if someone is extending core tests in contrib we'd want to know about it and we can probably find a different way to achieve the same thing.

I'm still not very keen on doing this for not-really-an-api-but-you-might-want-to-extend-and-tweak things like plugins and controllers though.

chi’s picture

+1
I think in the end all not-abstract classes should be final.

I don't think copying a whole class is something we should promote as an alternative,

The alternative should be adding more abstract classes to Core. That would probably require some research of contrib codebase to identify most subclassed core plugins, services, etc.
Worth noting that making a class final should be accompanied with a few other syntactic changes.

  • static => self
  • protected => private
  • get_class() => __CLASS__

It's obviously too late to introduce this in D9, but we could start by adding the deprecation notice to all existing classes (#13) and making new classes final (#18).

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

xjm’s picture

@longwave suggested in #3131900: Refactor assertions that assign return values to variables that test helpers should be private. I might go a step further and say that most individual test classes should perhaps be final (and it's the one place in core that it's a fairly clear choice IMO). There are the rare cases where we deliberately subclass tests to run test methods on multiple different child setups, and I've heard of contrib extending tests occasionally, but the 99% case is a class that should not be extended.

Edit: Belatedly realized I'm repeating #32 and alluding to points in #35.

larowlan’s picture

There are the rare cases where we deliberately subclass tests to run test methods on multiple different child setups

- yep and then by removing the final keyword, we're deliberately signaling this is an extension point.
So yeah, +1 to that.

mxr576’s picture

- yep and then by removing the final keyword, we're deliberately signaling this is an extension point.

How about always adding (and updating existing) non-final classes/methods with @api and @internal annotations, just to let them clearly known what is considered as an extension point and what it is not. It is kinda documented on Drupal.org but it should be rather documented in code. Also, IIRC last time when I checked how many @api annotation in core I found less than 10?

chi’s picture

@mxr576 I think @internal on class definition also forbids using public methods of the class, not just extending it.
Also PhpStorm does not work quite well with it.
https://youtrack.jetbrains.com/issue/WI-31674

geek-merlin’s picture

I do understand both
* the need for a convention to mark not-to-inherit classes
* and the practice to do quick monkeypatching via inheritance

An ideal solution would be PHPDoc "extend on your own risk" tags that are IDE-supported but do not throw if violated.

Let's nudge PHPStorm to support @internal. Which we may use on classes as well as methods.

chi’s picture

An ideal solution would be PHPDoc "extend on your own risk" tags that are IDE-supported but do not throw if violated.

I disagree. That is a partial solution. PHP does give us an ideal solution which is final keyword.
For classes that have lots of protected methods we can add abstract classes. So that custom and contrib modules can still reuse code from core.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

aaronmchale’s picture

I have mixed feeling here.

As a matter of principle, I completely agree, if we didn't intend something to be a public facing API for other developers, and because we give ourselves the freedom to change internal classes (essentially) at will, then we should do our best to make sure that Drupal updates don't break custom and contrib code (excluding major BC-break updates). This would be one way to guarantee that.

However, in practice, when writing custom code, it's sometimes really useful to be able to extend a Controller or Form that core uses. One specific example that I've done recently is to extend ToolbarController in the toolbar module to override some parts of the class to change what is rendered in the toolbar. I didn't want to write it from scratch, because I still need 80% of what is in there does the job. In my case this is in a custom Drupal profile, and I know ToolbarController isn't a public API; But, I'm okay with accepting that risk/responsibility because this is a custom profile that for a site I run that I have full control over. That means if something does change in a minor update that breaks my code, I'll know that and I can react to that in good time, as I'm updating the site.

So, I'm a bit split on this one, on one hand it could be seen as good practice, on the other hand the current flexibility is sometimes very advantageous.

gabesullice’s picture

@AaronMcHale, I hear your concern, but there's a simple and maintainable solution: If you need to extend a class marked final, create a feature request to place that class into the public API as an extension point by removing the final keyword. For example, ContentTypeBase and DefaultSelection are clearly good candidates (and would never be marked final to begin with) while PharExtensionInterceptor is probably not a good candidate as it stands. By following such a process, we can think critically about our OOP designs and how they fit together.

The goal isn't to turn core into an immutable black box, it's about being honest with ourselves about our limited capacity to maintain BC on every class in Drupal core, whether it was intended as an extension point or not.

The widespread adoption of composer also makes patch management much more reliable and feasible, so downstream developers can always create such a feature request and immediately apply their own patch making the altered class open to extension before their issue is even committed.

mglaman’s picture

I did a quick experiment using php-parser: https://github.com/mglaman/drupal-finalifier

I scanned the drupal/core package for classes that: we not abstract, not already final, and did not extend other classes. There were about 400 results for classes the could be considered final.

I then wrote a script that takes those class names and checks if a contrib extends any of them. For Commerce + it's dependencies, I got these results of classes that were candidates for final but were extended:

[
    "Drupal\\Core\\Entity\\Routing\\DefaultHtmlRouteProvider",
    "Drupal\\Core\\Entity\\Sql\\SqlContentEntityStorageSchema",
    "Drupal\\Core\\Entity\\Sql\\SqlContentEntityStorageSchema",
    "Drupal\\Core\\Entity\\Routing\\DefaultHtmlRouteProvider",
    "Drupal\\Core\\Entity\\Sql\\SqlContentEntityStorageSchema",
    "Drupal\\Core\\Utility\\Token"
]

So just about anything entity related: doomsday to mark final. The Token class is extend by Token.

I committed JSON files of data to the repo.

alexpott’s picture

@mglaman the good thing about yuor work in #48 is that none of what you've listed is covered by the targets in the issue summary ie...

  • Plugins
  • Events
  • Forms
  • Controllers
  • Paramconvertors
mglaman’s picture

@alexpott that's because I excluded anything which has a base class. Anything in the issue summary generally has a base class they extend.

I picked some arbitrary rules to decide from.

neclimdul’s picture

Version: 9.3.x-dev » 10.0.x-dev
Issue tags: -Drupal 9

I _really_ dislike this. I started writing some thoughts but it became like a 5 page rant so I just say I think many of the reasons have already been discussed that this _really_ hurts site builders and the idea that it would help maintainability seems like it is wishful thinking and will just lead to more elaborate and troubling hacks to work around things meaning we aren't actually gaining anything from saying @internal "you're on your own if you extend this".

Needless to say applying this in any large measure as the summary suggests would likely be _hugely_ disruptive and breaking by adding language faults to things that are currently discouraged but allowed so moving this to 10.

mglaman’s picture

I'm not sure how this hurts or impacts site builders who rarely write code, or only interact with hooks.

neclimdul’s picture

I mean, I build sites, and do those things. Also hire contractors to help me build those sites and they frequently do those things as well. So... I think we're using different definitions of site builders.

gabesullice’s picture

I'm not going to start a metadata war on this issue by changing it back, but I disagree that this is should be in the 10.x version. Obviously, no committer is going to slap final on a class before it is properly deprecated. That deprecation will need to happen in a minor version of the active major version (i.e. 9.x).

I guess the issue summary needs to be more explicit about exactly which steps we would need to take to mark a class as final and it is clearly not aligned with the current discussion any longer. Added a tag to update it.

I started writing some thoughts but it became like a 5 page rant so I just say I think many of the reasons have already been discussed that this _really_ hurts site builders and the idea that it would help maintainability seems like it is wishful thinking and will just lead to more elaborate and troubling hacks.

@mglaman's static analysis work should be seen as proof of the extreme care that we're taking to not be disruptive. Avoiding the BC song-and-dance on method signature changes within final classes is a tangible maintainability improvement, not a hand wave.

I'm not sure how decorators around finalized classes and implementing interfaces would be a hack especially in comparison to extending classes that no one purposely designed to be extended in the first place.

In the pre-OOP days, we repeated the mantra don't hack core. It meant: don't change functions inside core. Everything was final because the unit of abstraction was the function itself... and functions are not extendable. When we switched to OOP, where the primary unit of abstraction is the class, we suddenly decided that that was a bad idea?

On a philosophical level, I don't see how extending a class, overriding a protected method, and replacing it via dependency injection can be seen as any different than hacking core in the pre-OOP days. It's shouldn't suddenly be okay and part of Drupal's inherent flexibility because the extending developer jumped through some fancy OOP hoops to do it.

IMO, we should be embracing the final and private keywords and let their absence be a clear signal that says hey, remember how you weren't able to hack core before? Well, now because of OOP, we have a safe way to do it! Here's some starter-code. Here's some helper methods. Here's an interface you can implement to provide custom-yet predictable-ways to customize what was never customizable before.

Where we are today with OOP is a historical accident. Luckily our deprecation process and tools like @mglaman wrote give us a way to self-correct in a considered and cautious way.

james.williams’s picture

Version: 10.0.x-dev » 9.3.x-dev

@gabesullice that's just persuaded me, thank you! I think that's such a good corollary; that extending classes that weren't intended for extension, is like hacking core in the pre-OOP days.

And as you've pointed out, the scope of this work would initially be about starting the process, not actually marking classes as final yet. So I'm reverting the version here. This ticket is, after all, just the 'plan', not the eventual implementation task, which may well be something for 10.x (if at all).

neclimdul’s picture

So I'm still 100% adamant this is a bad idea still. But maybe that's because of the issue summary. What you're describing from old versions of Drupal of things that would not be extensible where generally places where we'd have to add hooks in to make things extensible. All of the things listed _are_ those hooks and the fact that they are extensible was kinda the point of making them classes imho. In a lot of cases they're quite a bit more complex then just writing a method somewhere but we gained some great injection and extension benefits.

For all the things listed, I really thing final _is_ a bad idea. Its where API's will go to die.

1) Unit tests get bad. You can't do a lot of the tricks you would normally do that use inheritance to mock or force methods values. If you want better auto updates, you want faster more granular tests, and this makes that worse.
2) Unit tests get bad again. You can't mock this in _other_ systems because again all the testing process rely on inheritance Again, bad for auto updates.
3) You can't predict what contrib and site customizers(better term then builders?) will need to do. "open an issue" isn't going to work, I'm waiting on feature for core customization from like 8.0 beta and I work on core so normal developers have no chance. Patching also not reasonable as telling people "hack core" is always the wrong answer. It may be "easier" now but its not less complex. If the point of this is to improve auto updates and your solution to the side effects is make auto updates _worse_ by patching core then its not going to work.
4) I even hesitate to use final outside of these extension points. Final makes future prototyping and refactoring really hard because types get locked in since you can't extend things and it becomes a tangled web of impossible. I've tried to fix and improve the Settings API dozens of times but its impossible at this point.

I guess a more personal feeling on it, I've run into a couple final classes in things like Symfony, and its like a giant red flag to stay away from the entire section of the library. I know from experience now that the minute I run into trouble I've got no wiggle room to fix something and it won't get noticed till crunch time so its probably worth looking for a different library or writing something from scratch. Kinda defeats the point of using something like Plugins in Drupal if that's the case though. Maybe like the managers and stuff but if we're talking plugin implementations... yikes.

I have to wonder, instead of boxing out this entire class of customization that _I_ think has made Drupal 8+ successful, if mglaman is making this cool parser and scanning things already based on @internal, why aren't we making that like a lint or CS rule or something. Like "hey this is going to possibly break auto updates" or "this may be a bad idea" but that leaves contrib etc open to continue doing cool things while maybe tracking a core issue that takes years to resolve.

gabesullice’s picture

There is definitely a mismatch in this issue between what you think this plan is and what I think this plan is. I appreciate you taking the time to lay out your reasons for why you feel this issue is a bad idea. I will take the time to do the same and then update the IS.

I'm ignoring the issue summary and focusing on the title. The title is almost tautological: if a class isn't meant to be extended, it should be final. That's very clear and hard to argue against. As a community, we shouldn't be afraid of the final and private keywords. They exist in every major OOP language for a reason and we are not above (or below) those reasons. We should use them with good, engineering discretion.

Since that tautology is clear, the disagreement must be about the imagined outcomes. Indeed, this is where all the disagreement seems to be focused and why I believe we are all arguing past each other. The proponents imagine the issue's resolution will be painless and lead to easier maintenance; the opponents imagine it will lead to unacceptable breaking changes and rigidity.

Our imaginations aren't serving anyone well. Let's proceed like so:

The proponents on this issue must be responsible for identifying concrete examples of existing core classes which should not be extendable. After identifying them, they propose to deprecate and eventually mark them as final in a major version bump.

Then, opponents of that precisely defined change should object to that change (if they object) with a practical and reasonable example of what they would be unable to accomplish after the change.

Finally, proponents should take the time to hear those concerns. If they still feel their proposal is legitimate, it will be the proponents' responsibility to satisfactorily address them to a core framework manager.

chi’s picture

Re #56. The main point of using final is ability to refactor code without carrying about BC. Static analyzers won't help on this as they are not widely used and can be easily ignored. Currently core maintainers tend to decline patches that may break existing code even when the target class is not part of public API. Otherwise we would get lots of reports like "My site is broken after update to Drupal 9.x.x!".

I think, inability to refactor code due stict BC policy one of our biggest problems that makes Drupal core more and more outdated.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

andypost’s picture

catch’s picture

kristiaanvandeneynde’s picture

Issue summary: View changes
ghost of drupal past’s picture

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.