Add per entity type managers to improve DX

So I shortly sat down with sun to shape a possible solution:

1. Rename EntityManager to EntityTypeManager
2. Make an EntityManagerBase which has everything needed + gets the EntityManager injected
3. Entity types providing modules have to provide a respective service, along with a suiting class.

Downside:
Point 3) requires entity type providing modules to provide a respective class + service. We might be able to figure out providing the service automatically, but people would at least have to provide a simple manager class. I'd expect people to start using code-generation tools for there entity types in future anyway, so I don't see that as a big problem.

Given that one could do:
$nodeManager->load()->isPublished(); // Auto-completion for isPublished() works.
while we could inject the node manager into nodes to do
$this->manager->save($this);

The class could be defined similar to the following:

         class NodeManager extends EntityManagerBase {

            protected $entityType = 'node';

            /**
             * @return NodeInterface
             */
            function load($id) {
              return parent::load($id);
            }

            /**
             * @return NodeInterface[]
             */
            function loadMultiple($ids) {
              return parent::loadMultiple($ids);
            }
          }

Other methods like
$manager->view($node) or $manager->access($node, 'view') be inherited from the base class.

A problem lies in entity types which are not using some of the available controllers, so node types would not have a working $manager->view() right now. Either we live with that (what encourages people to implement the whole set) and figure out suiting handling for that case or we use traits to add in the respective methods only when needed.

Lastly, this would not cover contrib module controllers - but still it fixes DX for the core cases which should cover the most important use-cases already.

Isn't it the storage (controller) that's the repository? I can't see what the manager (in current speak) and the storage then separates?

Yeah, I'd see at that way as well. Would you expect a viewMultiple(), access(), or form() method on the repository?

#2/#5 seems interesting, but where does that lead us compared to the manager approach. Would we bring respective helper like viewMultiple() or form on the class as well? Then, we'd end up with quite some static methods + would have to inject all the various controller in each entity instance?

Lastly, #2 would not encourage or even work with DI, while a NodeManager service would. And it does that while making it less tedious compared to having to inject multiple controllers or just the entity manager ($this->entityManager->getStorageController($this->entityType)->save($this); - ouch)

Right, that sounds good in general - but it does not work as injected dependency. Once you'd work with dependencies, you'd have to go with various controller objects or the general entity manager again?

I'd assume we should encourage people to use DI, so it should work flawlessly - what an injected ${ENTITYTYPE}Manager would allow? Or is that not worth the troubles?

I figured, there is one problem with that approach though. Entit-type specific storage methods (e.g. load term children) would still be only on the storage controller, thus accessing them does not work so neatly any more or people would have to re-define and forward them on the entity-type specific manager (ouch!).

Right. The per entity-type manager removes the burden of having to know about all the different controllers from developers though, and makes DI and working with the API way more convenient as you'd have everything in one place, compared to having to inject multiple dependencies + caring about adding another dependency each time it turns out you need to call another method living elsewhere. E.g., a random page callback controller class that would load and render some related entities would need
- storage for loading
- access for checking view access
- view builder for viewing
- entity manager for getting the right translation context
...
Having all entity-related APIs available via a single entity-type specific manager would provide a better DX here, imo.

So, the alternative would be injecting just the entity manager + getting everything from there. I have been thinking that's where we are heading, but $this->entityManager->getStorageController('node')->load(); calls are not only needlessly verbose (as you have to specify the entity type on each method call), but moreover have we lost the possibility to do per entity-type type hints with that calls.

What really helps DX is type hinting in an IDE, and, as Berdir points out, the 'manager per entity type' approach does not provide that..

Not alone, but it makes it possible. Once we've overridden NodeManager::load() to specify @return NodeInterface, I could do
$this->nodeManager->load().
But when I override node's storagecontroller's load right now, $this->entityManager->getStorageController('node')->load() still won't pick it up.

Alternatively, it would also work with $this->nodeManager->getStorage()->load() + overridding node's storagecontroller's load.

IMO, injecting a manager is almost always wrong, your class needs to know exactly what it will use, e.g. just the storage controller. An approach of "give me everything you have and I'll see later what I'll use from it" is as "good" as injecting the whole container..

Give me a "nodemanager" is surely not the same as a "give me everything" - it's a question of getting the granularity right. Being able to say I want to work with "nodes", so give me its manager and I'm good would be convenient, while give me node A, B, C and oh Z as well isn't so.

That said, our entity controllers do a good job in separation of concerns and code re-usability. But is the knowledge of which controller is in charge of what really something anyone working with entities should have to worry with?

Putting shortcuts on the entity object itself is definitely something that works and helps to solve the problem, but it doesn't solve it for all methods working with multiple entities. So that could be a good rule of thumb, have multiple helpers on the manager, others go on the entity class.

In your example, access and correct translation stuff is available on the entity, so all you need is the storage and view builder.

getTranslationFromContext() isn't on the entity, yeah access() is, but e.g. createAccess() isn't. So you can re-phrase my example and also display a creation link if you want ;-)

Also. wouldn't this make cases where the entity type is dynamic more complicated?

I don't think so. For one you can still work generally with the current EntityManager, then I think we can make it possible to work with the type specific manager generally as well - e.g. do $entityManager->getManager('node')(not sure about the right terms here, but you get the idea).

It's actually quite nice that we've domain-specific methods on the entity type specific interfaces, like $node->isPublished(). But without that last step I don't see it becoming very useful and handy - it's all there, but by default hidden :-/

It would also enforce the first (controllers that have a specific method on the entity manager) and second class (additional ones that need go to through the generic method) controllers, by making them work completely different. Yes, they are usually not really a public API (e.g. views data generation), but still..

True, but I think that's the case already as we only provide convenient access methods for "known" controllers. I don't see that as a big problem - we can do what we can to ease usage of what we know, what's still better than leaving everything with a generic $entityManager->getController(...)

#22 sounds good in general, but it are special solutions for load() and create() only. Then, would this mean code should not use dependencies and use Node::load() instead? This doesn't seem to be a reasonable suggestion while we are shooting for a dependency injected world.

Wouldn't it be better to have a NodeManager instead which can have overridden create/load() methods with improved documentation? Maybe this could be even auto-generated?

Plus, we could do the shortcut
Node::getManager()->load($id);
for non dependency-injected code.