You are here

Ensure custom EntityType controllers can be provided by modules

Problem/Motivation

Contrib (and even optional core modules like config_translation) have no uniform way to add and invoke custom controllers.

EntityType does not have bespoke getters for controllers, and is missing setters for some.
If an EntityType class is swapped out and wants to override a controller-specific method, it will be completely bypassed by EntityManager.
Furthermore, these entity controllers share a lot more in common than they did initially.
For example, every single type of controller (excluding forms, which use FormInterface/ContainerInjectionInterface) need the module handler, but half use setter injection and half pollute their constructor.

Proposed resolution

Add EntityManagerInterface::getControlller() to help contrib/optional modules
Add getters and setters to EntityType for core controllers
Add setModuleHandler to all, not just half, of the controller types that require it
Fix config_translation to be the example of best practices, and not a clever workaround
Write tests to cover it all

Remaining tasks

N/A

User interface changes

N/A

API changes

Changes

EntityManager::getControllerClass() is replaced by EntityManager::getController()

Drupal\config_translation\Exception\InvalidMapperDefinitionException is now Drupal\Component\Plugin\Exception\InvalidPluginDefinitionException, and is to be used whenever a plugin definition does not meet expectations (instead of the previous usage of \InvalidArgumentException)

The 'list_controller' property of hook_config_translation_info() is gone, you should use hook_entity_info() or hook_entity_info_alter() to set the 'config_translation_list' controller:

This compromises and adds a param to getDefinition to throw an exception.
It turns out that ever view builder and storage class also needs the module handler, making everything but forms the same. So I streamlined that.
config_translation, once again, was doing weird stuff, so I tried to clean that up. It helps highlight how contrib should define and invoke custom controllers.

The old title is one thing this does on the path towards this goal: Contrib should be able to sanely provide custom controllers.
config_translation is an example of how it was not easy, so workarounds and hacks were added.

This cleans them up, provides the ability for contrib to follow suit, and adds a good amount of test coverage.

- getList() / getAccess() - makes no real sense, doesn't return a list, doesn't return an "access".
I don't think we want to be rehashing the whole controller/handler/(null) debacle here. For now all of those are still called controllers, why don't we stick to that here, and rename in the issue that's supposed to rename ?

- Great thing that we unify how those various objects are instanciated, but:
Some receive a moduleHandler, some don't ? Yet they all have relatively equal chances to have a need for invoking a hook or an alter ?

Similarly, it's common for one controller to need functionnality provided by another controller, thus needing to get back to the EntityMangar. Could we inject the entity manager in all of those ?
(hm - or is that covered by the fact that they receive the EntityType, which now gives you shorthands to any controller ?)

Those are getters for the similarly named properties. There are already setters with those names.

AFAIK, all of the non-form controllers receive the module handler. All can easily opt into it by implementing a setModuleHandler method...

With the exception of the list controller needing the storage controller, I don't think any controllers ever need the other controllers.
They all can use injection to get whatever they want, as in HEAD, this is not changed here.

1) Fixed
2) Added
3) Reverted
4) Fixed
5) Fixed
6) Removed
7) Actually, I removed this as well, since it required overriding the property to make it public, and it didn't really test anything. That was per @msonnabaum's request.

Why is the getter necessary, couldn't we go with the controller type only?

This is internal-only; it just ensures that core uses the custom getters the same as anyone else, in case they are overridden in some way.

Agreed, everything returning a class should have a Class suffix, .e.g. getControllerClass().

@msonnabaum and @dawehner agreed with you in IRC, and @yched said this would address his concerns above. So I went ahead and did that as part of this patch, as we were touching 90% of those lines anyway.

Inconsistency:
- the generic hasControllerClass() includes a class_exists() check.
- the generic getControllerClass() calls the above and thus includes the class_exists() check
- the specific getXClass() methods call getControllerClass() and thus include the class_exists() check
- but the specific hasXClass() methods are stricly isset() based, thus without the class_exists() check

1) We try really hard to not use concrete classes for things like this, it's a shame setName() isn't on any interface at all :(
I removed the @todo for now though.

2) As I said in #31, "This is internal-only; it just ensures that core uses the custom getters the same as anyone else, in case they are overridden in some way."
But good point on the variable name, changed it.

3) As @Berdir said in #33, that was originally done on purpose, leaving it here.

This is a regression. It's always a good idea to wrap access to properties like this in methods, especially in this case where the class has no dedicated factory and provides no default when the setter isn't used.

So I went ahead and did that as part of this patch, as we were touching 90% of those lines anyway.

Awesome!

This is internal-only; it just ensures that core uses the custom getters the same as anyone else, in case they are overridden in some way.

I see. It's quite ugly, but when it's internal only it shouldn't be a big deal.
Looking up the patch it appears to be on the interface also though, where it shouldn't be then. -> setting back to needs work.

Why do those methods not follow the get-pattern? I've seen that's not something new, but when we introduce a new base class it should follow current best practices. So, any reasons for not going with the pattern here?

I think you misunderstood me. EntityType::getStorageClass is a public method and should stay on the interface. I meant that the $controller_class_getter parameter of EntityManager::getController is internal.

We use getters for properties of the object itself, not for wrappers of services. See ControllerBase for example.

I think you misunderstood me. EntityType::getStorageClass is a public method and should stay on the interface. I meant that the $controller_class_getter parameter of EntityManager::getController is internal.

yes, I was talking about that as well. So if it is internal, it shouldn't be on the interface, right? But the patch adds it to EntityManagerInterface:

This is really helpful debugging information and I'm hard-pressed to think of a reason why you would NOT want it. (In fact, I have some DX issue open about the utter inability to debug a plugin-related error for exactly this reason.) It's weird to a) have a parameter for this, b) have it default to FALSE, and c) not do this on all plugins, not just this one. Tim's opening up a follow-up issue to discuss this pattern and its applicability across all plugin types (he did originally try to do this everywhere, but ran into issues like if ($this->getDefinition('invalid')...)

Here again we're doing weird things here that we don't in other plugins. I asked Tim more about this and he said that in #2005716: Promote EntityType to a domain object we added getters for these class names and so it makes sense to use them, vs. calling $entity_type->getControllerClass('storage') like everyone else does. Not sure I get that. Coming back to it in a bit.

Ok, talked through with Tim. Basically, the crux of the issue is that config translation introduced its own custom info hook and started monkeying with entity_info properties there, like list_controller. It did so because it couldn't call getController since that wasn't public (now fixed), and added setter methods to mirror the getter methods added in the previous issue, so that it can use those instead. Fixing this accessor issue for config translation hopefully helps fix it for anyone else in contrib who has to pull similar shenanigans.

This is a pre-existing condition, but it'd be great to understand *why* we are special-casing those particular things. A small follow-up for that would be nice.

+++ /dev/null@@ -1,42 +0,0 @@- public function testMethods() {

I love that we replaced like 7 assertions with 1200 lines of full test coverage. :) NICE! There's even a few 'banana' references in there, awww. ;)

OK, I can't find anything commit-blocking here. I'm not 100% sure I grok every line of this patch, but definitely get the gist and it seems solid.

Committed and pushed to 8.x. Thanks!

I don't *think* we need a change notice for this one, as it's mostly internal refactoring, but it wouldn't hurt to look over the various entity documentation to ensure something doesn't need to get updated.