From #1783964-9: Allow entity types to provide menu items:

We really need to rename entity controllers. "Controller" is "callable that the Kernel calls", basically formerly "page callback". Like "bundle" that's an inherited term from Symfony so we cannot really change it.

Before

/**
 * @Plugin(
 *   id = "block",
 *   label = @Translation("Block"),
 *   module = "block",
 *   controller_class = "Drupal\block\BlockStorageController",
 *   access_controller_class = "Drupal\block\BlockAccessController",
 *   render_controller_class = "Drupal\block\BlockRenderController",
 *   list_controller_class = "Drupal\block\BlockListController",
 *   form_controller_class = {
 *     "default" = "Drupal\block\BlockFormController"
 *   }
 * )
 */

After

/**
 * @Plugin(
 *   id = "block",
 *   label = @Translation("Block"),
 *   module = "block",
 *   controllers = {
 *     "storage" = "Drupal\block\BlockStorageController",
 *     "access" = "Drupal\block\BlockAccessController",
 *     "render" = "Drupal\block\BlockRenderController",
 *     "list" = "Drupal\block\BlockListController",
 *     "form" = {
 *       "default" = "Drupal\block\BlockFormController"
 *     }
 *   }
 * )
 */
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

effulgentsia’s picture

FWIW, I think Java calls these things "servants": http://en.wikipedia.org/wiki/Design_pattern_Servant.

plach’s picture

What about Handlers? I think i'ts more intuitive.

sun’s picture

Issue tags: +Bikeshed, +symfony

I was a bit surprised by that comment and suggestion in the first place.

A "controller" virtually is a universal term, so I don't really get how and why we'd suddenly assume that "it means only one and this only one thing."

tim.plunkett’s picture

Issue tags: +VDC

Handlers are a Views term.

Let's call them context entity nouns.

Crell’s picture

plach’s picture

@sun:

Same for handler, which is a far less characterized term than controller, I'd say.
It's used also in the Field API, btw.

http://en.wikipedia.org/wiki/Handler#Computing
http://en.wikipedia.org/wiki/Controller

Also manager could be fine but we are using it for DI, right?

Crell’s picture

I don't think "Manager" has a fixed definition yet. :-) (yay, an unused noun!)

tim.plunkett’s picture

Plugin managers.

Crell’s picture

Issue tags: -VDC

So Plugin Managers manage plugins, Entity Managers manage Entities. Neither one has the overlap that "Controller" does, which has meaning outside of Drupal we're fighting against.

tim.plunkett’s picture

Issue tags: +VDC

So we'd have a 'entity list manager', 'entity storage manager', 'entity form manager', and soon 'entity render manager'?
That's not too awful, if we actually decide we want to even bother :)

plach’s picture

I'd still prefer:

'entity list handler', 'entity storage handler', 'entity form handler', 'entity render handler', 'entity translation handler'

but I can live with the manager terminology :)

sun’s picture

Handler linguistically makes more sense than Manager, given the purpose of these classes.

Crell’s picture

I am OK with handler if the Views folks are OK with it. It's less of a global conflict than controller.

dawehner’s picture

Views itself uses more and more the terminology of $handler_type plugins like views field, filter plugin
so i don't think that would really confuse people.

effulgentsia’s picture

Title: Rename entity storage/list/form/render controllers to something else » Rename entity storage/list/form/render controllers to handlers
Issue tags: -Bikeshed

+1 to handlers. I'll be optimistic and call the bikeshedding portion of this issue over :)

tim.plunkett’s picture

Status: Active » Needs review
FileSize
23 KB

Here's a patch.

In code, the storage controller is just 'controller class'. In some comments that is also true, in others it is 'storage controller'.
That was fine when it was the only controller, but now that we have list and form, do we want to specify?
And if so, is that a follow-up, or just something we should do once so that anyone tracking HEAD doesn't have to rename it twice?

A second idea, why not "list handler", "storage handler" instead of "list handler class"?

tim.plunkett’s picture

Note, this only changes the 'controller class' part to 'handler class'. Documentation and other places (entity_get_controller) still need changes.

tim.plunkett’s picture

We have consensus, which is what is important.
This patch already touches a lot of code, and renaming entity_*_controller would make it even more painful.

I'd postpone this, but since it's not major, there's no reason to.

I'm just not going to work on this until Decemeber 2nd. :)

Crell’s picture

For the record, I'm fine with letting this sit until Slush as long as Dratch doesn't object.

sun’s picture

Status: Needs review » Postponed

Agreed - this is super-typical clean-up material. :)

andypost’s picture

+++ b/core/includes/entity.api.phpundefined
@@ -149,8 +149,8 @@ function hook_entity_info() {
-      'controller class' => 'Drupal\node\NodeStorageController',
-      'form controller class' => array(
+      'handler class' => 'Drupal\node\NodeStorageController',
+      'form handler class' => array(

-handler class?
+storage class!

fago’s picture

Maybe "service" would be a good term to use? I assume that in future those controllers will live in the DIC anyway:
- entity.storage.TYPE
- entity.form.TYPE

..?

Thus, you define the class for the entity storage service, or the entity form service?

Agreed, that this should be handled post-feature freeze.

fubhy’s picture

effulgentsia’s picture

Status: Postponed » Needs work

Unpostponing, because there's a couple issues I'd like to get to related to this, like #1831264: Use the Entity manager to create new controller instances, that I'd rather just do once with the correct names.

"needs work" because #16 doesn't apply any more. I'm willing to reroll, but what do you folks think of:

handlers = {
  "storage" = "Drupal\node\NodeStorageHandler",
  "render" = "Drupal\node\NodeRenderHandler",
  "form" = {
    "default" = "Drupal\node\NodeFormHandler"
  }
}

Notice the word "class" doesn't appear anywhere above, both for brevity, and so that if in the future we want to support the values being either class names or service ids, then we can do so (it's not hard for PHP to find out if a string is a class name or service id).

tim.plunkett’s picture

+1 to #24

plach’s picture

Sounds reasonable to me too, but perhaps I'd add a " handler" suffix to make those keys more explicit.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
38.53 KB

Here's step 1: renaming the keys according to #24, but not the classes or entity.inc functions yet. Wanting a bot check on just this much.

perhaps I'd add a " handler" suffix to make those keys more explicit

My suggestion is for those keys to be grouped inside of a top-level 'handlers' key. Do you still think they warrant suffixes too?

plach’s picture

My suggestion is for those keys to be grouped inside of a top-level 'handlers' key. Do you still think they warrant suffixes too?

Nope, I missed that part, looks pretty good this way :)

effulgentsia’s picture

FileSize
196.56 KB

And here's one with all the classes renamed too.

tim.plunkett’s picture

FileSize
1.05 KB
196.64 KB

WOW!
This is great. I love the new annotation syntax.

Only missed two bits, as far as I see.

tim.plunkett’s picture

FileSize
51.68 KB

Because all of the renames are rather tedious, here's a patch that does just the changes to the annotation structure, without the actual change from handlers to controllers.
I'd like to see this get in first, and the renames can be handled in a separate patch.
This is a DX win on its own.

dawehner’s picture

Don't bikeshed about the name "controllers" as it will be replaced later anyway.

+++ b/core/lib/Drupal/Core/Entity/EntityManager.phpundefined
@@ -26,23 +26,40 @@
+ *   - list: (optional) The name of the class that provides

Talked with tim about that, we will probably have to rewrap the comments in one of the next paths anyway.

+++ b/core/modules/breakpoint/lib/Drupal/breakpoint/Plugin/Core/Entity/Breakpoint.phpundefined
@@ -23,7 +23,9 @@
+ *     "storage" = "Drupal\Core\Config\Entity\ConfigStorageController",

+++ b/core/modules/breakpoint/lib/Drupal/breakpoint/Plugin/Core/Entity/BreakpointGroup.phpundefined
@@ -20,7 +20,9 @@
+ *     "storage" = "Drupal\Core\Config\Entity\ConfigStorageController",

Annotation exception?

+++ b/core/includes/entity.api.phpundefined
@@ -155,7 +155,7 @@ function hook_entity_bundle_info_alter(&$bundles) {
-  $entity_info['node']['controller_class'] = 'Drupal\mymodule\MyCustomNodeStorageController';
+  $entity_info['node']['controllers']['storage'] = 'Drupal\mymodule\MyCustomNodeStorageController';

Just that line is a huge DX improvement!

tim.plunkett’s picture

FileSize
2.09 KB
51.68 KB

Thanks @dawehner!
Fixed the trailing commas and the typo.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Awesome.

sun’s picture

Issue tags: +API clean-up

That makes a really great first step and looks really awesome! Thanks, @tim.plunkett!

Not even sure whether we need to rename from controller to handler. The original idea seems to follow the same paradigm as the idea for renaming blocks, but that was essentially won't fixed.

effulgentsia’s picture

Title: Rename entity storage/list/form/render controllers to handlers » Rename entity storage/list/form/render controllers to handlers, part 1: organize existing annotation keys

Retitling to aid committers. See #24 for what it will look like when the next part is done.

Not even sure whether we need to rename from controller to handler.

We currently have Drupal\system\CronController, which is a controller in the Symfony sense. But the Symfony controller for User module is named Drupal\user\UserRouteController to disambiguate from all the other nothing-to-do-with-routing entity controllers in the same namespace. I think renaming the entity ones to handlers still makes sense.

sun’s picture

We currently have Drupal\system\CronController, which is a controller in the Symfony sense. But the Symfony controller for User module is named Drupal\user\UserRouteController to disambiguate from all the other nothing-to-do-with-routing entity controllers in the same namespace. I think renaming the entity ones to handlers still makes sense.

I think we're shooting ourselves in the foot with trying to disambiguate any terminology.

IMHO, the proper answer is namespaces:

If you provide a controller for entities, why don't you tell me so? Drupal\foo\Entity\FooController.

If you provide a controller for routing, why don't you tell me so? Drupal\foo\Routing\FooController.

Crell’s picture

The Symfony bundle convention is BundleName\Controllers\ThingieController.

It's just a code ambiguity question, though. It's also just normal conversation. "Do a thingie and add a Controller and modify the framistan". Which controller? Anyone who doesn't already know Entity API is going to think of the "thing formerly called page callbacks", and then when they see something else is also called a "controller" is going to go "WTF?"

webchick’s picture

Status: Reviewed & tested by the community » Needs work

Something I committed tonight conflicts with this.

I really love this patch quite a lot. However, since it's purely API clean-up, and also touches every single entity which provides a high likelihood of conflicting with feature patches, I'd rather wait until after Feb 18 to commit this, I think.

yched’s picture

Of interest to the folks subscribed here :
#1920498: Have rdf.module only act on renderable entities needs the EntityManager to be able to answer "does this entity type have a 'foo' controller ?" without a) actually instantiating the controller b) raising an exception as a means to say "the answer is no".

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
49.32 KB

Conflicted with #1882240: Remove default assignment of render_controller_class in EntityManager among others, just keeping it green.

tim.plunkett’s picture

FileSize
49.28 KB

Whoops, forgot to pull before rebasing.

tim.plunkett’s picture

Rerolled. It's after February (and March and April) 18th now!

Trying something out in patch A, but patch B should work fine.

tim.plunkett’s picture

Okay, that confirmed my thoughts. Here we are with the most minimal annotation we could hope for.

Interdiff against 49-b

msonnabaum’s picture

Status: Needs review » Reviewed & tested by the community

#50 looks great to me. Big improvement.

fago’s picture

This looks like a great step forward. Just one thing though:

+++ b/core/lib/Drupal/Core/Entity/Annotation/EntityType.php
@@ -42,13 +42,37 @@ class EntityType extends Plugin {
+   * @todo Interfaces from outside \Drupal\Core or \Drupal\Component should not
+   *   be used here.

Where should modules add their controllers to then?

msonnabaum’s picture

Oh, btw, I started a new issue for the controller renaming since this thread is now focussed on annotations: #1976158: Rename entity storage/list/form/render "controllers" to handlers

tim.plunkett’s picture

@fago, http://api.drupal.org/api/search/8/hook_entity_info has been repurposed to act as a "build+enhance" pass to entity info gathering. It runs before alter.

That is where translation_entity should be hooking in.

Also, note that this @todo is only moved by this patch, it was added in the @EntityTYpe issue.

alexpott’s picture

Title: Rename entity storage/list/form/render controllers to handlers, part 1: organize existing annotation keys » Change notice: Rename entity storage/list/form/render controllers to handlers, part 1: organize existing annotation keys
Priority: Normal » Critical
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

Committed bc3a902 and pushed to 8.x. Thanks!

We need to look at the current change notices and update accordingly...

plach’s picture

We also need a change notice announcing this, I guess.

xjm’s picture

Issue tags: -Avoid commit conflicts
xjm’s picture

Title: Change notice: Rename entity storage/list/form/render controllers to handlers, part 1: organize existing annotation keys » Change notice: Reorganizie entity storage/list/form/render controller annotation keys

The title of this issue is misleading; can we retitle like this?

Xano’s picture

The exceptions that are thrown by the entity manager can now be quite confusion to people who've never seen them before, as a missing storage controller exception reads "The entity (...) did not specify a storage" (the word "controller" is no longer there).

Xano’s picture

FileSize
1.67 KB
alexpott’s picture

@Xano can you create a new issue for #60... thanks!

Xano’s picture

tim.plunkett’s picture

Title: Change notice: Reorganizie entity storage/list/form/render controller annotation keys » Reorganizie entity storage/list/form/render controller annotation keys
Priority: Critical » Normal
Status: Active » Fixed
Issue tags: -Needs change record

Since this is a HEAD2HEAD change, it doesn't need a stand-alone change notice, but I've gone through pre-existing ones and updated them:

http://drupal.org/node/1827470
http://drupal.org/node/1819308
http://drupal.org/node/1799642
http://drupal.org/node/1818734
http://drupal.org/node/1734556

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

Anonymous’s picture

Issue summary: View changes

Adding proposal