Remove non-storage logic from the storage controllers

1. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigStorageController.php
   @@ -292,18 +292,8 @@ protected function buildQuery($ids, $revision_id = FALSE) {
   +    $class = $this->entityInfo['class'];
   +    $class::attachLoad($this, $queried_entities, $revision_id);
   
   +++ b/core/lib/Drupal/Core/Entity/DatabaseStorageController.php
   @@ -354,19 +354,8 @@ protected function attachLoad(&$queried_entities, $load_revision = FALSE) {
   +    $class = isset($this->entityInfo['class']) ? $this->entityInfo['class']: $this->entityClass;
   +    $class::attachLoad($this, $queried_entities, $load_revision);
   

   We could probably move this into a parent class, not sure if we want to do it here, but I'm already adding a different implementation in a different issue, so this would help avoid code duplication. The weird class thing will go away there too.

2. +++ b/core/modules/node/lib/Drupal/node/Entity/Node.php
   @@ -79,6 +79,27 @@ public function getRevisionId() {
   +
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public static function preCreate(EntityStorageControllerInterface $storage_controller, array &$values) {
   +    // @todo Handle this through property defaults.
   +    if (empty($values['created'])) {
   +      $values['created'] = REQUEST_TIME;
   +    }
   +  }
   

   The @todo should actually be possible now. Can you try to add it to baseFieldDefinitions() ?

3. +++ b/core/modules/node/lib/Drupal/node/NodeGrantDatabaseStorage.php
   @@ -256,4 +256,14 @@ public function count() {
   +  public function deleteNodeRecords(array $nids) {
   +    db_delete('node_access')
   +      ->condition('nid', $nids, 'IN')
   

   I think we can use $this->database here.

4. The methods that you add should also allow us to kill EntityTestStorageController::create() as that needed $this->entityType.

+++ b/core/modules/system/tests/modules/entity_test/lib/Drupal/entity_test/Entity/EntityTest.php
@@ -136,6 +137,9 @@ public static function baseFieldDefinitions($entity_type) {
+      'settings' => array(
+        'default_value' => $entity_type,
+      ),

As discussed, this doesn't work, as we need to bundle to get the fields.

+++ b/core/lib/Drupal/Core/Entity/EntityStorageControllerBase.php
@@ -272,4 +277,26 @@ protected function invokeTranslationHooks(EntityInterface $entity) {
+   * Attaches data to entities upon loading.
+   *
+   * This will attach fields, if the entity is fieldable. It calls
+   * hook_entity_load() for modules which need to add data to all entities.
+   * It also calls hook_TYPE_load() on the loaded entities. For example
+   * hook_node_load() or hook_user_load(). If your hook_TYPE_load()
+   * expects special parameters apart from the queried entities, you can set
+   * $this->hookLoadArguments prior to calling the method.
+   * See Drupal\node\NodeStorageController::attachLoad() for an example.
+   *

This needs to be updated. The base class doesn't attach fields anymore and the things below aren't correct anymore.

This seems to be the only case where we move a hook invocation to the entity classes. Why?

+++ b/core/lib/Drupal/Core/Entity/Entity.php
@@ -642,9 +642,36 @@ public static function postDelete(EntityStorageControllerInterface $storage_cont
-  public static function postLoad(EntityStorageControllerInterface $storage_controller, array $entities) {
+  public static function postLoad(EntityStorageControllerInterface $storage_controller, array $entities, $revision_id = FALSE) {

I don't think it makes sense to invoke the entity load from here. The regular pattern is the storage controller invoking the hook + the entity class method later on. Are there any reasons to not follow the same pattern here?

E.g. that's what we do for preSave():

      $entity->preSave($this);
      $this->invokeFieldMethod('preSave', $entity);
      $this->invokeHook('presave', $entity);

Avoiding code duplication is not a reason for moving it to the entity class, we can add a method invokeLoadHook() on the storage controller also.

That's not a regular pattern, that's a stupid thing we did because invokeHook was dealing with fields.

It's the pattern. We can argue on whether it's stupid or not, sure. But even if it's stupid, it should be fixed for all places in a consistent manner unless there are good reasons for being inconsistent. However, I do not see any?

Imo, the goal of this issue was to move entity type postLoad() specifications to the entity classes, as we already do it for e.g. save. Whether we want to move general, not entity-type-specific storage logic to the entity class is another issue and needs to be discussed first. If you are insisting to not separate that discussion to its own issue, then okok - let's have the discussion here.

So let's clarify what we are talking about. What lives in the storage controllers what is not storage engine specific? I find the following:
a) invokeHook(), invokeTranslationHooks(), maybe invokeHookLoad() or so also
b) invokeFieldMethod(), invokeFieldItemPrepareCache()
c) cacheGet(), cacheSet()

Anything else? Going with that for now.

So, when we remove that general logic from the storage we could move it to the entity class, but I dislike adding that much of general purpose code in people's entity classes. We should try to keep them as simple as possible. I know they aren't really simple but it would be way worse if we'd move all that stuff on entity class as well.

So, instead what about moving that general-purpose logic to suiting services we partly already do, e.g. for getting entity field definitions or instantiating fields. We could move a) to entity manager, b) to the entity field manager (= field type manager?) and c) could be moved to a EntityCacheController/Handler/Service which we have been discussing doing for entity cache. Then, we could go and call this services from the entity class and have the logic removed from storage controllers, while entity classes do not become heavier. Thoughts on that?

Re-roll, let's see if this still works.

Ups, this should fix hopefully the fails and the exceptions.

Ok, so now I better understand what's going on here.

These are the options that we have to solve that load hook disagreement that we had in Prague:

1. Do what the patch does, move the hook invocation to the entity class. We can still do b) or c) additionally too.
2. Keep it in the storage controller, then we need to do one of a/b/c:

a) Make hookLoadArguments() on the entity class public so that we can call it from there.
b). Generalize the additional load hook arguments and pass along the bundles for all, including the generic hook_entity_load()
c). Drop the additional load hook arguments feature completely, unless there's a more useful use case than passing through the bundles. You need a loop and check every $entity *anyway*, as there could be multiple bundles and you only apply to one of those. The only thing you win is to be able to initially decide that no entities interest you, therefore saving a loop over them. The performance gain is tiny, and it's actually slower if you do apply to at least one entity, because you need to check twice. And you need more code, so it's hardly a DX improvement.

Thoughts? I actually kind of like c), although that would be an API change and I'd prefer 2 just because there's less conceptual change in this patch and we can open a follow-up to discuss and eventually move all hook invocations to the entity classes.

1. +++ b/core/lib/Drupal/Core/Entity/Entity.php
   @@ -340,9 +340,36 @@ public static function postDelete(EntityStorageControllerInterface $storage_cont
   +    // Call hook_TYPE_load(). The first argument for hook_TYPE_load() are
   

   This comment seems to stem from node-ages, just as the whole hook look arguments code. Do we still need this?
   After reading options of #40: I guess I like c)!

2. +++ b/core/lib/Drupal/Core/Entity/EntityInterface.php
   @@ -207,14 +207,17 @@ public static function preDelete(EntityStorageControllerInterface $storage_contr
   -   * Acts on loaded entities before the load hook is invoked.
   +   * Calls the entity load hooks.
   

   Now it's inconsistent with other post-* crud hooks, as said - whatever we do it should stay consisent.

1. Do what the patch does, move the hook invocation to the entity class. We can still do b) or c) additionally too.
2. Keep it in the storage controller, then we need to do one of a/b/c:

As discussed in Prague, having the entity taking care of invoking it's own hooks actually makes sense and I have to agree with chx that the increased size of entity class isn't that problematic, plus it doesn't look like being a lot of code to add anyway. -> So let's do 1)!

However, we really should do that for all hooks - so things stay consistent. So let's move invokeHook to the entity class and use it from there?

+++ b/core/lib/Drupal/Core/Entity/Entity.php
@@ -340,9 +340,36 @@ public static function postDelete(EntityStorageControllerInterface $storage_cont
+    foreach (\Drupal::moduleHandler()->getImplementations($entity_type . '_load') as $module) {

Is there a way to avoid accessing the global module handler? Maybe, get it from the passed storage controller, or better have the entity manager providing a helper for invoking entity hooks + get that via the storage controller?

- Re-rolled
- Hook load arguments dropped. Updated hook_node_load() documentation. The example might now look slightly more complicated, but the old one was kind of weird in that it checked but then still used all nids. Usually, you'd need that loop & check anyway IMHO.
- Tried to move invokeHook() but it's not that easy unless we make it a public hook. There are a few hooks that have no corresponding method on entities, e.g. delete revision (this should have one I think as we have save revision too) but not sure what to do about the translation hooks. Seems like they should also move to postSave(), but then we need to do the dataTable check there. Also not sure about the field invoke method helper method, should that move to Content entity base?

Where I'm getting at is that moving everything isn't trivial, so I'd rather not do it here. Either move on with this or keep load in the storage controller too for now. And open a follow-up issue to move all/the rest of it.

Attempt to fix those tests. The actual implementations show nicely that no real implementation even bothered to look at $types, some don't even define it (e.g. forum_node_load()) except of that node load test.

Re-roll. Removed the load hook tests for node and custom block completely. We have generic entity crud hook tests for various entity types and it's all unified now, no need for custom tests for that stuff now that $types is gone.

Either move on with this or keep load in the storage controller too for now. And open a follow-up issue to move all/the rest of it.

Yeah, let's go through that in a follow-up.

+  /**
+   * Attaches data to entities upon loading.
+   *
+   * @param $queried_entities
+   *   Associative array of query results, keyed on the entity ID.
+   * @param $revision_id_id
+   *   ID of the revision that was loaded, or FALSE if the most current revision
+   *   was loaded.

id_id

That's in there several times, best s/r to fix the s/r error.

+  /**
+   * {@inheritdoc}
+   */
+  public function deleteNodeRecords(array $nids) {
+    $this->database->delete('node_access')
+      ->condition('nid', $nids, 'IN')
+      ->execute();
+
+  }

Unneccessary empty line.

Else patch looks good.

+++ b/core/lib/Drupal/Core/Entity/EntityInterface.php
@@ -207,14 +207,17 @@ public static function preDelete(EntityStorageControllerInterface $storage_contr
    * @param array $entities
    *   An array of entities.
+   * @param int|bool $revision_id
+   *   ID of the revision that was loaded, or FALSE if the most current revision
+   *   was loaded.
    */
-  public static function postLoad(EntityStorageControllerInterface $storage_controller, array $entities);
+  public static function postLoad(EntityStorageControllerInterface $storage_controller, array $entities, $revision_id = FALSE);

The revision_id of which of the loaded entities? :)

This kills the weird $revision_id argument completely :)

It's used for exactly one thing, to figure out if we should load the fields specific revision or not. Which is something that we can now calculate automatically. We need to refactor the field stuff further to not require loaded entity objects and might need to re-introduce it there again, but that won't go through postLoad() anymore.

This has the additional benefit of removing the concept of a $revision_id/flag from EntityInterface and the storage controller base class, which don't know anything about revisions.

Also found some more postLoad() methods that I could kill. The only one that survived is comment, because that needs to happen *before* we map the data to entities. We'll get to that later...

I will never learn to at least do a simple syntax check before I upload broken patches...

Wow. that means we invoke the load hooks for config entities every time they're accessed, because we don't have a static cache. That's... crazy, need to do some profiling on how costly config loading is.

This should fix that, let's see if there are additional errors.

Would be too embarrassing to post an interdiff.

This look like an awesome move overall.

I'm just a bit worried about the amount of stuff that's dumped into the postLoad() step, either in the controller or the entity class. It makes it difficult to figure out what happens in which order, you need to unwind the class hierarchy and the order of the parent:: calls.
Also, having field values loaded in a step called postLoad() seems a bit weird/counterintuitive ?

I didn't really follow the issue, so feel free to discard this...

You are perfectly right, loading field values should not be in there nor converting things into entities, and it will have to move around so that we can load the field values before we create the entity objects. The way it works right now is a left-over of when we had the NG storage controller, that had to pick what it changed to not having to duplicate too much code.

I need to create an issue, but there's already a bullet point for this in the meta issue.

Thanks @Berdir, makes sense.
Not fully clear to me what's for this issue and what's for followups / side issues then ?

Ah, now I found what @chx was referencing to. There was one piece where the actual revision id was used. And it switched back and forth between a $load_revision and $revision_id, weird stuff. And it had a fall back in place anyway. So I just removed all that code and it looks like the entity tests are still passing.

It is still a bit weird, because we conditionally convert $entities from arrays to objects if there is *no* data table, and the other code only runs if there is a revision data table, so it's fairly sure that it doesn't run when $entities is an array of entity objects, but to be sure, I still added an is_object() check there.

@yched: Not sure, I think we shouldn't do more here other than fixing the tests, unless you see something. This basically just involves moving things from entity storage to entity classes, anything that's internal refactoring should imho be done in a follow-up issue, unless directly necessary for this issue.

So we need to pass $entities around by reference.

IMHO it's wrong that the store returns differently sorted entities, but that's a different topic.

Some nitpicks that I noticed, will re-roll myself tonight or so unless someone else has time first.

1. +++ b/core/lib/Drupal/Core/Entity/EntityInterface.php
   @@ -207,14 +207,14 @@ public static function preDelete(EntityStorageControllerInterface $storage_contr
    
      /**
   -   * Acts on loaded entities before the load hook is invoked.
   +   * Calls the entity load hooks.
       *
   

   Not sure about this change, should probably just have the before load hooks invoked part removed. The important part is what it can be used to do and when it's called, not what the default implementation does.

2. +++ b/core/lib/Drupal/Core/Entity/EntityStorageControllerBase.php
   @@ -152,4 +157,15 @@ protected function invokeHook($hook, EntityInterface $entity) {
   +   *
   +   * @param $queried_entities
   +   *   Associative array of query results, keyed on the entity ID.
   

   Missing an array

3. +++ b/core/lib/Drupal/Core/Entity/EntityStorageControllerInterface.php
   @@ -153,4 +153,18 @@ public function save(EntityInterface $entity);
   +   * Returns the current entity type.
   +   *
   +   * @return string
   +   */
   +  public function entityType();
   +
   +  /**
   +   * Returns the current entity info.
   +   *
   +   * @return string
   +   */
   +  public function entityInfo();
   

   Missing descriptions of the @return.

Documentation updated, patch still applies

Looks like this becomes a great clean-up :-) Here a review:

1. I'm not sure whether this has been discussed already, but I'm baffled by the inconsistency of postLoad() invoking entity hooks and others like postSave() don't. Why not invoke entity-load hooks in the storage controller also?
   Also, if it would stay in the entity class the interface docs should probably define that so implementers know.

2. +++ b/core/lib/Drupal/Core/Entity/EntityStorageControllerInterface.php
   @@ -150,7 +150,24 @@ public function save(EntityInterface $entity);
   +   * Returns the current entity type.
   

   I'm confused by "current" here. What does this mean or refer to?

3. +++ b/core/lib/Drupal/Core/Entity/EntityStorageControllerInterface.php
   @@ -150,7 +150,24 @@ public function save(EntityInterface $entity);
   +  /**
   +   * Returns the current entity info.
   +   *
   

   Same here.

4. +++ b/core/modules/node/lib/Drupal/node/Entity/Node.php
   @@ -77,6 +77,16 @@ public function getRevisionId() {
   +  public static function preCreate(EntityStorageControllerInterface $storage_controller, array &$values) {
   +    // @todo Handle this through property defaults.
   

   This does not call parents while others do. Even if it's empty, it might not stay empty?

5. +++ b/core/modules/node/lib/Drupal/node/Entity/Node.php
   @@ -149,6 +159,13 @@ public static function preDelete(EntityStorageControllerInterface $storage_contr
   +  public static function postDelete(EntityStorageControllerInterface $storage_controller, array $nodes) {
   ...
   +  }
   

   Same here.

6. +++ b/core/modules/system/tests/modules/entity_test/lib/Drupal/entity_test/Entity/EntityTest.php
   @@ -95,6 +96,15 @@ protected function init() {
   +  public static function preCreate(EntityStorageControllerInterface $storage_controller, array &$values) {
   +    if (empty($values['type'])) {
   ...
   +    }
   +  }
   

   again, call the parent?

Ok, moved the hooks over, calling parents and removed "current". Let's see if this works.

dreditor just ate my review for breakfast ;-/ anyway, here the short version:

+  public static function postLoad(EntityStorageControllerInterface $storage_controller, array &$entities) {
+    $storage_controller->loadCategories($entities);
+    parent::postload($storage_controller, $entities);
+  }

All postLoad() overrides should use default ordering now as there is no reason left not to, i.e. call parent first.

-   * Overrides Drupal\Core\Entity\DatabaseStorageController::attachLoad().
+   * Overrides Drupal\Core\Entity\DatabaseStorageController::postLoad().
    */
-  function attachLoad(&$queried_users, $load_revision = FALSE) {
+  function postLoad(array &$queried_users, $revision_id = FALSE) {

$revision_id should be gone here, not ?

-      $this->attachLoad($queried_entities, $revision_id);
+      $this->postLoad($queried_entities, $revision_id);

Again.

+   *
+   * @todo Don't call parent::postLoad() at all because we want to be able to
+   * control the entity load hooks.

deprecated comment

Ok, cleaned that up too. Checked with @amateescu, he said it's ok to remove the @todo If I understood correctly :)

Found two more - else this is RTBC imo.

+++ b/core/lib/Drupal/Core/Entity/FieldableDatabaseStorageController.php
@@ -372,11 +363,11 @@ public function loadRevision($revision_id) {
-      $this->attachLoad($queried_entities, $revision_id);
+      $this->postLoad($queried_entities, $revision_id);

Another $revision_id left over.

1. +++ b/core/modules/menu_link/lib/Drupal/menu_link/Entity/MenuLink.php
   @@ -576,6 +576,42 @@ public function postSave(EntityStorageControllerInterface $storage_controller, $
   +    }
   +    parent::postLoad($storage_controller, $entities);
   

   Should move up also.

Ok, cleaned that up as well.

Thanks!

Thanks, follow-up issues: #2137801: Refactor entity storage to load field values before instantiating entity objects and #2137807: Move entity storage related hooks to pre/Post methods on entity classes.

70: storage-logic-2095283-70.patch queued for re-testing.

Rerolled after #2076445: Make sure language codes for original field values always match entity language regardless of field translatability.

Probably moot, since this is already RTBC, but just in case, tagging as a beta target, since it touches on data storage issues.

74: storage-logic-2095283-74.patch queued for re-testing.

Re-roll after the aggregator categories were removed.

RTBC no longer triggers testbot?

Passed, back to RTBC.

79: storage-logic-2095283-79.patch queued for re-testing.

Tagging and setting correct priority after discussion with @catch

That was just a re-roll, so setting back to RTBC.

Committed 65be82a and pushed to 8.x. Thanks!

Amazing work, all! :)

There's just one detail I wondered about: Some postLoad() overrides are enhancing the queried entities with entity specific data, and they call into parent::postLoad() to attach fields and invoke module hooks.

However, the position of when parent::postLoad() is called and in turn, the point in time when module hooks are invoked, is not consistent for all entity types. Most of them are calling parent::postLoad() as the first operation, before any additional operations are performed. As a consequence, module hooks will not be able to act on additional data supplied by the entity class, or am I missing something?

It looks like parent::attachLoad() was always invoked as a last step previously, so that module hook implementations always had the full data available?

Is that something we're going to polish via #2137807: Move entity storage related hooks to pre/Post methods on entity classes, or should we open a separate issue for that?

@sun: Just discussed this with Xano as well in IRC too. postLoad() right now is still weird, because FieldableDatabaseStorageController still relies on something that dates back to a hack introduced in the NG storage controller to avoid duplicating the load method. Therefore, that specific implementation receives stdClass objects and converts them to entities within that method, before calling the parent.

I will work on this in #2137801: Refactor entity storage to load field values before instantiating entity objects.

Created a change notice about the removed $types argument/additional load arguments support: https://drupal.org/node/2166881

There is afaik no specific change notice about the other moved methods, only a short note in https://drupal.org/node/1400186, added this issue as a reference in that as well (because there aren't enough yet :)). I intend to write this down in the documentation when we're done with the changes.