Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
EntityStorageBase::loadMultiple
returns entities with ids other than provided as the argument when WorkspaceManager::shouldAlterOperations
returns true and the static cache is warm.
Proposed resolution
Fix Drupal\workspaces\EntityOperations::entityPreload
Comment | File | Size | Author |
---|---|---|---|
#11 | interdiff-11.txt | 4.3 KB | amateescu |
#11 | 3068733-11.patch | 3.68 KB | amateescu |
#11 | 3068733-11-test-only.patch | 1.57 KB | amateescu |
#10 | interdiff.3068733.6-10.txt | 938 bytes | pmelab |
#10 | 3068733-10.drupal.EntityStorageBaseloadMultiple-returns-unwanted-entities-when-the-static-cache-is-warm.patch | 2.41 KB | pmelab |
Comments
Comment #2
blazey CreditAttribution: blazey at Amazee Labs commentedHere's the test case.
Comment #3
blazey CreditAttribution: blazey at Amazee Labs commentedComment #4
blazey CreditAttribution: blazey at Amazee Labs commentedComment #6
blazey CreditAttribution: blazey at Amazee Labs commentedHere's an attempt to fix the problem. It works under the assumption that the active workspace doesn't change between the calls to
loadMultiple
. If the workspace change is something we want to support then it should probably be added to the static cache cid.Comment #7
blazey CreditAttribution: blazey at Amazee Labs commentedComment #8
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedNice find, @blazey!
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:In order to be consistent with the rest of the assertions from this test class, let's assert this inside of
assertEntityLoad()
, between checkingloadMultiple()
andloadUnchanged()
:Comment #9
cilefen CreditAttribution: cilefen at Institute for Advanced Study commentedComment #10
pmelab CreditAttribution: pmelab at Amazee Labs commentedRe-rolled the patch against latest 8.7.x
Comment #11
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedFixed the points from #8.
Comment #13
pmelab CreditAttribution: pmelab at Amazee Labs commentedReviewed and tested in an actual use case. Looks good!
Comment #14
hchonovVery 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: ContentEntityStorageBase::loadRevision() should use the static and the persistent entity cache like ContentEntityStorageBase::load().
Comment #16
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedComment #17
Ghost of Drupal PastJust 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 withif ($ids === []) return [];
. And then:$flipped_ids
inside the if ($ids) {, the exact same check is run twice on subsequent lines (add else for FALSE)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 theif
are fine with empty arrays and $preloaded_entities is always an array according to the preload typehint.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.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.
Comment #18
amateescu CreditAttribution: amateescu for Pfizer, Inc. commented@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.
Comment #20
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedComment #21
Gábor HojtsyHm, 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.
Comment #22
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedBecause 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 :)
Comment #24
catchCommitted c86caa0 and pushed to 8.8.x. Thanks!
Agreed it's worth opening a follow-up for #17.
Comment #25
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedThanks, @catch! As mentioned above, this should be backported to 8.7.x :)
Comment #26
catchCherry-picked to 8.7.x too.