Currently, we use entity_get_info() to get the definition of each controller and call new $class() with each one. The point of the plugin system was to be able to not just get information about plugins, but create them as well.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

Status: Active » Needs review
FileSize
8.13 KB

Here's a first attempt at this.

The factory is needed because of the different constructors.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
16.74 KB

Okay, here's more what I had in mind.

Though our usage EntityStorageControllerInterface::create() is really stupid. Half of the overrides alter $values before calling parent::create(), the other set data directly on the object afterward. Ideally I'd think we'd have one or the other, or override __construct().

attiks’s picture

short question:

+++ b/core/lib/Drupal/Core/Entity/EntityFactory.phpundefined
@@ -0,0 +1,65 @@
+    // Operation based plugins have a different constructor.
+    if (!empty($configuration['operation'])) {

this can use $plugin_class instead of $configuration['operation']

tim.plunkett’s picture

It's currently only used for form_controller_class, which is explicitly called an 'operation'.
I also think that if we have any other array-based class definitions like that, they'd also be called operations.

However, that should say "Operation-based controllers have a different constructor.", I'll fix it in a re-roll. Leaving CNR for more reviews for now.

Status: Needs review » Needs work

The last submitted patch, drupal-1831264-3.patch, failed testing.

yched’s picture

That's a skewed use of the Plugin API. Typically,
- a PluginManager class deals with one single plugin type (e.g. "image effects"
- the implementation definitions are held by the classes providing the implementation (e.g. the "resize" effect class).

Here,
- the same Manager class deals with 4 different plugin types (storage, list, render & form controllers)
- the definitions are not held by implementation classes (i.e a node is *not* a plugin, contrary to what I erroneously wrote in #1763974-187: Convert entity type info into plugins)
- the definitions listed by the discovery are not plugin definitions, they define something else which includes (in addition to other properties not at all related to the notion of plugins) a combination of plugin implementation classes for the 4 plugin types above.

I guess this off-the-track use of the plugin API might be ok if plugin API people are fine with it themselves, but this should at least be documented at the top EntityManager class ?

The one drawback I see to this is that we know that we want contrib to be able to provide additional controller types (if views was still in contrib, it would still need a views controller, right ?), and the current patch hardcodes the list of controllers types EntityManager can instantiate.

+++ b/core/lib/Drupal/Core/Entity/EntityFactory.php
@@ -0,0 +1,65 @@
+class EntityFactory implements FactoryInterface {

This factory does not create Entities. For clarity, can we name this EntityControllerFactory ?

+++ b/core/lib/Drupal/Core/Entity/EntityManager.php
@@ -290,4 +290,61 @@ protected function processDefinition(&$definition, $plugin_id) {
+  public function createStorageInstance($plugin_id) {
+    return $this->factory->createInstance($plugin_id, array('class_type' => 'controller_class', 'data' => NULL));
+  }

Usually in PluginManager classes the wrapper around $this->factory->createInstance() is getInstance().
For consistency, I'd say this method should thus be getStorageInstance (and same for other controller types)

This way,
- those get**Instance() methods could take care of statically persisting the instantiated controllers within the Manager object
- when #1764278: Run PluginManagerBase::processDefinition() in a ProcessDecorator and #1764232: CacheDecorator provides no way to clear cached definitions are fixed, entity_get_info() can remove its own caching in favor of CacheDecorator in EntityManager,
- then EntityManager::clearCachedDefinitions() can clear persisted controllers in addition to calling CacheDecorator::clearCachedDefinition.

And this fixes #1832418: Possibility of stale cache in EntityManager::$controllers : instanciated controllers all get a static cache, cleaned at the same time that the underlying entity_info.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
1.25 KB
16.96 KB

Re: EntityControllerFactory, hopefully it will also be used to create Entity type instances.

I'm not sure what you mean about getInstance(). Can you elaborate? We're very much creating instances.

I think adding static caching here should be done purposefully with a reason in a follow-up.

This fixes the fail and the comment.

tim.plunkett’s picture

yched’s picture

Re: EntityControllerFactory, hopefully it will also be used to create Entity type instances

Well, currently it isn't :-) Also, if EntityManager was used to create actual entities at some point, then I guess this wouldn't use the same factory than to create controllers, would it ?

I also think we should stop having "EntityManager extends PluginManagerBase". We're not using plugins, we're using a discovery with a wrapper manager. That's perfectly fine, but that has nothing to do with PluginManagerBase - or PluginManagerInterface.

fubhy’s picture

Assigned: tim.plunkett » fubhy

Here is a conversation I just had with @timplunkett on IRC:

<fubhy> I am not sold on the idea of having methods for each controller yet because that leaves us in an awkward position for new, contrib added, custom controllers
<fubhy> that's what would've been easy to solve with registering the controllers one by one
<fubhy> or through a factory per type or something
<fubhy> but I do agree with the point made that it's pretty bad to have the definition duplicated (once on the entity info and once in the DIC)
<fubhy> so what about just putting it in the DIC and separating controllers from the entity info?
<fubhy> timplunkett ^
<Crell> fubhy: Not if you can automate replicating it into the DIC.  Although Bundles are so limited that may not be possible.
<timplunkett> fubhy: that's because of the constructors
<timplunkett> fubhy: note that you don't NEED those methods
<timplunkett> fubhy: they just improve the DX
<yched> fubhy: EntityManger->getInstance('node', array('type' => 'form'));
<fubhy> yched, yes but that is totally weird from a DX perspective
<timplunkett> fubhy: createInstance($plugin_id, array('class_type' => 'my_custom_whatever', 'data' => $something_i_need));
<fubhy> yes, so we will have the wrapper methods for building the Core controllers and will, once again, end up with custom wrapper _functions_ for custom controller
<timplunkett> fubhy: what's the other option, have crappy DX for everything? :)
<timplunkett> fubhy: or, subclass EntityManager and swap it out of the DIC :)
<fubhy> timplunkett, I would have to drink a few beer to come up with a conclusion for that :P
<fubhy> timplunkett, riiiiight... :)
<fubhy> timplunkett, we could still register separate services for each controller type
<fubhy> without actually registering every controller _instance_
<timplunkett> fubhy: sure
<timplunkett> fubhy: if you want to take over that issue and try your hand at that, feel free! that sounds like an interesting approach

So, yeah.. I will give that "hybrid" solution a try.

tim.plunkett’s picture

#9: drupal-1831264-9.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, drupal-1831264-9.patch, failed testing.

effulgentsia’s picture

I agree with the goal and title of this issue, but not with the use of create*Instance() methods to achieve it. I'd rather add a method like getController() (or, after #1807042: Reorganizie entity storage/list/form/render controller annotation keys, getHandler()), and make it work for all controller/handler types by figuring out how to generalize the constructor argument differences.

And in a separate issue, I'd like to give EntityManager a factory that makes createInstance() do what entity_create() does: create the entities themselves (i.e., the instances of the plugin class).

Unless someone beats me to it or shoots the idea down, I'll roll a patch for this after #1807042: Reorganizie entity storage/list/form/render controller annotation keys is done.

effulgentsia’s picture

in a separate issue, I'd like to give EntityManager a factory that makes createInstance() do what entity_create() does

#1867228: Make EntityTypeManager provide an entity factory

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
21.44 KB

Not much movement, so I revisited this.
It needed a reroll.
Just some name changes, mostly.

// Old
$node = entity_create('node', $values);
// New
$node = $this->manager->storage('node')->create($values);

Still open to fubhy's idea.

tim.plunkett’s picture

FileSize
46.45 KB

One major benefit of not using the wrappers is that it exposes some key places we could be injecting the EntityManager.

Status: Needs review » Needs work

The last submitted patch, entity-1831264-17.patch, failed testing.

tim.plunkett’s picture

Assigned: fubhy » Unassigned
Status: Needs work » Needs review
FileSize
44.27 KB

After talking to EclipseGC about this, we decided on removing the EntityFactory (sine that should really be used as a part of #1867228: Make EntityTypeManager provide an entity factory) and just having methods direct on the EntityManager.

Also, EntityManager::getControllerClass() will allow contrib to use any of its own controllers, since it forces you to do the one line instantiation. The syntax is much less confusing.

We also now have cheap and easy static caching of controllers. Before we had drupal_static around storage and access controllers, which this includes, but an easy pattern to add it to the others in the future if we see fit.

EclipseGc’s picture

I've not read the whole patch, but reading the EntityManager class gives me great confidence that this is moving in the right direction. My one caveat is

$this->discovery->getDefinition($entity_type)

Should be:

$this->getDefinition($entity_type)

The manager implements DiscoveryInterface, and proxies to this giving you extra flexibility and is the current standard we're pushing plugin types to.

Eclipse

tim.plunkett’s picture

FileSize
44.74 KB

Yep, makes sense, it only had that because it was previously in a Factory

aspilicious’s picture

Status: Needs review » Needs work
+++ b/core/modules/config/tests/config_test/config_test.moduleundefined
@@ -135,4 +136,4 @@ function config_test_cache_flush() {
\ No newline at end of file

needs newline

+++ b/core/modules/node/lib/Drupal/node/Tests/SummaryLengthTest.phpundefined
@@ -35,8 +35,9 @@ function testSummaryLength() {
+    $render_controller = $this->container->get('plugin.manager.entity')->render('node');

Now it feels weird to call "render" and get a render controller.

tim.plunkett’s picture

I'll add the newline in the next reroll, or if it comes back green.

$render_controller = $this->container->get('plugin.manager.entity')->renderer('node');
?

The goal was to keep these short and not redundant. Also I'm not too worried about this, if you're dealing with controllers and you call render() without another method, you'll figure it out pretty quickly. Also it has docs and is located in the middle of 5 other similarly named methods.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
3.6 KB
44.57 KB

Also not only is renderer a word, but it seems appropriate.

Fixed that and the newline, and it passed tests!

EclipseGc’s picture

Status: Needs review » Reviewed & tested by the community

assuming this passes tests, RTBC

Eclipse

yched’s picture

Yay at the code, but really not convinced by the method names...

- storage() (gives you the storage controller)
- renderer() (render controller)
- listing() (list controller)
- form() (form controller)
- access() (access controller)

$manager->access(), $manager->form()... - feels extra weird.
So, er, $manager->form()->form() ? :-)

I'm all for brevity when it is not misleading, but in this case we shouldn't spare the FooBarController() pattern IMO.

tim.plunkett’s picture

Well we can do fooController() if that's what we need to make this less ambiguous...
Going to leave at RTBC for now.

sun’s picture

Status: Reviewed & tested by the community » Needs work

I agree with @yched.

We generally shouldn't continue the anti-pattern of badly named method names in HEAD: E.g., foo() is supposed to do something, not get something.

->getAccessController()
->getFormController()
->getListController()
->getRenderController()
etc.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
34.97 KB
45.31 KB

Whew, I was feeling pretty uncomfortable about the short names I picked, so I'm grateful for feedback there!

tim.plunkett’s picture

FileSize
36.93 KB

Wrong interdiff. Very similar in size!

tstoeckler’s picture

+++ b/core/lib/Drupal/Core/Entity/EntityManager.php
@@ -303,4 +310,123 @@ public function processDefinition(&$definition, $plugin_id) {
+      throw new PluginException(sprintf('The entity (%s) did not specify a controller class.', $entity_type));
...
+        throw new \InvalidArgumentException(sprintf("Missing controller for '%s: %s'", $entity_type, $nested));

If I get this Exception in my face, I would really like to know which controller type this failed on, pretty plz :-)
This also got me thinking if we might want to fail a little less hard on this. Imagine there were a views data controller :-), and views wants to find out which entities have declared such a controller. Currently it would have to scan the definition itself, but I think it would be cool to just do

if ($controller_class = $manager->getControllerClass($entity_type, 'views')) {
...
}

On the other hand

$definition = $manager->getDefinition($entity_type);
if (!empty($definition['views controller class'])) {
...
}

isn't really that much more code, so I guess that's a wash...

+++ b/core/lib/Drupal/Core/Entity/EntityManager.php
@@ -303,4 +310,123 @@ public function processDefinition(&$definition, $plugin_id) {
+    $plugin_class = $plugin_definition[$controller_type];
...
+      $plugin_class = $plugin_class[$nested];
...
+      throw new PluginException(sprintf('Entity (%s) instance class "%s" does not exist.', $entity_type, $plugin_class));
...
+    return $plugin_class;

I think "controller class" makes more sense than "plugin class" and "instance class", IMHO.

+++ b/core/lib/Drupal/Core/Entity/EntityManager.php
@@ -303,4 +310,123 @@ public function processDefinition(&$definition, $plugin_id) {
+    if ($nested) {

Edge-case, but this disallows keys such as 0 and '0'. I guess a isset() wouldn't hurt, right?

+++ b/core/modules/config/tests/config_test/config_test.module
@@ -135,4 +136,4 @@ function config_test_cache_flush() {
-}
+}
\ No newline at end of file

:-(

fago’s picture

->getAccessController()
->getFormController()
->getListController()
->getRenderController()

Yes, let's use this descriptive method names. Also +1 on the general approach. While it does not deal with controllers registered by modules, it's a great step forward. I guess we'd could just go with registering a factory for module provided controllers in the DIC.

tim.plunkett’s picture

FileSize
2.23 KB
126.55 KB

Good suggestions in #31, this should address them.
#32 was already done.

EntityManager::getControllerClass() will be enough for contrib to instantiate their own controllers.

tim.plunkett’s picture

FileSize
45.3 KB

Whoops, that included the entirety of #1779026: Convert Text Formats to Configuration System. Interdiff was right...

EclipseGc’s picture

Status: Needs review » Reviewed & tested by the community

if this comes back green, still totally RTBC imo

fago’s picture

EntityManager::getControllerClass()

Yes, but reading the class out isn't really the problem - it's handling instantiating, maintaining instances and returning them in a way auto-completion works.

One issue that this does not address is having different dependencies for controller, or just letting the objects inject into the manager. We might wanna built (at least optional) support for having controller instances in the DIC, or factories for them. But howsoever, we need to move on in steps and this is a good first step for moving on. Thus, I second the RTBC.

tim.plunkett’s picture

FileSize
45.96 KB

Nothing in #36 was possible in HEAD anyway, I agree its a good topic for a follow-up.

To minimize this breaking as stuff is added to HEAD, I've NOT deleted the entity_*_controller() wrappers, and marked them deprecated instead.

That and a reroll.

EclipseGc’s picture

assuming this passes tests, I'm still totally RTBC on this.

Eclipse

andypost’s picture

+1
Also this patch should help to move Menu entity to it's home from system module!

sun’s picture

The new getter method names look very nice to me. Nice work, @tim.plunkett!

fubhy’s picture

I guess this is the best we can do right now. I still believe that we mostly just moved stuff around instead of actually solving the 'problem' but as Tim already said in regard to #36 we can't really do more right now in HEAD anyways. It's still a good cleanup.

RTBC+1

webchick’s picture

Status: Reviewed & tested by the community » Fixed

So I'll fully admit that I have no idea what this is doing, and it seems to make the code a heck of a lot uglier, but tim and eclipse say they need this, and it has +1s from all the right folks. :P So...

Committed and pushed to 8.x. Thanks! Hope I don't live to regret this. ;)

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.