EntityStorageBase::loadMultiple returns unwanted entities when the static cache is warm

Nice find, @blazey!

1. +++ b/core/lib/Drupal/Core/Entity/EntityStorageBase.php
   @@ -276,8 +277,11 @@ public function loadMultiple(array $ids = NULL) {
   +    if (!$flipped_ids || !empty($ids)) {
   

   Let's use the same conditional logic that we have below in this method: if ($ids === NULL || $ids) {. It will make the code easier to parse visually :)

   And while we're at it, we should change the first sentence of the comment above this check to something like:

   "Try to gather any remaining entities from a 'preload' method."

   We should also remove the IDs that were preloaded from the list of entities that need to be loaded, with something like this inside the if (!empty($preloaded_entities)) { check:

         // If any entities were preloaded, remove them from the IDs still to load.
         $ids = array_keys(array_diff_key($flipped_ids, $entities));
   

2. +++ b/core/modules/workspaces/tests/src/Kernel/WorkspaceIntegrationTest.php
   @@ -322,6 +323,10 @@ public function testWorkspaces() {
   +    foreach (Node::loadMultiple([1]) as $entity) {
   +      $this->assertEquals(1, $entity->id(), '::loadMultiple returns entities with the provided id.');
   +    }
   

   In order to be consistent with the rest of the assertions from this test class, let's assert this inside of assertEntityLoad(), between checking loadMultiple() and loadUnchanged():

       // Check loading entities one by one.
       foreach ($expected_default_revisions as $expected_default_revision) {
         $entity_id = $expected_default_revision[$id_key];
         $entities = $this->entityTypeManager->getStorage($entity_type_id)->loadMultiple([$entity_id]);
         $this->assertCount(1, $entities);
         $this->assertEquals($expected_default_revision[$revision_key], $entities[$entity_id]->getRevisionId());
         $this->assertEquals($expected_default_revision[$label_key], $entities[$entity_id]->label());
         $this->assertEquals($expected_default_revision[$published_key], $entities[$entity_id]->isPublished());
       }
   

Re-rolled the patch against latest 8.7.x

Fixed the points from #8.

Reviewed and tested in an actual use case. Looks good!

Very very nice find, @blazey and thank you folks for solving it!

----

Would it be possible to add a core test for the core functionality additionally to the one provided for the workspaces module? Nevertheless this is change in the entity API and therefore I think it should have its own test independent from the workspaces module as well. I think that \Drupal\KernelTests\Core\Entity\ContentEntityStaticCacheTest would be the perfect place for such a test.

I am not changing the issue status, because the patch looks good. I even think that it might be solving the problem leading to the only fail in #2620980-135: Add static and persistent caching to ContentEntityStorageBase::loadRevision().

Just looking at loadMultiple do we want to clean it up in this issue or let's add more warts to it and clean it up in a second issue? The method should start with if ($ids === []) return [];. And then:

1. Move $flipped_ids inside the if ($ids) {, the exact same check is run twice on subsequent lines (add else for FALSE)
2. if (!empty($preloaded_entities)) which is double superflous: the empty() is unnecessary because the variable is always set and the check whether there are any entities unnecessary because both calls inside the if are fine with empty arrays and $preloaded_entities is always an array according to the preload typehint.
3. if ($ids === NULL || $ids) { is now unnecessary since the only case when this if doesn't happen is for empty array $ids which we aborted early.
4. if (!empty($queried_entities)) { because of our previous move, $queried_entities is now always set, you can drop another !empty()

The patch with the above abort-early optimization can lose $preloaded_entities = []; , if ($ids === NULL || $ids) {. And overall, the whole thing becomes much easier to read.

@Charlie ChX Negyesi, since this is a bugfix, having a small patch here is preferable so I would rather do the cleanup in a followup issue.

Hm, why is this built against 8.7.x and not 8.8.x? @amateescu did not explain that when he moved it to 8.7.x in July.

Because I thought the common practice is to target the lowest branch in which a bug fix should be committed, but I guess I was wrong :)

Committed c86caa0 and pushed to 8.8.x. Thanks!

Agreed it's worth opening a follow-up for #17.

Thanks, @catch! As mentioned above, this should be backported to 8.7.x :)

Cherry-picked to 8.7.x too.