Problem/Motivation

Found a bug in #1237636: Lazy load multiple entities at a time using fibers a couple of hours after commit, via rebasing #3496369: Multiple load path aliases without the preload cache. I think it can be a quick fix so opening this as a follow-up - should make the specific problem easier to follow in a fresh MR too.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3549380

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

catch created an issue. See original summary.

catch opened merge request !13351

catch’s picture

catch
he/him
Primary language English
commented
Priority: Normal » Critical
Status: Active » Needs review

Test failure from the new test coverage can be seen here:

https://git.drupalcode.org/issue/drupal-3549380/-/jobs/6707490

This is also a further performance improvement - e.g. when a fiber ends up loading entities in two (or more) separate calls, fibers resumed afterwards which only need to retrieve their entities from the static cache won't try to load the second entity, they'll just ignore it, leaving it to be loaded later which means more lazy loading. Umami performance tests don't have that scenario though.

Bumping to critical because the original bug this fixes, exposed by #3496369: Multiple load path aliases without the preload cache, can result in an entity not being loaded at all - when $this->entityIdsToLoad has been set after the entity we're actually trying to load was already put into the static cache.

godotislate’s picture

godotislate
he/him
commented

1 small comment about a missing word in the MR, otherwise looks good.

godotislate’s picture

godotislate
he/him
commented
Status: Needs review » Reviewed & tested by the community

I feel slightly weird about $this->assertSame($return1[3]->id(), $ids[2]);, since the index 3 is actually the entity ID being tested, but I see that was how other similar asserts were done, and this isn't a problem unless somehow the DB isn't returning IDs that are sequential by 1.

LGTM.

  • alexpott committed e3ccb201 on 11.x 
    Issue #3549380 by catch, godotislate: Fiber entity multiple loading...
alexpott’s picture

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

Committed e3ccb20 and pushed to 11.x. Thanks!

Now that this issue is closed, please review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, please credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

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