Entity API documentation should consistently refer to handlers rather than controllers

Good idea... Any thoughts on what to say for the overview though? There's already a list of what handlers exist on the Entity API page, and I don't think it's all that useful to say "they do stuff" or "they provide all sorts of functionality'. It's really kind of a meaningless term that we've invented to mean "a bunch of other classes that separate out the basic functionality of entities into different pieces".

So rather than actually defining what a 'handler' is, I think it might be sufficient to just change where it says:

The annotation will refer to several controller classes, which you will also need to define:

in the Entity API topic, to instead say:

The annotation will refer to several "handler" classes, which you will also need to define:

My suggestion for EntityHandlerInterface and EntityHandlerBase is that we just add a line saying
@ingroup entity_api
which will refer people to the topic if they are wondering what these are about.

Where can I find the Entity API documentation?

https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Entity%21...

http://cgit.drupalcode.org/drupal/tree/core/lib/Drupal/Core/Entity/entit...

Hello,

I've included a patch with the changes as suggested by @jhodgdon.

This is only a documentation update.

Regards,

Fixed the indentation to fill as many characters as possible.

Tag fix

Not everything in the issue summary under Proposed Resolution is taken care of, I think?

I am going to have a look at proposal 1

I am going to have a look at issue 1

I'm a mentor. Helping @katewelling to work on the issue.

Picking this up for catalyst academy. This will be my very first patch

In Entity API documentation topic, changed "controller" to "handler" in translation section under Defining an entity type. That should cover the first proposed resolution. Second resolution was already included as part of previous patch with the suggestion @ingroup entity_api from @jhodgdon. I have used grep -nrI "* .*controller" * to search for any other instances of controller in the files, which there were none except those referring to routing controllers. Please review, this is my very first patch on drupal.

Nice work, thanks!

I also grepped for "controller" in Core and saw several additional places that also need to be fixed:

a)

core/includes/entity.inc: * "controllers['storage']" key in the entity plugin annotation. These classes

This should be handlers['storage']

b)

core/lib/Drupal/Core/Config/NullStorage.php: * This storage is always empty; the controller reads and writes nothing.

This is referring to an entity storage *handler*

c)

core/lib/Drupal/Core/Field/FieldItemInterface.php:   *     as it is determined automatically by the storage controller depending

storage *handler*

d) This one is a bit tricky. In entity.api.php (in the Entity API topic), in the routing section, there are actually some references to controllers that should be handler. Where it says:

The value is composed of the entity type machine name and a form controller type from the entity annotation n (see Defining an entity type above more more on controllers and annotation). So, in this example, block.default refers to the 'default' form controller on the block entity type, whose annotation contains:

In this passage, everywhere it says "controller" it should say "handler".

e) Two base/generic handler classes have docs headers that still call themselves "controllers":
- EntityListController
- EntityViewController

f) Several specific entity handler classes have docs headers that still call themselves "storage controllers" or "form controllers" or something similar:
- CommentStorage
- CommentTypeForm
- CommentViewBuilder
- CommentForm
- ProfileForm (in user module)
- RegisterForm (in user module)
- NodeStorage
- NodeForm
- NodeViewBuilder [should say it is a view builder handler not a render controller]
- NodeGrantDatabaseStorage
- NodeTypeForm
- ShortcutForm
- ShortcutSetForm
- FeedViewBuilder (in aggregator module)
- FeedForm (aggregator)
- ItemViewBuilder (aggregator)
- BlockContentViewBuilder [should say it is a view builder handler not a render controller]
- BlockContentForm
- TermViewBuilder (taxonomy module) [should say it is a view builder handler not a render controller]
- TermForm (taxonomy module)
- VocabularyStorage (taxonomy)

g)

core/modules/language/src/LanguageListBuilder.php:   *   The entity storage controller class.
core/modules/language/src/Config/LanguageConfigOverride.php:   *   A storage controller object to use for reading and writing the
core/modules/node/src/Plugin/views/argument/Type.php:   * NodeType storage controller.
core/modules/taxonomy/src/Form/OverviewTerms.php:   * The term storage controller.

These also seem to be referring to entity storage handlers

h)

core/modules/config_translation/src/ConfigNamesMapper.php:   *   - list_controller: (optional) Class name for list controller used to

... class name for list handler...

i)

core/modules/views/src/EntityViewsData.php:   * Entity type for this views controller instance.
core/modules/views/src/EntityViewsData.php:   *   The storage controller used for this entity type.

The first one should be talking about a views data handler; the second is an entity storage handler.

j) Several spots in core/modules/field_ui/src/EntityDisplayModeListBuilder.php in the isValidEntity method doc block. These are all referring to entity view builder handlers, not route controllers.

k) The EntityFormModeListBuilder class has the same problem as (f).

Hopefully everything @jhodgdon mentioned has been addressed in this patch. And thank you @jhodgdon for your extensive and descriptive comment

Thanks! Looking at the interdiff, it looks like everything was taken care of except:

e) EntityViewController... but yeah actually that *is* a controller. My bad!

So... Could you please make one more pass through this patch, and make sure things that are view builder handlers are all called that? Some places, they are called "render handlers" still. Let's make them a bit more uniform, if possible. Same with storage handlers, sometimes the word "storage" is missing from the description.

And a few lines need to be rewrapped closer to 80 characters after changing wording.

I need to run, sorry... hope that is enough detail!

Another go at it, thanks @jhodgdon for the comments

Found a wrapping issue in my last patch, should be fixed here

Thanks for the new patches! This looks good except for a few things (mostly one error repeated several times, see below).

1. +++ b/core/lib/Drupal/Core/Entity/Controller/EntityController.php
   @@ -35,7 +35,7 @@ class EntityController implements ContainerInjectionInterface {
      protected $entityManager;
    
      /**
   -   * Constructs a new EntityController.
   +   * Constructs a new EntityHandler.
   

   Oops. The class name here is still EntityController, so we shouldn't change this line.

2. +++ b/core/lib/Drupal/Core/Entity/Controller/EntityListController.php
   @@ -10,7 +10,7 @@
    use Drupal\Core\Controller\ControllerBase;
    
    /**
   - * Defines a generic controller to list entities.
   + * Defines a generic handler to list entities.
     */
    class EntityListController extends ControllerBase {
   

   This one is actually a routing controller, not an entity handler. So let's not make any change to this doc block. Sorry for the misdirection in my earlier review!

3. +++ b/core/modules/aggregator/src/FeedForm.php
   @@ -12,7 +12,7 @@
   + * Form storage handler for the aggregator feed edit forms.
   

   Whoops. Sorry I wasn't clear in my too-quick-gotta-run comment previously!

   There are several types of handlers:
   - Entity storage handlers
   - Entity form handlers
   - View builder handlers
   - List handlers
   etc.

   Not all of them are "storage" handlers, just the entity storage handlers themselves.

   So this one should just say "Form handler" not "Form storage handler". [Note: in comments below I've just said something like "Form handler (not storage)" to indicate other places where this change is needed.]

4. +++ b/core/modules/block_content/src/BlockContentForm.php
   @@ -16,7 +16,7 @@
   + * Form storage handler for the custom block edit forms.
   

   Form handler (not storage)

5. +++ b/core/modules/comment/src/CommentForm.php
   @@ -20,7 +20,7 @@
   + * Base for storage handler for comment forms.
     */
    class CommentForm extends ContentEntityForm {
   

   How about:

   Base handler for comment forms.

6. +++ b/core/modules/field_ui/src/EntityFormModeListBuilder.php
   @@ -15,7 +15,7 @@
   +   * Filters entities based on their storage handlers.
   

   Looking at the code for this method (isValidEntity()), it does this:

       return $this->entityTypes[$entity_type]->hasFormClasses();
   

   Then the EntityType::hasFormClass() method does this:

    return !empty($this->handlers['form']);
   

   So it appears it is filtering based on the presence of form handlers, not storage handlers. I suggest:

   Filters entities based on their form handlers.

7. +++ b/core/modules/node/src/NodeForm.php
   @@ -14,7 +14,7 @@
   + * Form storage handler for the node edit forms.
   

   Form handler (not storage)

8. +++ b/core/modules/node/src/NodeTypeForm.php
   @@ -15,7 +15,7 @@
   + * Form storage handler for node type forms.
   

   Form handler (not storage)

9. +++ b/core/modules/shortcut/src/ShortcutForm.php
   @@ -11,7 +11,7 @@
   + * Form storage handler for the shortcut entity forms.
   

   Form handler (not storage)

10. +++ b/core/modules/shortcut/src/ShortcutSetForm.php
    @@ -11,7 +11,7 @@
    + * Form storage handler for the shortcut set entity edit forms.
    

    Form handler (not storage)

11. +++ b/core/modules/taxonomy/src/TermForm.php
    @@ -11,7 +11,7 @@
    + * Base for storage handler for taxonomy term edit forms.
    

    Form handler (not storage)

12. +++ b/core/modules/user/src/ProfileForm.php
    @@ -13,7 +13,7 @@
    + * Form storage handler for the profile forms.
    

    Form handler (not storage)

13. +++ b/core/modules/user/src/RegisterForm.php
    @@ -13,7 +13,7 @@
    + * Form storage handler for the user register forms.
    

    Form handler (not storage)

Done.

+++ b/core/modules/comment/src/CommentTypeForm.php
@@ -16,7 +16,7 @@
- * Base form controller for category edit forms.
+ * Base form handler for category edit forms.
  */
 class CommentTypeForm extends EntityForm {

This error is not introduced by this patch... but maybe we can fix it here anyway.

This doc block was apparently copy/pasted from another class, probably in User module.

But anyway... This is actually the form handler for comment type edit forms. Not "category" edit forms.

Everything else looks great in this patch. So to save time, I edited this one line in the patch to say "Base form handler for comment edit forms", and took the liberty of marking it RTBC.

1. +++ b/core/lib/Drupal/Core/Config/NullStorage.php
   @@ -10,7 +10,7 @@
   - * This storage is always empty; the controller reads and writes nothing.
   + * This storage is always empty; the handler reads and writes nothing.
   

   This is about config and not entities - out-of scope. Config storage documentation should get its own issue.

2. +++ b/core/lib/Drupal/Core/Entity/Controller/EntityListController.php
   @@ -28,4 +28,3 @@ public function listing($entity_type) {
    }
   -
   

   This is an out-of-scope change

3. +++ b/core/modules/config_translation/src/ConfigNamesMapper.php
   @@ -103,7 +103,7 @@ class ConfigNamesMapper extends PluginBase implements ConfigMapperInterface, Con
   -   *   - list_controller: (optional) Class name for list controller used to
   +   *   - list_controller: (optional) Class name for list handler used to
       *     generate lists of this type of configuration.
   

   I think this change is incorrect - this needs more investigation. See FieldConfigListController and EntityListController - these are controllers and not entity handlers. Also I can't see how the list_controller is actually used. I think it is defunct and should be removed - in a separate issue.

4. +++ b/core/modules/language/src/Config/LanguageConfigOverride.php
   @@ -33,8 +33,8 @@ class LanguageConfigOverride extends StorableConfigBase {
   -   *   A storage controller object to use for reading and writing the
   -   *   configuration override.
   +   *   A storage handler object to use for reading and writing the configuration
   +   *   override.
   

   This is not about entity storage it is about config storage again... Also imo both handler and object are unnecessary. But I think it is out-of-scope to this issue.

The patch does not apply to the current HEAD..candidate for reroll

I am working in this issue.

Updated the patch based on alexpott suggestion.

@shashi please upload interdiff in case of new patch with respect to old one.
Thanks for your contribution.

I'll review this once there is an interdiff file, thanks.

I am unable to create interdiff file for it.
Getting message

1 out of 1 hunk FAILED -- saving rejects to file /tmp/interdiff-1.azQznI.rej
interdiff: Error applying patch1 to reconstructed file

This is what I got while creating interdiff from patch #31 and #35

reverted:
--- b/core/lib/Drupal/Core/Config/NullStorage.php
+++ a/core/lib/Drupal/Core/Config/NullStorage.php
@@ -10,7 +10,7 @@
 /**
  * Defines a stub storage.
  *
+ * This storage is always empty; the controller reads and writes nothing.
- * This storage is always empty; the handler reads and writes nothing.
  *
  * The stub implementation is needed for synchronizing configuration during
  * installation of a module, in which case all configuration being shipped with
reverted:
--- b/core/lib/Drupal/Core/Entity/Controller/EntityListController.php
+++ a/core/lib/Drupal/Core/Entity/Controller/EntityListController.php
@@ -28,3 +28,4 @@
   }

 }
+
reverted:
--- b/core/modules/config_translation/src/ConfigNamesMapper.php
+++ a/core/modules/config_translation/src/ConfigNamesMapper.php
@@ -103,7 +103,7 @@
    *   - names: (optional) An array of configuration names.
    *   - weight: (optional) The weight of this mapper, used in mapper listings.
    *     Defaults to 20.
+   *   - list_controller: (optional) Class name for list controller used to
-   *   - list_controller: (optional) Class name for list handler used to
    *     generate lists of this type of configuration.
    * @param \Drupal\Core\Config\ConfigFactoryInterface $config_factory
    *   The configuration factory.
reverted:
--- b/core/modules/language/src/Config/LanguageConfigOverride.php
+++ a/core/modules/language/src/Config/LanguageConfigOverride.php
@@ -33,8 +33,8 @@
    * @param string $name
    *   The name of the configuration object being overridden.
    * @param \Drupal\Core\Config\StorageInterface $storage
+   *   A storage controller object to use for reading and writing the
+   *   configuration override.
-   *   A storage handler object to use for reading and writing the configuration
-   *   override.
    * @param \Drupal\Core\Config\TypedConfigManagerInterface $typed_config
    *   The typed configuration manager service.
    * @param \Symfony\Component\EventDispatcher\EventDispatcherInterface $event_dispatcher

and

--- interdiff-1.vE1W6b                                                                                                                         
+++ interdiff-1.vE1W6b                                                                                                                         
@@ -61,7 +61,7 @@
    * @param \Drupal\Core\Entity\EntityTypeInterface $entity_type   
    *   The entity type definition.         
    * @param \Drupal\Core\Entity\EntityStorageInterface $storage  
-   *   The entity storage controller class.     
+   *   The entity storage handler class.                                        
    * @param \Drupal\Core\Language\LanguageManagerInterface
    *   The language manager.                                        
    * @param \Drupal\Core\Config\ConfigFactoryInterface $config_factory     

Kindly let me know how to create interdiff in this case.

Thanks for trying on the interdiff file -- yes, they can sometimes be tricky, especially when some files are in one patch and not in the other.

Anyway... reviewed the patch. It's looking pretty good, and and the review in #32 was addressed, so thanks! But I found a few more problems that still need to be fixed:

1. +++ b/core/lib/Drupal/Core/Field/FieldItemInterface.php
   @@ -71,8 +71,8 @@ public static function mainPropertyName();
   +   *     as it is determined automatically by the storage handler depending on
   +   *     the table layout and the property definitions. It is recommended to
   

   Hm. I guess this is part of the Field API not the Entity API so, along the lines of comment #32, this change is out of scope for this issue and should be removed.

2. +++ b/core/modules/aggregator/src/FeedViewBuilder.php
   @@ -16,7 +16,7 @@
   + * View builder handler for aggregator feed items.
     */
    class FeedViewBuilder extends EntityViewBuilder {
   

   This should say "for aggregator feeds" ...

3. +++ b/core/modules/aggregator/src/ItemViewBuilder.php
   @@ -10,7 +10,7 @@
   + * View builder handler for aggregator feed items.
     */
    class ItemViewBuilder extends EntityViewBuilder {
   

   ... because this is the class that handles aggregator feed *items*.

4. +++ b/core/modules/comment/src/CommentTypeForm.php
   @@ -16,7 +16,7 @@
   + * Base form handler for comment edit forms.
     */
    class CommentTypeForm extends EntityForm {
   

   This is actually the handler for comment *type* edit forms, not comment edit forms.

5. +++ b/core/modules/field_ui/src/EntityDisplayModeListBuilder.php
   @@ -130,14 +130,14 @@ public function render() {
   +   *   entity doesn't have the correct view builer handler.
   

   builder is misspelled here.

6. +++ b/core/modules/field_ui/src/EntityFormModeListBuilder.php
   @@ -15,7 +15,7 @@
   +   * Filters entities based on their form handlers.
   

   form -> form mode

Added suggestions.

Thanks for the new patch! We're getting closer, but it still needs a bit of work:

+++ b/core/modules/aggregator/src/FeedViewBuilder.php
@@ -16,7 +16,7 @@
+ * View builder handler for aggregator feeds items.

Sorry, I guess I wasn't clear in my last review. Just "aggregator feeds" here. This entity is for entire feeds, not items within a feed.

And then here:

+++ b/core/modules/aggregator/src/ItemViewBuilder.php
@@ -10,7 +10,7 @@
+ * View builder handler for aggregator feeds items.

This one should say "for aggregator feed items", because this entity is for the items within a feed. Not "feeds items".

And ... now there is a new problem introduced in this patch:

+++ b/core/includes/entity.inc
@@ -147,10 +147,10 @@ function entity_revision_delete($entity_type, $revision_id) {
+ * or, most commonly, extend the Drupal\Core\Entity\Sql\SqlContentEntityStorage
+ * class.
  * See Drupal\node\Entity\Node and Drupal\node\NodeStorage
  * for an example.

You've rewrapped this part of the documentation, but it's not wrapped correctly now.

A few problems:

a) Classes with namespaces -- the namespace should start with \ always.

b) The whole thing, including the last two lines, needs to be wrapped as a single paragraph. Or if you think that the sentence starting See ... in the last two lines belongs in a separate paragraph, then you need to leave a blank line between the previous paragraph and this paragraph... personally I think it is all meant to be one paragraph so it needs to be wrapped as one paragraph.

Sorry, misinterpreted. Added new suggestions.

No, still not right.

1. +++ b/core/includes/entity.inc
   @@ -143,16 +143,15 @@ function entity_revision_delete($entity_type, $revision_id) {
   + * \Drupal\Core\Entity\Sql\SqlContentEntityStorage class.
   + * See Drupal\node\Entity\Node and Drupal\node\NodeStorage for an example.
   

   Again...

   Please rewrap these two lines so that they go out to nearly 80 characters without going over.

   Also please make sure that the class namespace starts with \ in the second line.

   Thanks!

2. +++ b/core/modules/aggregator/src/FeedViewBuilder.php
   @@ -16,7 +16,7 @@
   + * View builder handler for aggregator feeds items.
   

   Please see previous review. Still not fixed.

   This should say:

   ... for aggregator feeds.

   NOT feeds items.

Thanks, Added suggestions.

Thanks! It all looks right now.

Committed/pushed to all three 8.x branches, thanks!