Problem/Motivation

If you forget that CacheableMetadata::merge() doesn't alter the object it's called on, you potentially mess up your caching and could cause data exposure.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3575492

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

joachim created an issue. See original summary.

andypost’s picture

andypost
he/him
Primary language Russian
commented

would be great to move here https://git.drupalcode.org/project/drupal/-/merge_requests/14887

andypost opened merge request !14896

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Active » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Postponed

Sorry no harm in being review but don't want to accidentally get reviewed. #3575572: Add [#NoDiscard] to Utility classes is determining if this is worth it.

prudloff’s picture

prudloff commented
Issue tags: +Security improvements

I agree this would be useful. It is an easy mistake to make and it can cause serious cache bugs.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Postponed » Needs review

as policy approved

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

LGTM

  • godotislate committed 08901b47 on 11.x 
    task: #3575492 Add [#NoDiscard] to CacheableMetadata::merge()

By:...

  • godotislate committed a7b77cbf on main 
    task: #3575492 Add [#NoDiscard] to CacheableMetadata::merge()

By:...
godotislate’s picture

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

Committed and pushed a7b77cb to main. and 08901b4 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.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue tags: +DX (Developer Experience)

Very nice! 🎉

I predict that when the ecosystem starts requiring PHP 8.5, that this core commit will single-handedly trigger a bunch of contrib modules security releases 🔮