Add a hook_entity_preload() for modules that need to load a different revision than the default one

Hi, can you update the issue summary to indicate why this is preferred over the implementing hook checking $entity->isDefaultRevision()?

I assume there is a reason, e.g. performance or cache or similar. Its not clear why a new hook is preferred.

Couple of minor observations - thanks

1. +++ b/core/lib/Drupal/Core/Entity/EntityStorageBase.php
   @@ -308,6 +309,31 @@ protected function postLoad(array &$entities) {
   +    foreach ($this->moduleHandler()->getImplementations('entity_default_revision_load') as $module) {
   +      $function = $module . '_entity_default_revision_load';
   +      $function($entities, $this->entityTypeId);
   ...
   +    foreach ($this->moduleHandler()->getImplementations($this->entityTypeId . '_default_revision_load') as $module) {
   +      $function = $module . '_' . $this->entityTypeId . '_default_revision_load';
   +      $function($entities);
   

   Any reason not to call ModuleHandlerInterface->invokeAll()?

   Or failing that ModuleHandlerInterface->invoke()

2. +++ b/core/lib/Drupal/Core/Entity/entity.api.php
   @@ -903,6 +903,46 @@ function hook_ENTITY_TYPE_storage_load(array $entities) {
   + * The results of this hook will not be cached.
   

   Can you elaborate more on this?

@amateescu yes 😉

1. +++ b/core/lib/Drupal/Core/Entity/EntityStorageBase.php
   @@ -287,6 +299,51 @@ public function loadMultiple(array $ids = NULL) {
   +    // Call hook_entity_preload().
   +    $preload_ids = $ids ?: [];
   +
   +    $preload_entities = $this->moduleHandler()->invokeAll('entity_preload', [$preload_ids, $this->entityTypeId]);
   

   Ubernit: move the comment down to precede the actual line calling hook_entity_preload().

2. +++ b/core/lib/Drupal/Core/Entity/EntityStorageBase.php
   @@ -287,6 +299,51 @@ public function loadMultiple(array $ids = NULL) {
   +      // If we had to load all the entities ($ids was set to NULL), get an array
   +      // of IDs that still need to be loaded.
   

   Is this tested?

   Also, does this mean $storage->loadMultiple() will load all entities without going through hook_entity_preload()?

#15.2 The hook is called when you do loadMultiple(), that's exactly what this code is about. Imagine you have 10 nodes, and 2 of them have a workspace association and you load them, then the storage needs to load the other 8 afterwards. Means an additional query for that but we can't do much about that. Don't know about test coverage.

This looks pretty good to me.

Was wondering about the order of arguments for hook_entity_preload(), seems a bit strange to have $entity_type_if afterwards but that's consistent with hook_entity_load().

Also, we might want to profile this. The new hook is called for all entities, not just content entities, so that's quite a few additional hooks being called I think. What I'm wondering is if there's really a use case for this or is it something we should only support on content entities?

For example, the Workspace module in core swaps the default revision with a workspace-specific revision, if one exists.

So using the storage load method, which loads an entity in its default revision leads in that case to $storage->load($id)->isDefaultRevision() === FALSE?

Additionally if you do this then it becomes impossible to load the entity in its default revision.

Moreover can this work together with #2620980-109: Add static and persistent caching to ContentEntityStorageBase::loadRevision() .2 :

Couldn't we instead - whenever we put a revision in the revision static cache check if it is the default revision and then populate the entity static cache directly?

?

Ok, then I understand. I am afraid that this might introduce some bugs when used, so it will be more of a feature - use it with caution.

+++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
@@ -961,13 +1000,21 @@ public function loadUnchanged($id) {
+    $preloaded_entities = $this->preLoad($ids);

I think it would be useful to pass the information, that now we are loading the unchanged entity. This will be needed when #2620980: Add static and persistent caching to ContentEntityStorageBase::loadRevision() is ready, as it introduces entity revision cache and a loadRevisionUnchanged method. But knowing this I think it would be best to provide this information to the hook implementations here instead of doing it later, so that the adjustment will be as easy as possible.

I've meant that we have to pass the information about loading the unchanged entity to the hook implementations. So in your case probably you'll have to load the revision through loadRevisionUnchanged instead through loadRevision.

When we add a static cache for entity revisions to the storage, then loadRevision will be returning the entities from that static cache and not like now always new unchanged objects.

If there is an implemented hook_entity_preload, which is preloading an entity by loading it through loadRevision then we'll always get the entity revision from the static cache.

Later during the saving process we need the unchanged entity and we use loadUnchanged to retrieve it. Now the implemented hook_entity_preload should not return the entity revision from the static cache, but a new unchanged object. If the hook implementation returns the entity revision from the static cache then you'll be unable of detecting changes because e.g. CEB::hasTranslationChanges will be comparing the same entity object with itself. So inside CEB::hasTranslationChanges $this === $original will evaluate to TRUE, but it shouldn't. In order to solve this problem implementations of hook_entity_preload should have the information whether they are being called from load or from loadUnchanged and in the second case load the unchanged entity revision instead the one from the static cache. For this to happen we need to pass one more parameter $unchanged to the hook implementations.

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   @@ -453,6 +453,44 @@ public function finalizePurge(FieldStorageDefinitionInterface $storage_definitio
   +          ->condition($this->entityType->getKey('id'), array_keys($entities), 'NOT IN')
   

   Shouldn't this have an accessCheck(FALSE)?

2. +++ b/core/lib/Drupal/Core/Entity/EntityStorageBase.php
   @@ -272,6 +272,14 @@ public function loadMultiple(array $ids = NULL) {
   +    // Gather entities from a 'preload' hook. This hook can be used by modules
   

   If preLoad is only in ContentEntityStorageBase shouldn't we override this method to add this logic instead of adding it here? Or if it needs to be here, add a comment?

Did some profiling as well on a site that is *not* actually using workspaces and despite 108 additional calls to invokeAll(), the time difference was for that case only 0.3%. It also reported almost 5% difference on the IO wait time, which this shouldn't influence in any way, so clearly the variance is much higher than the difference that is reported here. And that doesn't even include the considerable function call overhead of active profiling. So I think the performance impact is not an issue with the current patch.

The only thing I'd like to put up for discussion is whether we really need the entity type specific hook. Are there really enough use cases to make it worth doubling the additional invokeAll() calls? So far all CRUD hooks are always invoked for both variants, but we do have examples of entity hooks where we don't do that, e.g. hook_entity_view_mode_alter(). I don't have a strong opinion myself, it depends on what we consider to be more important I guess: consistency with other entity hooks vs. performance.

Setting to RTBC as that seems like a question for @catch or a framework manager.

+1 on dropping the entity-type specific hook here. I don't have any further objections, but also the tests haven't finished running yet so not committing right now.

Committed 8293016 and pushed to 8.7.x. Thanks!

We added a new hook so we should add a change notice for this.

We just encountered a small problem while working on the patch for the revision cache in #2620980-122: Add static and persistent caching to ContentEntityStorageBase::loadRevision(). Probably someone could take a look and sign-off the change there or we do it in a follow-up of the current issue so that the other patch keeps the main focus on the revision cache?

The problem there is that we would require that the preLoad occurs before the retrieval from the static cache, otherwise under some circumstances the entity in the default revision might already be present in the entity cache and therefore the preLoad hooks will not be executed and thus the entity will not be exchanged in "preLoad" anymore.