Problem/Motivation

Sometimes we try to load entities that don't exist. This is most common with config entities - e.g. language content settings is a config entity per entity bundle, and not every bundle has language content settings, so we try to load something that may or may not exist.

However we don't statically cache the fact that an entity wasn't loaded, so we keep trying to load it from the persistent cache again and again.

Discovered because I was seeing multiple config cache gets for language content settings even after #3560633: Load content language settings in bulk when rebuilding bundle and extra fields info.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#35 3587565-followup.patch2.59 KBamateescu
#19 3587565-nr-bot_1nvqdjrk.txt98 bytesneeds-review-queue-bot
#17 3587565-nr-bot_4jvsffqk.txt91 bytesneeds-review-queue-bot
#14 3587565-nr-bot_oqxywdfg.txt91 bytesneeds-review-queue-bot
#12 3587565-nr-bot_3pvu7c4r.txt91 bytesneeds-review-queue-bot
#10 3587565-nr-bot_m_cyymeb.txt91 bytesneeds-review-queue-bot
#8 3587565-nr-bot_ymw231hc.txt91 bytesneeds-review-queue-bot
#6 3587565-nr-bot_d55igcb_.txt98 bytesneeds-review-queue-bot

Issue fork drupal-3587565

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’s picture

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

MR is up.
Updated performance tests, other tests might fail still

catch opened merge request !15608

catch’s picture

catch
he/him
Primary language English
commented
Issue summary: View changes
catch’s picture

catch
he/him
Primary language English
commented

Added a unit test. I don't know why AssetAggregationAcrossPagesTest needed an update - less files is good but not sure how this could affect that.

edit: it's because HEAD was broken.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3587565-nr-bot_d55igcb_.txt98 bytes

The Needs Review Queue Bot tested this issue. The merge request has merge conflicts and cannot be merged. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Needs review

Rebased.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3587565-nr-bot_ymw231hc.txt91 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Needs review

Rebased.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3587565-nr-bot_m_cyymeb.txt91 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Needs review
needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3587565-nr-bot_3pvu7c4r.txt91 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Needs review

I'd briefly thought about separate methods but thought it would make cache invalidation etc. a bit more complex, however now we have uuid lookup caching in there, it's just another one.

So added a different cache key prefix for not found + get/set methods for not found entities and switched to using those. Agreed this looks better. If it did somehow break a subclass then I think it would at least be more obvious why.#

edit: it broke ConfigEntityStorage but found a way to unbreak it within the main base class for bc.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3587565-nr-bot_oqxywdfg.txt91 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Needs review

Rebased.

catch’s picture

catch
he/him
Primary language English
commented

Rebased.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3587565-nr-bot_4jvsffqk.txt91 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Needs review

Rebased.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3587565-nr-bot_1nvqdjrk.txt98 bytes

The Needs Review Queue Bot tested this issue. The merge request has merge conflicts and cannot be merged. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Needs review

Rebased.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Reviewed & tested by the community

Looks good to me now.

godotislate’s picture

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

Some questions about comments and a couple nits. OK to self-RTBC.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

Comment changes were all good spots, think I got everything.

godotislate’s picture

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

Needs a rebase after #3612172: Merge Umami performance tests.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Reviewed & tested by the community

Rebased.

  • godotislate committed 36e3ad42 on main 
    task: #3587565 Static cache entities that aren't loaded

By: catch
By:...
godotislate’s picture

godotislate
he/him
commented
Version: main » 11.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed 36e3ad4 and pushed to main. Thanks!

Merge conflict in the performance tests. It might make sense to get #3612172: Merge Umami performance tests in to 11.x too.

catch opened merge request !16622

catch’s picture

catch
he/him
Primary language English
commented
Status: Patch (to be ported) » Reviewed & tested by the community

Put up an 11.x MR, only performance test changes so moving straight back to RTBC.

godotislate’s picture

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

Tests are failing, including PHPStan.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Reviewed & tested by the community

Missed that... the unit test change applied cleanly, but it depends on a method that doesn't exist. We can just remove the call because the method sets up something that's in ::setUp().

  • godotislate committed 2ab239c5 on 11.x 
    task: #3587565 Static cache entities that aren't loaded

By: catch
By:...
godotislate’s picture

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

Committed 2ab239c and pushed to 11.x. Thanks!

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

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

Maintainers, credit people who helped resolve this issue.

amateescu’s picture

amateescu commented
StatusFileSize
new 3587565-followup.patch2.59 KB

This change broke some tests in Trash, where we have to prevent writing soft-deleted entities to the persistent cache. The fix is quite small, so I'm not sure whether a quick followup MR would be better than a new issue :)

catch’s picture

catch
he/him
Primary language English
commented
Status: Fixed » Needs work

Re-opening for visibility, I'd be fine either way with a follow-up MR vs. new issue, the same change should apply to both 11.x and main.

amateescu opened merge request !16713

amateescu’s picture

amateescu commented
Status: Needs work » Needs review

Cool, opened the followup MR here.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Version: 11.x-dev » main
Status: Needs review » Reviewed & tested by the community

I think I was wondering about whether that array_diff_key() was safe, but I thought more about the queried entities than $ids. This is consistent with an earlier calls to get missing ids.

  • catch committed de84a108 on 11.x 
    task: #3587565 Static cache entities that aren't loaded

By: catch
By:...

  • catch committed 057a6964 on main 
    task: #3587565 Static cache entities that aren't loaded

By: catch
By:...
catch’s picture

catch
he/him
Primary language English
commented
Version: main » 11.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to main and 11.x, thanks!

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

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

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

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