Undeprecate EntityHandlerBase

Move the module handler dependency to the implementations that need it but don't yet use the interface.

Certainly +1, this is more of an edge case.

Add StringTranslationTrait and DependencySerializationTrait wherever they are actually needed.

DST is tricky, as it leads to all kind of bugs all over the place. Also the STT is used in some many places, it would be annoying if you
would have to add it all the time. For generic plugins we have decided to put those traits on everything, see \Drupal\Core\Plugin\PluginBase so we should follow that pattern, to be honest.

We can at least mark this class as @deprecated: #2539848: Mark EntityHandlerBase as @deprecated for 9.x.

Making this a child of #2729597: [meta] Replace \Drupal with injected services where appropriate in core because EntityHandlerBase has a reference to \Drupal::moduleHandler(), and because many of EntityHandlerBase's subclasses use \Drupal accessors due to not implementing IoC injection. (Say it with me: \Drupal is an antipattern.)

Setting to 8.3.x. Not sure why it's marked postponed.

EntityHandlerBase is basically what ModuleHandlerTrait would be if we made it.

It's probably not a good idea to have a trait per service, but if the goal is to remove EntityHandlerBase, then this is what we should do.

Then all the subclasses can use the trait, call setModuleHandler() in their factory method, and we're golden.

This patch:

• Creates ModuleHandlerTrait.
• Immediately marks ModuleHandlerTrait as deprecated, so no one actually uses it. :-)
• Moves the code from EntityHandlerBase to the trait.
• Immediate subclasses of EntityHandlerBase in core use the trait and cease to be subclasses.

Note that the trait uses \Drupal::moduleHandler() to glean the service if it's not already set. All core classes set the service in the factory method, so only contrib will need this.

EntityHandlerInterface is poorly named if it's only there to signal IoC injection, though it's the perfect name if entity handlers are required to inject this way. And this is a requirement I'd support.

If it's going to be necessary to call hooks from all entity handlers, then moduleHandler() or some invokeHooks() method should be part of the interface.

Picking up the stragglers.

Patch still applies. Running test again.

I don't see the point of replacing a @deprecated base class with a trait that is deprecated too. We end up with 5 lines of additional code in total and don't gain anything?

Either we add a non-deprecated way to add this for the places that need it So that we can stop using any deprecated code in core and just remove it in 9.x, or we postpone this to 9.x and do it in a BC breaking, clean way.

And we could definitely already do so with the two test classes in the patch that use this, which right now use the trait *and* still extend from the base class?

I don't see the point of replacing a @deprecated base class with a trait that is deprecated too. We end up with 5 lines of additional code in total and don't gain anything?

The thinking was that this would join the traits we eventually remove or properly inject in #2733703: [plan] Service traits should require IoC injection

Also, see the first paragraph of the issue summary.

Looking at this some more (surprisingly easy to reroll), it's the same problem as all the other base classes: EntityStorageBase for instance needs the module handler. So either you use a trait which breaks IoC by using \Drupal, or all subclasses have to manage the create/__construct cycle for all superclasses.

This is why #3 in the proposed resolution isn't such a good thing to do, at least not in Drupal code-land.

3. Move the module handler dependency to the implementations that need it but don't yet use the interface.

#3 needs some clarification/explanation, since it's odd that we have this disconnect in the first place.

In fact, the solution is to convert to a more compositional mode and turn classes like EntityStorageBase into traits which have setter methods for their required services. This way the static factory for the class which uses the trait can call the setter for the services it needs, without having to pass all dependencies up the constructor chain for all the intervening subclasses. (A *lot* of classes extend EntityStorageBase.)

But that's not what this issue says to do, so here we are.

Here's a re-roll of #10, for completeness' sake.

I found this again and even filed a now-closed issue about it: #2985768: EntityAccessControlHandler is based on the deprecated EntityHandlerBase

So here's a re-roll.

+++ b/core/lib/Drupal/Core/Extension/ModuleHandlerTrait.php
@@ -0,0 +1,60 @@
+ * @deprecated in 8.6.x for removal before Drupal 9.0.0 release. Perform your
+ *   own injection of dependencies to entity handlers using
+ *   \Drupal\Core\Entity\EntityHandlerInterface.
+ *
+ * @see \Drupal\Core\Entity\EntityHandlerBase
+ * @see \Drupal\Core\Entity\EntityHandlerInterface::createInstance()
+ */
+trait ModuleHandlerTrait {

We're adding a trait that is immediately deprecated, which is fair enough, but our code is also being changed to use it.

Do we therefore need follow-up issues to remove use of this trait from core?

Yeah, I don't really see the point of replacing one deprecated thing with another.

I'm currently working on finishing #2624770: Use more specific entity.manager services in core.services.yml , that will add a trait to help with deprecated properties, so maybe we can use that here then as well.

#2624770: Use more specific entity.manager services in core.services.yml is in, so we should update the patch based on #21.

I started doing that, but kind unsure about it. storage + list builder + view builder all actually need the module handler, so it wouldn't be a deprecated property from them, we actually need to inject the module handler to be able to use it. Which means (again) requiring a constructor change for all entity storage handlers (10), view builders (3) and list builders (20 in core alone) handlers with an own custom constructor.

And changing EntityHandlerBase doesn't make sense, it is already deprecated as a whole, so we can just leave it as it is.

So maybe we *should* have a ModuleHandlerTrait? That is *not* deprecated, just like StringTranslationTrait, we also recently added MessengerTrait, so I don't think it's something we stopped doing.

Discussed with @alexpott. We came to the conclusion that we should probably just revert the actual deprecation of this. It has been marked as deprecated a long time ago, when we still deprecated things without having an complete plan in place to deal with that deprecation.

The points in the issue summary and the gain from removing this is not really worth the effort and time that was already spent and would still need to be spent. It doesn't actually help us (enough) with our technical dept or complexity or so.

The main argument is this:

> On the other, the base class is a layer of misdirection we don't need, since it does not implement, nor reference, nor remotely resemble EntityHandlerInterface

But that's just as much the fault of EntityHandlerInterface, which also doesn't provide anything that's actually entity handler related. What it should have been called is ContainerFactoryEntityHandlerInterface, since that's all it does and all it will ever do, considering that we can't anything new to it without likely breaking a ton of subclasses. It's not technically @api, but as close to it as we have IMHO.

So the proposal is to remove the @deprecated definition. Maybe here, maybe in a new issue in and keeping this open in case someone finds a good reason to do this. We could then however still create a new issue.

Seems like nobody objects to that, so here's a patch.

Rtbc+1 do we need to update any chance records?

There are no change records that mention EntityHandlerBase, so I'd say we don't need any updates.

Wondering if we should mark this explicitly as @internal, as it's only meant to be a base class for entity handler base classes?

> Wondering if we should mark this explicitly as @internal, as it's only meant to be a base class for entity handler base classes?

What about contrib modules that define a new type of handler? Presumably, they might want to make use of this too?

The IS could do with an update, as it now describes the opposite of the current patch.

Re marking this class @internal - we already have direct usages in contrib. The Feeds module for example.

The patch applies and removes the deprecation has expected.

Committed c8af78e and pushed to 8.8.x. Thanks!