Rename entity storage/list/form/render "controllers" to handlers

I think it helps for there to be some common name grouping these things. Otherwise, "access" and "label" are keys on the same level, but the former refers to a class/service while the latter refers to a string literal.

We are going back and forth on which bad name to choose

I thought consensus was fairly solid in #1807042: Reorganizie entity storage/list/form/render controller annotation keys on "handler". From a purely English standpoint (e.g., "a baggage handler"), I think that's a reasonable name. However, I'm not aware of it being a commonly adopted name in the software field. On Wikipedia, the pattern is named Servant.

The list controller isn't a list, it's a "thing that builds a list than you can render" ?
Similarly, the render controller isn't a renderable, it's a "thing that builds a renderable array" ?

Controller, handler - the important bit IMO is to have a word that represents that "thing that does X" indirection.
With a personal preference on "handler", precisely because it seems more agnostic than "controller".

"Handles", as in "takes care of", looks just fine to me.

Sure, "render arrays" are a drupalism, but an object returning something that's "renderable" does qualify as being a "render handler", independently of the implementation details ?

Might very well be so, but then this is about the model that is our current D8 state more than about the nomenclature we put on it ?

I mean, the model & pattern might be bad, but I'd think we're better off putting names that designate it clearly for what it is ?

I agree that handler isn't a perfect name, and that they're not all in one role. But one perspective I'd like to offer, feel free to disagree: access, list, form, and render are kind of the UI equivalent of CRUD (not individually a 1:1 map, just when taken as a whole). Whereas storage is the non-UI part of CRUD.

With #1983844: Add EntityListController and convert picture.module callbacks to routes/controllers to provide a use-case this just got a whole lot more confusing. We now have an EntityListController class which is in fact not an entity controller, but a route controller, which finds the correct entity list controller for the entity type. For extra fun, it is located in \Drupal\Core\Entity\Controller\EntityListController while all the entity controllers are located directly in \Drupal\Core\Entity. Lucky that we don't have a general list controller base class otherwise we would have \Drupal\Core\Entity\Controller\EntityListController call a method of \Drupal\Core\Entity\EntityListController.

I like the idea of doing away with the handler/controller suffix. Initially I thought it maps well to Storage and Form already, but not so with e.g. rendering. I think that is as we are used to referring to the form functions handling a form as "form", while we aren't used to something similar for Renderable.

$list->render() fits very well also, I'm not sure about Renderable and AccessController though. Maybe better we'd have AccessControll to differentiate better to Controllers, thus $access_controll->viewAccess() ?

For Renderable, why not go back to "View" ? We have entity forms and form modes, why not have entity views and view modes? "RenderController" is actually a bit un-specific, forms or lists are rendered also. What we are rendering here is an entity view though, so why not have $block_view->render() instead of $block_render_controller->view() ?

The list controller isn't a list, it's a "thing that builds a list than you can render" ?

Like a Form isn't the real form, but a "thing that builds the form array you can process". It still represents our form though.

We've already started ditching "Controller" in some form controllers, like \Drupal\filter\Form\FilterDisableForm and \Drupal\user\Form\UserRoleDelete

I agree that we should ditch "Controller" where it isn't needed to explain what it is:

FooStorageController => FooStorage
FooOperationFormController => FooOperationForm
FooListController => FooList

But for the other ones, I say we swap for Handler, just to resolve any ambiguity with Controller (which is becoming more of a problem the further #1971384: [META] Convert page callbacks to controllers progresses):

FooRenderController => FooRenderHandler
FooAccessController => FooAccessHandler
FooTranslationController => FooTranslationHandler

Mark, you're not going to push for FooRepository instead of FooStorage? :-)

#13 works for me. The only thing I'm still bother with whether we should revert back to "view-controller" or "view-handler" instead of rendering, as
- not only the entity-view is rendered, the result of a form also - so that's unspecific
- methods do not involve rendering, actually rendering is done later on - so it's actually wrong
- "render" does not map with the hooks fired, which are "view"

So what about FooViewHandler?

It's entity-view and we already have a hook called like that, so if it's confusing with Views module it is already.

+1 to #13. How does it affect annotation keys?

+1 to #16 if whoever rolls this patch is willing to include it as part of the scope.

Right, we still have the "controllers" top-level key in the annotation.
If we're switching Controller to Handler in most of them, we should rename that "handlers" as well.

Discussed with @Dries, @effulgentsia, @msonnabaum, and @alexpott. We came to the conclusion that renaming these is worth the BC break and disruption to other patches. We agree with the first part of #13. For render, we'd like to propose FooRenderBuilder, and for access and translation, let's try to come up with some other possibilities for the names. It would be better to avoid "handler" if possible.

Let's re-scope this as a meta issue and create sub-issues for each name pattern. The first three name changes in #13 are approved and we can start creating patches for those right away.

So, we had a lot of fun discussing this one, these are the conclusions we reached:

CONCLUSION: ones we have and how we will rename them:
- storage -> FooStorage
- form -> FooOperationForm
- list ->FooList
- view_builder -> FooViewBuilder
- access -> FooAccess
- translation -> FooTranslationHandler, FooTranslationUiAdaptor, FooTranslationUiProvider, FooTranslationUiBridge → conclusion is that we cannot name this as probably most of it will go away and we are not sure on what will be left here.
It’s fine if the patch only renamed the classes, because that’s a good DX win already. Then we open a separate issue for renaming the controllers key and moving certain keys to top level.
TODO: open issue to settle on the name of the parent key (previously controllers): handler vs service

Full notes: https://docs.google.com/document/d/108NNmlN2QmX_d3db00MQE9Cwbc_YsmubT4at...

And here's the first one: #2097903: Rename EntityRenderController to EntityViewBuilder

Turns out to be more work than I thought, so moving to sub-issues.

Tagging.

The new one #2100467: Menu links are not ready to use a view builder so remove it from the annotation for now

Do we still agree this is major, given that 20% 13.4% of the criticals are entity/field API related, and patches like these will only force re-rolls of those, plus other bug fixes in the queue?

Wasn't it ~13.4% three days ago? I, for one, still think this is major and that it needs to get done.

Yep, sorry. I forgot my calculations. :) I'll respond in the other issue.

Added #2154435: Rename EntityAccessController to EntityAccessControlHandler
suppose the biggest patches would be form and storage related

Is this issue still major? maybe better to postpone this one to allow other changes to make in?

That's a rather big change in how devs perceive the system and affects a lot of class names, thus imo it should be targeted for beta.

So according to #22 we've an agreement, except for:

TODO: open issue to settle on the name of the parent key (previously controllers): handler vs service

We need to figure this out *now*, or we'll stay with totally confusing controllers.

Personally, I like "service" but I don't think it's a good match for all of the controllers as some of them expose no API that is meant for public consumation at all, e.g. stuff like the entity translation controllers or ViewIntegration controllers. Consequently, I think that "handler" fits us better.

Thoughts?

I remember there was a discussion once to register these entity "controllers" in the DIC, and maybe that's something we still want to do. Would that influence the decision here?

In any case, I prefer "handlers" too.

+1 to "handlers", also patches in issues needs to be updated to use this!

Another +1 for "handlers" :)

Let's get conclusion here, any other thoughts?

I'm still ok with 'handlers' as the top-level key for all of them.

Alternatively, per #31, we could introduce two top-level keys: 'services' and 'delegates', where the first is for ones for outside use and the second is for ones completely encapsulated by the entity class. However, while the naming purist part of me likes this alternative, I see two potential downsides to it from a practical standpoint:
1) We'd need to agree which are which, and I don't think that will get settled very quickly.
2) 'services' would be the correct term from a DDD standpoint, but could cause confusion due to the *.services.yml file being something similar but not exactly the same.

s/EntityListController/ListBuilder suggested in #2120871-45: Rename EntityListController to EntityListBuilder but this list builder bound to be used for entities

The same question is about EntityAccess that actually access checker but this name already used, and EntityAccess is actually bad name #2154435: Rename EntityAccessController to EntityAccessControlHandler

Filed suggested issue about storage #2188613: Rename EntityStorageController to EntityStorage

I thought more about #36 and the concerns I raised there, and have an updated recommendation:

1. We remove the "controllers" parent key entirely. Instead, we have "storage", "access_control", "list_builder", "view_builder", and "forms" as top-level annotation keys (note: those key names are my current suggestion but can be debated/tweaked as needed).
2. We remove get*Controller() as public methods from EntityManager. Instead, we make those protected methods using whatever terminology we decide on, but the point is no one from the outside deals with that name at all. Instead, we add a public API to EntityManager to satisfy the public needs related to storage, access control, lists, and view. E.g., load(), save(), createAccess(), access(), viewList(), and viewMultiple(). EntityManager can delegate to the classes specified by the annotation, but that's an implementation detail that doesn't need to leak to consumers.
3. We move EntityManager::getFormController() to Entity::getForm(), and that needs to be public and defined in an interface.
4. For other "controllers" (e.g., "translation", "config_translation_list", and whatever contrib modules want to add), we make those real services. No annotation key. Instead, a proper *.services.yml entry, tagged in such a way as to automatically connect it to the correct service type and entity type. That would resolve the confusion of #36.2.

Thoughts?

1) That feels like cheating, but doesn't bother me so much.
2) Unless we add a method to EntityManager for every single public method on each of the different controller interfaces, I don't see how this is feasible. And that would be a lot of methods.
3) We're actually really really close to not needing getFormController at all, only HtmlEntityFormController and EntityFormBuilder should be using it... I think getForm() is confusing in that sense since it returns an object, not a form array.
4) I really don't understand this one. Unless we use magical naming, how would we find the right service for a given entity type? And then we'll need FooServiceProvider classes everywhere?

Problem with #40 is that we need this to be extensible on a modular basis.

E.g., Rules would provide something like a RulesIntegrationController/Handler/Service/something which integrates an entity type with Rules, but can be overridden and adapted as needed. Removing getController() because of naming issues seems like a step back as it removes the re-usability of that and would urge every module needing that to reinvent it.

SO what is a fate of #2154435: Rename EntityAccessController to EntityAccessControlHandler - do we really wanna have 2 kind of controllers?
1) request handlers
2) access handlers

Also EntityStorage needs review in #2188613: Rename EntityStorageController to EntityStorage

FWIW, I've just wasted an evening fixing a range of test config entities because their keys and classes were incorrect, and I feel like there is no real improvement or value in any of the keys or names chosen.

This does appear to me to be a bikeshed which has somehow resulted in renaming. If we can't think of good names for the types of objects we create, we probably shouldn't waste time redesigning their naming conventions and keys. For example, "list" to "list_builder" makes almost zero difference to someone who doesn't understand the intricacies of the system.

Just a thought...

And the entity form rename is up: #2238077: Rename EntityFormController to EntityForm

So only key name and last one #2154435: Rename EntityAccessController to EntityAccessControlHandler left here

Updated the issue summary.

On @xjm's advice, I've improved the API change "before" and "after" a bit.

@berdir raised yesterday the question of the annotation group key itself. At this point I think it would be best to simply change 'controllers' to 'handlers'.

Yep, that + methods like getControllerClass(), probably some documentation references to "entity controllers" or so, and $form_state['controller']: #2313931: Merge $form_state['controller'] with $form_state['build_info']['callback_object'] and add a method for it.

Here it goes :)

- Renames controllers to handlers
- Renames all *Controller* methods on EntityType and EntityManager to *Handler*
- Removes entity_get_controller()
- Updates a ton of sometimes insanely old and outdated documentation, beautiful things like this:

- * Defines a common interface for entity controllers.
+ * Defines a common interface for entity handlers.
  *
- * This interface can be implemented by entity controllers that require
- * dependency injection. These are not controllers in the routing sense of the
- * word, but instead are handlers that perform a specific function for an entity
- * type.
+ * This interface can be implemented by entity handlers that require
+ * dependency injection.

Ups, somehow missed a few entity types and variables/documentation, let's see if this works better.

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityDatabaseStorage.php
   @@ -23,11 +23,9 @@
   - * Defines a base entity controller class.
   + * A content entity database storage implementation
   

   needs dot(.) at the end of sentence

2. +++ b/core/lib/Drupal/Core/Entity/EntityDatabaseStorage.php
   @@ -12,7 +12,7 @@
   - * Defines a base entity controller class.
   + * Defines a base entity storage class.
   
   +++ b/core/lib/Drupal/Core/Entity/EntityHandlerInterface.php
   @@ -2,27 +2,23 @@
   - * Defines a common interface for entity controllers.
   + * Defines a common interface for entity handlers.
   
   +++ b/core/lib/Drupal/Core/Entity/EntityStorageBase.php
   @@ -13,7 +13,7 @@
     * A base entity storage class.
   
   +++ b/core/lib/Drupal/Core/Entity/EntityStorageInterface.php
   @@ -8,15 +8,13 @@
   - * Defines a common interface for entity storage classes.
   + * Defines the interface for entity storage implementations.
   
   +++ b/core/modules/aggregator/src/FeedStorageInterface.php
   @@ -11,7 +11,7 @@
   - * Defines a common interface for aggregator feed entity controller classes.
   + * Defines an interface for aggregator feed entity storage classes.
   
   +++ b/core/modules/comment/src/CommentStorageInterface.php
   @@ -12,7 +12,7 @@
   - * Defines a common interface for comment entity controller classes.
   + * Defines an interface for comment entity storage classes.
   
   +++ b/core/modules/file/src/FileStorageInterface.php
   @@ -10,7 +10,7 @@
   - * Defines a common interface for file entity controller classes.
   + * Defines an interface for file entity storage classes.
   
   +++ b/core/modules/node/src/NodeStorageInterface.php
   @@ -11,7 +11,7 @@
   - * Defines a common interface for node entity controller classes.
   + * Defines an interface for node entity storage classes.
   
   +++ b/core/modules/shortcut/src/ShortcutSetStorageInterface.php
   @@ -11,7 +11,7 @@
   - * Defines a common interface for shortcut entity controller classes.
   + * Defines an interface for shortcut_set entity storage classes.
   
   +++ b/core/modules/taxonomy/src/TermStorageInterface.php
   @@ -11,7 +11,7 @@
   - * Defines a common interface for taxonomy term entity controller classes.
   + * Defines an interface for taxonomy_term entity storage classes.
   
   +++ b/core/modules/taxonomy/src/VocabularyStorageInterface.php
   @@ -10,7 +10,7 @@
   - * Defines a common interface for taxonomy vocabulary entity controller classes.
   + * Defines an interface for vocabulary entity storage classes.
   
   +++ b/core/modules/user/src/RoleStorageInterface.php
   @@ -10,7 +10,7 @@
   - * Defines a common interface for roel entity controller classes.
   + * Defines an interface for role entity storage classes.
   
   +++ b/core/modules/user/src/UserStorageInterface.php
   @@ -11,7 +11,7 @@
   - * Defines a common interface for user entity controller classes.
   + * Defines an interface for user entity storage classes.
   

   let's unify them once touched

Great to see the renames finally become complete. The approach taken looks great to me and makes a lot of sense. I've not done a full, proper review (yet), just checked the patch from a bird-eye view.

Re-roll after some minor context conflicts due to renames and additions, tried to unify the documentation a bit more.

Failed because of the new views_data entity handler.

Re-roll.

+++ b/core/includes/entity.inc
@@ -283,21 +283,6 @@ function entity_create($entity_type, array $values = array()) {
-function entity_get_controller($entity_type) {
-  return \Drupal::entityManager()
-    ->getStorage($entity_type);
-}

It's hilarious to me that this function is still around.

Reviewed line by line, thanks @Berdir for seeing this through.

+++ b/core/lib/Drupal/Core/Entity/EntityManager.php
@@ -300,19 +300,19 @@ public function getAccessControlHandler($entity_type) {
+  public function getHandler($entity_type, $handler_type, $controller_class_getter = NULL) {
+    if (!isset($this->controllers[$handler_type][$entity_type])) {

The doc block for this needs a update and also EntityManager::$controllers. Additionally - $controller_class_getter? Maybe this should be changed to {@inheritdoc} and the class getter added to the interface.

$this->assertEqual($entity_type->getStorageClass(), 'Drupal\Core\Entity\EntityDatabaseStorage', 'Entity controller class info is correct.');

In EntityApiInfoTest needs fixing too.

EntityManager::getFormObject() also uses $this->controllers

Ok, cleaned up everything now I think. All remaining "controller" mentions in Drupal\Core\Entity now refer to actual controllers.

Re-roll.

PS: This looks really sane. No real out of scope changes, berdir++

1. +++ b/core/lib/Drupal/Core/Entity/EntityFormInterface.php
   @@ -13,7 +13,7 @@
   - * Defines a common interface for entity form classes.
   + * Defines an interface for entity form classes.
   

   I love this change. From one pointless line to another one, but sure not a problem of this patch.

2. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
   @@ -241,92 +241,80 @@ public function hasController($entity_type, $controller_type) {
   -        throw new InvalidPluginDefinitionException($entity_type, sprintf('The "%s" entity type did not specify a %s class.', $entity_type, $controller_type));
   +        throw new InvalidPluginDefinitionException($entity_type, sprintf('The "%s" entity type did not specify a %s class.', $entity_type, $handler_type));
   

   I wonder whether we could improve the exception message here. It should rather be "... dit not specify a %s handler".

3. +++ b/core/lib/Drupal/Core/Entity/EntityManagerInterface.php
   @@ -186,32 +186,34 @@ public function getListBuilder($entity_type);
   +   * @param string $form_class_getter
   +   *   (optional) The method to call on the entity type object to get the handler class.
   ...
   -  public function getController($entity_type, $controller_type);
   +  public function getHandler($entity_type, $handler_type, $form_class_getter = NULL);
   

   I wonder whether it could be named in a more abstract way like, $handler_class_getter

1. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
   @@ -241,92 +241,80 @@ public function hasController($entity_type, $controller_type) {
   +  public function getHandler($entity_type, $handler_type, $form_class_getter = NULL) {
   +    if (!isset($this->handlers[$handler_type][$entity_type])) {
   ...
   +        $class = $definition->{$form_class_getter}();
   

   I don't see a place though where this parameter is passed along.

2. +++ b/core/modules/link/src/Tests/LinkFieldTest.php
   @@ -554,7 +554,7 @@ function testLinkSeparateFormatter() {
   -   *   (optional) Whether to reset the test_entity controller cache. Defaults to
   +   *   (optional) Whether to reset the entity_test cache. Defaults to
   

   Shouldn't we talk about the storage cache here?

Discussed the class_getter stuff with @dawehner and @timplunkett and nobody quite figure why we need it exactly, so it is gone now.

2. Using handler now.
the second 2.: Yep, made it storage cache now.

Someone is messing with me :)

Fixed that fail, looks like we got a new test entity type in the family ;)

Looks all addressed

I guess it makes sense to rename this issue now.

Committed 629ebca and pushed to 8.0.x. Thanks!

Woohooo ! Awesome !
1 year and a half for this rename :-)

Uff, that was quite a ride :)

I updated https://www.drupal.org/node/2200867 slightly and re-published it, so that people get notified.