Replace Node overview topic and Node API topic with Entity Hooks topic

Uff. Will try to review that, some feedback on those questions for now...

b) AFAIK, the only entity hook that receives the type (I assume you mean entity type) is hook_entity_load(), because those receive an array of entities and getting the type out of them is bit more involving. The others do not.

c) hook_node_validate() still exists ('search for 'node_validate') and it's only for nodes. hook_node_submit() too, I think it should not but not the topic of this issue. hook_node_prepare_form() is the ENTITY_TYPE version of hook_entity_prepare_form(). See below.

d) hook_entity_view_display_alter() (and form), do exist, but they are not really part of CRUD, they allow to alter the display configuration for viewing and editing entities. Same for hook_entity_prepare_form() that does exist, (called in EntityForm::init()) and allows to do stuff before the add/edit form is displayed for an entity.

Wrong issue.

Ups, wrong issue, review incoming...

1. +++ b/core/modules/block/block.api.php
   @@ -86,6 +86,8 @@
     *
     * @see hook_block_view_BASE_BLOCK_ID_alter()
     * @ingroup block_api
   + *
   + * @ingroup entity_crud
     */
   

   Why the space between the other @ingroup above?

   Note that the block hooks are special because they are not covered by the generic code, blocks use a entity view builder but completely override the code there and do their own thing, so not sure we should even add it to the entity documentation.

2. +++ b/core/modules/field/field.api.php
   @@ -254,6 +254,8 @@ function hook_field_info_max_weight($entity_type, $bundle, $context, $context_mo
     *   The field as it is pre-update.
   + *
   + * @ingroup entity_crud
     */
    function hook_field_config_update_forbid(\Drupal\field\FieldConfigInterface $field, \Drupal\field\FieldConfigInterface $prior_field) {
   

   Similar, this is a super special hook for configurable fields, not sure this makes sense to add here?

3. +++ b/core/modules/node/node.api.php
   @@ -381,147 +283,6 @@ function hook_node_grants_alter(&$grants, \Drupal\Core\Session\AccountInterface
   - * This hook is invoked from $node->save() after the database query that will
   - * insert the node into the node table is scheduled for execution, and after
   - * field values are saved.
   - *
   - * Note that when this hook is invoked, the changes have not yet been written to
   - * the database, because a database transaction is still in progress. The
   - * transaction is not finalized until the save operation is entirely completed
   - * and $node->save() goes out of scope. You should not rely on data in the
   - * database at this time as it is not updated yet. You should also note that any
   - * write/update database queries executed from this hook are also not committed
   - * immediately. Check $node->save() and db_transaction() for more info.
   

   This is the kind of documentation that we need to have on the generic entity hooks, reminder for myself to check that.

4. +++ b/core/modules/node/node.api.php
   @@ -708,6 +401,7 @@ function hook_node_update_index(\Drupal\node\NodeInterface $node, $langcode) {
    
   +
    /**
   

   Bogus empty newline.

5. +++ b/core/modules/node/node.api.php
   @@ -767,82 +461,6 @@ function hook_node_submit(\Drupal\node\NodeInterface $node, $form, &$form_state)
   - *
   - * When $view_mode is 'rss', modules can also add extra RSS elements and
   - * namespaces to $node->rss_elements and $node->rss_namespaces respectively for
   - * the RSS item generated for this node.
   - * For details on how this is used, see node_feed().
   

   The rss stuff is node specific, wondering what happend to that.

6. +++ b/core/modules/node/node.api.php
   @@ -951,6 +537,8 @@ function hook_node_type_delete(\Drupal\node\NodeTypeInterface $type) {
   + *
   + * @ingroup entity_crud
     */
    function hook_node_links_alter(array &$links, NodeInterface $entity, array &$context) {
   

   node specific, so not sure about that @ingroup?

7. +++ b/core/modules/node/src/Tests/NodeCreationTest.php
   @@ -18,7 +18,8 @@ class NodeCreationTest extends NodeTestBase {
       *
   -   * Enable dummy module that implements hook_node_insert() for exceptions.
   +   * Enable dummy module that implements hook_ENTITY_TYPE_insert() for
   +   * exceptions.
   

   When we change that anyway, would make more sense to refer to the actual implementation? This change makes it even harder to find the actual code that is doing this.

8. +++ b/core/modules/system/core.api.php
   @@ -288,12 +287,14 @@
   + *   - view_builder: For content entities, define a class that implements
   + *     \Drupal\Core\Entity\EntityViewBuilderInterface (usually extending
   + *     \Drupal\Core\Entity\EntityViewBuilder), to display a single entity.
   

   Some configuration entities use a view builder too, the interface is not specific to content entities, but the default class has content entity specific code, that can be overwritten.

9. +++ b/core/modules/system/core.api.php
   @@ -339,7 +345,25 @@
   - * @section load_query Loading and querying entities
   + * @section bundle Defining a content entity bundle
   + * For content entity types that use bundles, such as Node (bundles are content
   

   bundle() is defined on EntityInterface right now, although that doesn't really make much sense, but it's used in a few cases for something else than configurable fields.

10. +++ b/core/modules/system/core.api.php
    @@ -389,6 +413,25 @@
    + * // If the entity supports translation, load the translation.
    + * $entity = $entity->getTranslation($language->id);
    + * $build = $viewer->view($entity, 'view_mode_name', $language->id);
    + * // $build is a render array.
    + * $rendered = drupal_render($build);
    

    I'm not sure if this part is also correct.

    Might also make sense to refer to the routing or include an example for _entity_view: 'user' or so, which is probably the more common use case rather than calling it manually.

11. +++ b/core/modules/system/core.api.php
    @@ -402,25 +445,17 @@
    + * The default entity access controller invokes two hooks while checking
    + * access on a single entity: hook_entity_access() is invoked first, and
    + * then hook_ENTITY_TYPE_access() (where ENTITY_TYPE is the machine name
    + * of the entity type).
    

    It might be worth mentioning here that

    a) modules should in most cases not overwrite access() but checkAccess().
    b) It's also important to mention that the check method acts as a default logic if no hook overrides that default behavior.

    There's also a separate method and hooks for checking create access.

    The current behavior is discussed in #2204363: [sechole] Returning TRUE from hook_entity_access()/hook_ENTITYTYPE_access() must not bypass EntityAccessController::checkAccess() because it's a bit weird and resulted in security issues in block.module, which have been solved in a different way in the meantime.

12. +++ b/core/modules/system/core.api.php
    @@ -402,25 +445,17 @@
    + * The Node entity type has a complex permission system, which developers can
    + * interact with. This is described in the
    + * @link node_access Node access topic. @endlink
    

    permission isn't really the word I'd use here, access grant(s) system maybe?

13. +++ b/core/modules/system/entity.api.php
    @@ -11,6 +11,218 @@
    + * Entity create, read, update, and delete (CRUD) operations are performed by
    + * entity storage controllers; see the
    

    We're still hoping to get rid of controller here, can we avoid that word? entity storage classes/implementations or so?

14. +++ b/core/modules/system/entity.api.php
    @@ -11,6 +11,218 @@
    + * \Drupal\Core\Entity\EntityStorageBase().
    

    no ()

15. +++ b/core/modules/system/entity.api.php
    @@ -11,6 +11,218 @@
    + *
    + * For content entities, viewing and rendering are handled by a view builder
    + * controller; see the @link entity_api Entity API topic @endlink for more
    

    same, just view builder.

16. +++ b/core/modules/system/entity.api.php
    @@ -11,6 +11,218 @@
    + * Entity editing (including adding new entities) is handled by entity form
    + * controllers; see the @link entity_api Entity API topic @endlink for more
    

    And again, just entity forms.

    Also, entity forms are also used for creating, deleting (confirm forms) and other operations.

17. +++ b/core/modules/system/entity.api.php
    @@ -11,6 +11,218 @@
    + * - $viewer in the code examples is assumed to be an entity view builder
    + *   controller. See the @link entity_api Entity API topic @endlink for
    

    I think we usually use $view_builder? it's not a viewer it's something that builds a view/render array.

18. +++ b/core/modules/system/entity.api.php
    @@ -11,6 +11,218 @@
    + * // Set up the entity properly.
    + * $entity->save();
    

    That comment reads weird, not sure if it should be above the create, but it doesn't set up anything, it just saves it?

    Maybe instead of saving explicitly explain that this creates a new entity in memory that has not been saved yet?

    There's also $EntityClass::create() now. mostly meant for functions and test code.

19. +++ b/core/modules/system/entity.api.php
    @@ -11,6 +11,218 @@
    + * - Entity and fields are loaded from storage.
    + * - postLoad() is called on the entity class, passing in all of the loaded
    + *   entities.
    + * - hook_entity_load()
    + * - hook_ENTITY_TYPE_load()
    

    fields seems like an implementation detail, only the database storage controller differs between the two things but a) everything is a field for content entities and b) other backends like MongoDB will just load "the entity".

    Also, #597236: Add entity caching to core will make this a bit more complicated for content entities, as they will introduce a new hook (that already follows the ENTITY_TYPE pattern).

20. +++ b/core/modules/system/entity.api.php
    @@ -11,6 +11,218 @@
    + * @section update Update operations
    + * To update an entity, load it (see @ref load), modify its values, and
    + * save it (see @ref save).
    

    Update should probably be after save?

    There is also no update as far as the API is concerned (the only exception are the separate hooks), just saving of new and existing entities. So maybe just merge that into save, which you more or less already did?

21. +++ b/core/modules/system/entity.api.php
    @@ -11,6 +11,218 @@
    + * - preSave() is called on the entity object.
    

    Those methods are also called on the field objects, they replace the old hook_field_* hooks. Both on the field item list objects and the field items (field item list by default does nothing but pass it through to all items)

22. +++ b/core/modules/system/entity.api.php
    @@ -11,6 +11,218 @@
    + * - The base entity information and fields are saved to storage.
    

    Same here, just "entity values are saved to the storage"?

    Also, storage has a double-meaning as both the entity storage class and the actual storage (mysql, mongodb, ..)

23. +++ b/core/modules/system/entity.api.php
    @@ -11,6 +11,218 @@
    + * - Field configuration preSave(): hook_field_config_update_forbid()
    

    ah, that's why you added it, still think it's enough to just refer it here as an example.

24. +++ b/core/modules/system/entity.api.php
    @@ -11,6 +11,218 @@
    + * - Entity bundle postSave(): hook_entity_bundle_create() or
    + *   hook_entity_bundle_rename() as appropriate
    

    Not sure that this makes sense, "Entity bundle" is not an entity type, so maybe "entity types that are used as bundles" ?

25. +++ b/core/modules/system/entity.api.php
    @@ -11,6 +11,218 @@
    + * - hook_entity_form_display_alter()
    

    this is specific to content entities.

26. +++ b/core/modules/system/entity.api.php
    @@ -11,6 +11,218 @@
    + * Some entities have different methods of deleting. For instance, user
    + * accounts can be canceled, which involves more than just deleting a user
    + * entity. This process invokes hook_user_cancel().
    

    Not sure this makes sense.

    Canceling a user is a separate process that can or can not result in the entity being deleted in the end (based on how it's configured). If configured to delete users, then it still does a "normal delete" at the end.

    => not sure this makes sense here, as users are the only example that I know does something like that.

27. +++ b/core/modules/system/entity.api.php
    @@ -11,6 +11,218 @@
    + * - Revision is loaded.
    

    This is something that is currently apparently not documented on it's own?

28. +++ b/core/modules/system/entity.api.php
    @@ -11,6 +11,218 @@
    + * // If the entity supports translation, load the translation.
    + * $entity = $entity->getTranslation($language->id);
    + * $build = $viewer->view($entity, 'view_mode_name', $language->id);
    

    wondering where you found that example, already mentioned it above I think, but you don't have to both path the language along and get the translation for it.

    If you want to render in the current language, don't pass anything in. If you want to render a specific language, pass in the langcode, it will get the right translation automatically.

    See EntityViewBuilder::viewMultiple()

29. +++ b/core/modules/system/entity.api.php
    @@ -281,16 +527,47 @@ function hook_entity_load($entities, $entity_type_id) {
    + * @param array $entities
    + *   The entities keyed by entity ID.
    + *
    + * @ingroup entity_crud
    + * @see hook_entity_load()
    + */
    +function hook_ENTITY_TYPE_load($entities) {
    

    Should we update new code to EntityInterface[].

    Which actually points out a problem with this approach.

    for hook_node_create(), for example, we type hint with NodeInterface, not EntityInterface, we can't really document that here, unless you want to use a fake example interface.

30. +++ b/core/modules/system/entity.api.php
    @@ -281,16 +527,47 @@ function hook_entity_load($entities, $entity_type_id) {
    +function hook_ENTITY_TYPE_presave(Drupal\Core\Entity\EntityInterface $entity) {
    +  $entity->changed = REQUEST_TIME;
    +}
    

    Bad example, because this is now handled automatically for content entity changed (and created) fields. Just copied I know, but still sad to copy bad examples around.

31. +++ b/core/modules/system/entity.api.php
    @@ -298,6 +575,9 @@ function hook_entity_presave(Drupal\Core\Entity\EntityInterface $entity) {
     function hook_entity_insert(Drupal\Core\Entity\EntityInterface $entity) {
    

    THis is where I'd expect the removed information on hook_node_insert() to live now..

32. +++ b/core/modules/system/src/Tests/Entity/EntityCrudHookTest.php
    @@ -20,8 +20,10 @@
    - * As well as all type-specific hooks, like hook_node_insert(),
    - * hook_comment_update(), etc.
    + * Also tested are type-specific hooks, like hook_ENTITY_TYPE_insert(),
    + * hook_ENTITY_TYPE_update(), etc.
    

    This reads a bit confusing, like as an example vs. ENTITY_TYPE as generic pattern. I understand it's meant as insert/update as examples of the full set, but that's not how I read it first.

33. +++ b/core/modules/user/user.api.php
    @@ -97,8 +22,8 @@ function hook_user_delete($account) {
      * This hook is NOT invoked for the 'user_cancel_delete' account cancellation
    - * method. To react to that method, implement hook_user_predelete() or
    - * hook_user_delete() instead.
    + * method. To react to that method, implement hook_ENTITY_TYPE_predelete() or
    + * hook_ENTITY_TYPE_delete() instead.
    

    Should possibly at least say something like generic_hook for user entities here too, as you did elsewhere?

#10/#28: Ah yes, I see, search is the only example that hardcodes a specific language when rendering the entity (and views, depending on the configuration).

#31: That sounds like a good idea, highlighting what is inside the transaction and what's outside in the flow there. #1729812: Separate storage operations from reactions is related, which might move the hooks out of the transactions and create new ones inside.

1. +++ b/core/modules/system/core.api.php
   @@ -417,21 +422,23 @@
   + * Once you have a loaded entity and a viewer, if for some reason you need to
   

   still a viewer reference here.

2. +++ b/core/modules/system/core.api.php
   @@ -448,12 +455,17 @@
     *
   + * If you are overriding
     * @see i18n
   

   I think that shouldn't be here ;)

I think this has sat here long enough, but unfortunately when I went to commit it, it no longer applied.

Yeah, let's do this, certainly a huge improvement.

Will likely conflict with the menu link critical, as that is certainly messing with those hook as it replaces that entity with a new one, but it's removing something that the other issue will not have to re-add anymore, so I think that's OK.

Hm. Indeed, they do conflict and #2256521: [META] New plan, Phase 2: Implement menu links as plugins, including static admin links and views, and custom links with menu_link_content entity, all managed via menu_ui module is a 600K patch. :\ Don't really want to break that one unless I get some kind of confirmation from pwolanin, etc. that it's trivial to re-roll around conflicts. (And it might be; afaik they're working in a sandbox where they can just rebase.)

I left pwolanin a tell to chime in here.

This patch conflicts with HEAD

I did talk about this with @pwolanin already, to make him aware of the impact on the renamed/new entity type in that issue, and he wasn't troubled by this as it should only conflict on stuff they remove.. they're used to merging stuff ;)

Also, this is a big patch that conflicts frequently too, so would be nice to get it in.

Will do a quick re-roll later today if nobody else has time first.

Only one context conflict in EntityCrudHookTest because a @group Entity was added there.

Committed 5ffb1d3 and pushed to 8.x. Thanks!