Problem/Motivation

The EntityStorageBase::loadMultiple() loads entities by ID and returns them in the same order as the IDs that were passed to load them. This functionality is, however, not tested.

We need the same to apply also to the ContentEntityStorageBase::loadMultipleRevisions() to stay consistent and be able to ensure that if we query for revisions in a certain order, the loaded revisions stay in that same order.

Proposed resolution

  • Maintain the revision order on load
  • Test the loadMultiple() and loadMultipleRevisions() order logic

Remaining tasks

Tests and fix.

User interface changes

None

API changes

ContentEntityStorageBase::loadMultipleRevisions() will now return the revisions in the correct order.

Data model changes

None

Release notes snippet

None?

CommentFileSizeAuthor
#9 3019030-9.patch3.33 KBupchuk
#9 interdiff-3019030-9.txt1.58 KBupchuk
#3 3019030-3.patch2.7 KBupchuk
#3 3019030-3-test-only.patch1.95 KBupchuk

Comments

Upchuk created an issue. See original summary.

upchuk’s picture

upchuk commented
Title: Ensure loadMultiplRevisions() returns the revisions in the same order as the passed IDs » Ensure loadMultipleRevisions() returns the revisions in the same order as the passed IDs
upchuk’s picture

upchuk commented
Status: Needs work » Needs review
StatusFileSize
new 3019030-3-test-only.patch1.95 KB
new 3019030-3.patch2.7 KB

Here are the patches to prove the issue and the fix.

The last submitted patch, 3: 3019030-3-test-only.patch, failed testing. View results

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented

Nice catch! And thanks for providing a test. Looks good to me. I was wondering about the resetCache() in the tests. I guess this could be argued either way. With or without the resetCache() we are sort of testing different code paths and I guess one could argue that with the resetCache() we are testing a more complex code path so that it makes sense to test that one if we're only testing one of them. On the other hand, the actual code that does the re-keying is indepent of the two code branches, so one could also argue that the resetCache() is unnecessary. And in general I'm not a fan of resetCache() calls that would leave the test green when removed, i.e. if we refactor the test at some point, those could easily be dropped again... What do you think?

upchuk’s picture

upchuk commented

Actually, the cache reset is needed because of the way the static cache loads the entities:

<?php
  protected function getFromStaticCache(array $ids) {
    $entities = [];
    // Load any available entities from the internal cache.
    if ($this->entityType->isStaticallyCacheable()) {
      foreach ($ids as $id) {
        if ($cached = $this->memoryCache->get($this->buildCacheId($id))) {
          $entities[$id] = $cached->data;
        }
      }
    }
    return $entities;
  }
?>

It loops through the IDs (in the same order as they are passed). So for the regular entity loads, the test would not fail (without the ordering bit in place) if calling loadMultiple() the second time.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Status: Needs review » Reviewed & tested by the community

Ahh, that totally makes sense, thanks!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work

I think adding something about what is explained in #6 to the test is important. Nice find and test coverage though.

Also I think it is worth doing a load with the static cache to prove the static cache also returns in the expected order.

Also let's use assertSame() instead of assertEquals()

upchuk’s picture

upchuk commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-3019030-9.txt1.58 KB
new 3019030-9.patch3.33 KB

Alright, here we go. testing now with and without cache reset just to ensure we get correct results.

However, sticking to assertEquals() because for some reason loadMultiple() returns the keys as strings so it doesn't compare well with strict type checking. Would need to cast stuff unnecessarily in my opinion.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs review » Reviewed & tested by the community

@Upchuk thanks that looks good.

Re the integer to string key conversion - that's not nice but it is probably down to the fact everything comes back from the DB as strings. One day we might use TypedData to fix that.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Category: Feature request » Task
Status: Reviewed & tested by the community » Fixed

As #9 is only test improvements I think the rtbc in #7 counts.

Committed 5470a05 and pushed to 8.7.x. Thanks!

  • alexpott committed 5470a05 on 8.7.x 
    Issue #3019030 by Upchuk: Ensure loadMultipleRevisions() returns the...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.