Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Requirements
- Entity Controllers need to be services so they can get things like database connection or cache backends/bins injected (e.g. storage controller).
- The entity manager should be able to get instances of the controllers, without getting them injected, as we want them to be lazy loaded.
Note: This requirement differs from the one outlined in #1863816: Allow plugins to have services injected because we don’t really want to inject the controllers into our plugin instances but instead register them with the plugin manager. In return, the plugin manager would get forwarded to the plugin instances upon instantiation so that we can easily access it in places like $entity->save(), $entity->delete() or $entity->view().
Proposal
- Make controllers services, so they define their dependencies. The database entity storage controller, for example would get a database connection and a cache backend injected, whilst the config entity storage controller would get the config storage injected.
- In order to let the entity manager lazy load controllers on the fly it should get a controller factory, which is itself ContainerAware. This allows the controller factory to load the required services from the container on demand, and keeping the memory footprint as low as possible.
- We would then register our entity controller services with that controller factory... Possibly through a compiler pass or other means.
Developer Experience
- This proposal requires controllers to be defined as services. Unless we want to come up with a syntax to define services including dependencies, etc. in the annotations of the controller class we will have to register our controllers manually (e.g. NodeBundle).
- In the long run, however, we might be able to use this to decouple the controller definitions from the entity class annotations and instead simply rely on unique service names to register controllers for entity types. Simply put, we could turn the coupling of entity class and entity controllers upside down.
Related Issues
#1981516: Refactor ConfigStorageController to use entity query
Comment | File | Size | Author |
---|---|---|---|
#98 | 1909418-98.patch | 47.09 KB | damiankloip |
#96 | 1909418-95.patch | 47.08 KB | damiankloip |
#94 | 1909418-94.patch | 767 bytes | damiankloip |
#94 | interdiff-1909418-94.txt | 767 bytes | damiankloip |
#92 | 1909418-92.patch | 47.23 KB | damiankloip |
Comments
Comment #1
fubhy CreditAttribution: fubhy commentedShort addition: If we keep the entity controller definitions in the annotations whilst trying to register them with a possible controller factory that would mean we would be reading from $entity_manager->getDefinitions() in a compiler pass to register controller instances with the factory which in return should actually get injected into the $entity_manager itself. Umh?! How much of a wtf is that? :)
Comment #2
tstoecklerInstead of registering the controllers in the DIC we can use the approach that we have for route controllers, and hopefully soon for plugins as well: a static create() factory on the controller itself. The entity manager would then be controller-aware and simply do
$class::create($this->controller, $foo, $bar)
instead ofnew $class($foo, $bar)
. I think that is the cleanest approach and is (hopefully) emerging as sort of a standard.Comment #3
dawehnerI totally agree. Once we have figured out the plugin manager one, we would just be able to use the same kind of container object to create those instances or use Drupal::() that's not clear.
Comment #4
larowlangoing to see if this one isn't beyond me
Comment #5
Crell CreditAttribution: Crell commentedClarifying title.
The entity controllers should very much be "true" services, and either registered in the DIC or spawned from it, meaning they can take everything from straight up constructor injection. No statics needed. Entity Managers should NOT be container aware.
Comment #6
Berdir@Crell: Keep in mind that we're talking about a lot of services here. I'm counting 45 entity types right now in Core, including a lot of test entity types and ~5 different controller classes (config entities usually don't use that many). We have to register them explicitly for every entity type as they get the type and entity info as constructor argument.
Comment #7
larowlanYeah #5 is beyond me, #3 made sense
Comment #8
dawehnerCan you describe your thoughts a bit more? I think your point could be, that entity controllers are logic objects, which deal with data directly, like different kind of cache backends.
Comment #9
Crell CreditAttribution: Crell commentedBerdir: 45 is not a lot of services. The container can scale to about 2000 entries without performance degradation on a stock APC configuration. (Higher than that, up your APC file size limit and you can scale as far as you want.)
My underlying point is that I am *not* in favor of using the create() static for them. That makes sense for things that we're going to have hundreds or thousands of: Plugins, controllers, etc. Controllers should be glue-code anyway, not carrying for-reals logic that we'd want to unit test.
Entity storage and management is not such a use case. They're carrying real logic that we absolutely do want to unit test, with PHPUnit, with mock objects. (That's what I mean by "real services".) Making things container aware undermines that.
Comment #10
Berdir@Crell 45 entity types, not controllers :) Assuming we have ~3 controllers per entity type, that's 120. And not everything has been converted to (config) entities yet, e.g. node types and fields/instances. If we split storage controller (generic + type specific one), that means +1 for all.
And that's just core (probably half of those test types, though). Add a site with commerce, rules, search api and a few additional modules like that to the mix and it starts to get interesting :)
Agreed, but why does a create() method prevent that? We still have a constructor with explicit arguments.
Comment #11
Crell CreditAttribution: Crell commentedWhere and how do you specify the class on which to call create()?
Comment #12
BerdirIn the @Plugin annotation, see for example http://api.drupal.org/api/drupal/core%21modules%21node%21lib%21Drupal%21... (did you know that we now have 9 files/classes called Node(.php) ;)) or http://api.drupal.org/api/drupal/core%21modules%21user%21lib%21Drupal%21...
And they are created in http://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Entity%21E...
Comment #13
larowlanSo here's the ::create (factory) approach.
We have to use createInstance in this case because DatabaseStorageController::create already exists.
This patch adds Drupal\Core\Entity\EntityHandlerInterface as I thought EntityControllerInterface was misleading (they're not controllers in true sense of the word) but the name is just semantics.
Any entity controller (list, form, storage, translation, render) can implement that interface and then add a static createInstance method.
This receives the container as the first argument and then the other constructor arguments as a keyed array.
This patch changes the DatabaseStorageController and sub-classes to implement this interface and uses $this->database instead of the various db_select|update|insert|transaction functions as a POC.
Comment #15
BerdirMissing ->
Comment #16
larowlanFind and replace fail.
Comment #18
larowlanAny reason why menu link rebuild doesn't use the entity manager?
Comment #19
dawehnerThe problem is that menu_link module is not enabled, which means that the entity manager doesn't have the menu link entity annotation scanned, so the storage controller is not available.
Just some minor fixes.
Comment #21
Crell CreditAttribution: Crell commentedMinor nit: I think we're using lowercase "array" for type hinting.
This makes the Entity system tightly coupled to the Symfony DIC. Are we really sure we want that? Really? We want our storage system to be non-functional without the DIC?
That half-defeats the purpose of using DI. We can still inject everything else, sure, but this makes EntityManager dependent on having the DIC, even for testing purposes. That is, unit testing it just got 10x harder.
Are we really, really sure there's no other option?
This is fine for now, but if we're going to do this we should just damn well do it. No option here. Entity Handlers must be injected.
Comment #22
larowlanI have an idea about how to inject specific services into the controllers without their create method requiring the container. Only the entity manager will need it then.
Comment #23
larowlanAgreed, this is just a proof of concept.
Comment #24
larowlanAfter reading again, realised Crell is talking about the manager, so my idea is no better.
Not much of an argument, but I type hinted the interface.
Comment #25
larowlanShould fix test fails.
Discussed issue with injecting container interface into the entity manager with @dawehner on irc.
Quoting dawehner 'thought if we do unit tests we should always access/contstruct the entity controllers directly, so this shouldn't be a major problem?'
I tend to agree but let's see what others think.
Comment #26
BerdirWhy make the create method required? It doesn't prevent controllers from not injecting their dependencies and some might actually not need any dependencies (test implementations if no others).
I think @EclipseGc and also others voted pretty strongly for not requiring such a method for plugins, so why do it here?
Patch looks otherwise quite fine to me, just wondering how far we want to go. Do we want to, as an example, convert the database storage controller to be fully injected (as much as possible) as an example or do that in follow-ups as it might get quite big, there are a lot of dependencies in there I think.
Comment #27
tim.plunkettControllerResolver::createController() uses a similar pattern with ControllerInterface, but that's not temporary, those checks will remain.
This is pretty crappy DX, since we end up with a magical array. What about instead of using a constructor for the pre-existing values, each controller type provides setters? It would help codify what each one needs. So EntityStorageControllerInterface would have a setEntityType() method, that EntityManager::getStorageController() would call.
Comment #28
larowlanDiscussed this on irc, consensus is to add entity_type argument to form controller constructor.
Then we can add entity type argument to the createInstance method in the new interface.
Then we need setters on the list controller to set the storage controller and one on the form controller to set the operation.
Comment #29
msonnabaum CreditAttribution: msonnabaum commentedIf you'd always call setEntityType, why not just add type to the factory method signature? If a controller object isn't complete until that's set, it should probably be in the constructor.
Comment #30
msonnabaum CreditAttribution: msonnabaum commentedOops, should have refreshed before I posted. Agree with #28 :)
Comment #31
larowlanSo this adds the three arguments received by each of the constructors as parameters to the createInstance method on the interface. Those that aren't always required have defaults of NULL.
Each controller's createInstance method can pass on the required arguments to the constructor as required.
Entity type is required but the form controller createInstance would just ignore that argument.
Also renamed to EntityControllerInterface, there is another issue tracking renaming all of the entity controllers to handlers (they're not controllers in the true sense of the word).
Comment #32
tim.plunkettAFAIK, if they are
= NULL
, putting them in the interface doesn't matter, and they can be left off all of the ones that don't need it.So it could just be
public static function createInstance(ContainerInterface $container, $entity_type);
in the interface, and everyone else doespublic static function createInstance(ContainerInterface $container, $entity_type, $my_thing = NULL) {
where needed.Comment #33
tim.plunkettWorking on a patch with my suggestions, and some more conversions to get a feel for it.
Comment #34
tim.plunkettSo, unlike the ControllerInterface classes, where they're generally one-off and pretty slim, there is a good deal of extending here, which makes the __construct() a little brittle. But I still think this is a reasonable approach.
Comment #36
larowlanNeed to add the third arg to the new MenuLinkStorageController() call in _menu_navigation_links_rebuild() if someone beats me to it, if not - will be tomorrow.
Comment #37
larowlanFingers crossed.
Also re-roll for the service container yml changes.
Comment #39
larowlan#37: entity-controllers-di-1909418.37.patch queued for re-testing.
Comment #40
msonnabaum CreditAttribution: msonnabaum commentedI'm happy with #37.
Comment #41
tim.plunkettWas trying to do
Now that hunk is
Comment #42
larowlan#41: entity-controller-1909418-41.patch queued for re-testing.
Comment #43
larowlanBump
Comment #45
kim.pepperRe-rolled on latest head, fixed a couple of merge conflicts.
Comment #46
tim.plunkett#45: 1909418-entity-controller-45.patch queued for re-testing.
Comment #47
msonnabaum CreditAttribution: msonnabaum commentedStill looks good to me.
Comment #48
tim.plunkett#45: 1909418-entity-controller-45.patch queued for re-testing.
Comment #50
Sylvain Lecoy CreditAttribution: Sylvain Lecoy commentedI am not sure we need those createInstance static functions in controllers classes, since anyway the container will manage instances for us. The EntityControllerInterface is for me not needed at all, and it adds complexity to controllers.
Secondly, as stated in Avoid your code becoming dependent on the container, it is not a good practice to let objects configure themselves through a container instance.
Instead, we should configure the container to inject dependencies:
This will produces a semantical identical code in the dumped container than the static createInstance function, without the need of a new Interface which the only contract is to define a factory method, thus looks not correct for me.
Then we can refactor some of the EntityManager functions, such as:
For five methods, we are copy/pasting blatant logic which should be handled by the container itself. It is bad as it not fun to write nor to maintain, it is pure wiring glue code that someone has already written and tested before us, and it is passing a ContainerInterface implementation to our objects. While it is OK for the manager to have a container for DX shorthand, it is not for managed objects. A part from this disagrement, the other parts of the patch are for me really good, objects starts being awesome in Drupal :-)
Oviously we should first agree on whether we want to add these controllers definitions in container. But I already saw a topic on making these controllers services, so the idea is there. Secondly, the registration mechanism would be then delegated to pre-dump phase of the container, removing some discovery overhead at run-time, but assumes refactoring.
The manager would be the top-level class webchick is talking about in #1875086-48: Improve DX of drupal_container()->get(), handling the injector crap, with nice @return typehint for improved DX.
Should I work on a patch ?
Comment #51
tim.plunkettNope, that sounds like a completely different approach to how we're done all other controller injection. I'd suggest opening a new issue.
This is just s/_controller_class//, thanks to #1807042: Reorganizie entity storage/list/form/render controller annotation keys
Comment #52
Sylvain Lecoy CreditAttribution: Sylvain Lecoy commentedI thought this issue was asking architectural review, that's why I proposed an alternative approach.
Comment #53
tim.plunkettAh, my apologies. That tag predates the usage of this pattern in other places, I think we're pretty happy with it.
Comment #54
dawehnerThis comment seems wrong.
Can we describe the full variable?
Needs documentation. We ad new parameters, so we need new @param
need docs.
Docs
Can be {@inheritdoc}
Comment #55
Sylvain Lecoy CreditAttribution: Sylvain Lecoy commentedIs this a better approach to resolve at runtime the controller to instanciate or to configure the container so it is resolved at "compile-time" and comes for no cost at runtime ? I don't think...
Also, from my limited experience, successful software do not add a lot of interfaces, base class to extends, and so one... People like simplicity. That's why I don't think its a good idea to add another interface, plus there is already ContainerAwareInterface which is existing so why not reusing it ? Not invented here ? :(-
Eventually, I don't see the point creating a new issue when the first requirement is:
If by service you mean contained and managed by the Symfony container then it is likely what I believe Crell thought it was initially and the patch proposed does not respect this requirement. If by service you mean a completely another story then I'll create a new issue/follow-up as suggested.
I just need to know where to register these services prior to "compile-time": in an EntityBundle ? in the CoreBundle ?
Comment #56
Sylvain Lecoy CreditAttribution: Sylvain Lecoy commentedFor more information on how I see this - I may be wrong, please tell me - check the branch 7.x of the Doctrine ORM project.
Here you'll have a doctrine.inject.inc file which leverage the ContainerBuilder to configures its dependencies, and a service locator function called doctrine().
This service locator have obviously a hard dependency on the DI component but it is good as a shortcut for programmers. Advanced programmers who needs the EntityManager as dependency of their object might not use this locator function but instead wire their object to the 'doctrine.orm.manager' reference directly.
By analogy, we can see the EntityManager as an EntityControllerLocator, which will provide typehinted controller through the container (see my comment in #50). I think caching these services in the container makes a lot of sense, like Crell already stated, container has been tested and can support more than 2000 services without any problem.
Comment #57
tim.plunkettAddresses #54.
Comment #58
Sylvain Lecoy CreditAttribution: Sylvain Lecoy commentedVery first requirement is still not met (Entity Controllers need to be services).
Either change the issue summary or implement differently the component instantiation.
Comment #59
Sylvain Lecoy CreditAttribution: Sylvain Lecoy commentedNever mind I've created a task, 50% of the requirement in this issue, the other 50% in the issue bellow: #1980310: Entity Controller/Handler as a service. That's how we like drupal I guess :-)
Comment #60
dawehnerI'm sorry :( Some of that has been part of my previous review
Needs some documentation :(
@inheritdoc
Do we really want to list all kind of entity controllers here? Wouldn't that be possible extended by contrib, so that's never 100% accurate
Documentation and code does not match.
Needs docs.
Relative to DatabaseStorageController, this adds new paramaters, so we maybe should add documentation as well.
docs.
Shouldn't we also use $this->database->query instead of db_query?
docs
@inheritdoc
Comment #61
tim.plunkettWhoops, my interdiff in #57 wasn't actually applied to the patch in #57.
So this interdiff is against the patch in #57, and includes the last interdiff. Sorry for the confusion.
Comment #63
tim.plunkettException: Drupal\field\Plugin\Core\Entity\FieldInstance::serialize() must return a string or NULL in serialize() (line 99 of /Users/tim/www/d8/core/lib/Drupal/Core/KeyValueStore/DatabaseStorageExpirable.php).
Um, what?
Comment #64
dawehnerSo again, that's another issue where something is in the form state which stores some kind of plugin manager (in this case the entity manager)
which then fails completely.
This time it's the DisplayOverview object, which always had the Entity Manager injected, which now fails. Maybe we have to go with the \Drupal:: approach as well?
Comment #65
dawehnerWhat about this piece of code, to decouple this UI code from the storage controllers? It doesn't work yet, but it's just a general idea.
Comment #66
fagoThe approach taken looks good to me, however the issue summary seems to say something else. Does it need an update?
Comment #67
dawehnerJust see the errors. In general we would have to wait until the db connection is serialization.
Comment #69
tim.plunkettI like the idea of only storing the objects when needed, but in general it might be more useful to contrib to have the service available, especially for subclasses.
Straight reroll of #65 after #1913618: Convert EntityFormControllerInterface to extend FormInterface
Comment #71
tim.plunkettNeeded changes for #1946404: Convert forms in field_ui.admin.inc to the new form interface.
Fixing the damn entity_type thing since that has broken my patches enough times.
Comment #73
tim.plunkettSloppy reroll on my part.
Comment #74
dawehnerIt feels wrong to name it "injected into all controllers", but it's rather passed to the controller factory.
Can we pull the request in the form method and set it to the form state? Paris suggested that to be the proper way instead of storing on the controller.
Comment #75
tim.plunkett#73: drupal-1909418-73.patch queued for re-testing.
Comment #77
yched CreditAttribution: yched commented#73 includes field_ui snippets that look like a leak from #1982138: Clean out field_ui.admin.inc ?
Comment #78
damiankloip CreditAttribution: damiankloip commentedRerolled and made those changes, I'm still nto sure about the request, so I've moved it into the form method instead.
Trying to move this along so #1981516: Refactor ConfigStorageController to use entity query can get in. Sorry Tim, I know this is assigned to you but it's been a few days.
Comment #80
attiks CreditAttribution: attiks commented#78: 1909418-78.patch queued for re-testing.
Comment #82
damiankloip CreditAttribution: damiankloip commentedComment #83
damiankloip CreditAttribution: damiankloip commentedJust talking to dawehner, we should/could also inject the entity info, then we remove the call to entity_get_info which is what we want anwyay.
Comment #84
damiankloip CreditAttribution: damiankloip commentedComment #86
dawehnerThere has been a couple of issues with failing adapations and a problem with serialization on the drupal kernel.
Comment #87
yched CreditAttribution: yched commentedDoesn't seem needed ? (Symfony's SerializerInterface is serialize() + *de*serialize() so it doesn't seem to be what is implemented here, but rather \Serializable)
Also - why do we suddenly have a need to serialize the kernel ?
Comment #88
dawehnerOh you are totally right I tried to go with the interface first but then I realized that overriding the other methods seems just better.
The form objects which build the form, are part of the form_state, which is serialized due to caching of forms. Previously we hadn't the container stored on the entity manger but now any kind of form controller, that stores the entity manager now has at some point the kernel. (as it's part of the container).
Comment #90
yched CreditAttribution: yched commentedSo it's: $form_state -> form controller -> entity manager -> container ( -> kernel).
And $form_state gets serialized.
That sounds exactly like the "reference chain turns to serialization hell" problem at its maximum ?
Serializing the container seems like a *really* bad idea :-/
Serializing the container sounds *really* bad :-/
Comment #91
dawehnerWell, I personally think that we should not store the entity manager itself but just the controllers we need, like for example the storage controller.
This would allow us to get rid of that problem.
Comment #92
damiankloip CreditAttribution: damiankloip commentedJust fixing the tests.
Comment #93
BerdirThat @todo seems unecessary, no need to store something somewhere?
$module_enable isn't used.
I guess there's not much more that can be asserted here?
This looks nice otherwise, as discussed, we will have to inject the entity manager, no way around that. But what I think we should do, not necessarly in this issue, is implement __sleep()/__wakeup() where we throw out stuff we don't want to serialize (controllers, module handler, container, kernel, ....) and on wakeup get it back from the global container. I really think testability/mocking and stuff doesn't matter anymore at that point. In fact, we should probably actually make sure that we do have the current global services after wakeup as we might otherwise use stale data (e.g. an outdated field definition cache, in case someone added a field while we filled out the node form..., or invoke a module hook that was disabled in the meantime)
Speaking of the module handler, what about all the hooks in there? No need to bother about field attach callbacks though, that's being worked on in a different issue.
Comment #94
damiankloip CreditAttribution: damiankloip commentedSo I think we can just remove this stuff?
Comment #96
damiankloip CreditAttribution: damiankloip commentedWith the actual new patch and not just interdiffs
Comment #98
damiankloip CreditAttribution: damiankloip commentedRerolled after the language patch just got in.
Comment #99
tim.plunkettWe'll need a follow-up for __sleep()/__wakeup() but this is ready to go and blocks a huge chunk of issues.
Comment #100
BerdirNote that this will at least logically conflict with #1893820: Manage entity field definitions in the entity manager, which accesses the entity manager from the storage controller and we'd have to inject that too, which in turn would make __sleep() stuff from #99 necessary here. I guess this is blocking more things, so probably has a higher priority.
Comment #101
tim.plunkettOpened #2004282: Injected dependencies and serialization hell
Comment #102
alexpottCommitted 7961e03 and pushed to 8.x. Thanks!
Comment #103
yched CreditAttribution: yched commentedCould we just add a @todo to remove those once #2004282: Injected dependencies and serialization hell is resolved ?
Trying to serialize the kernel is the sign of something terribly wrong, and IMO *should* break rather than silently succeed.
Comment #105
jibranSorry for the noise but i think this needs change notification.
Comment #106
dawehnerI can't even one for entity controllers itself.
Comment #107
dawehnerJust create one: https://drupal.org/node/2067565
Comment #108
andypost@dawehner thanx!
Comment #109
smiletrl CreditAttribution: smiletrl commentedprobably is not a consistent method of 'create' to inject dependency across core code:(
See alternative proposal at #2015535: Improve instantiation of entity classes and entity controllers.
Comment #110
dawehnerYeah let's fix the changenotice once your other issue is in. I totally agree.
Comment #112
xjmUntagging. Please remove the tag when the change notification task is completed.
Comment #112.0
xjmUpdated issue summary.