Ensure invalid items are not written to FastBackend in ChainedFast

Iam working on this.

Hi @Fabianx,

I have created and attached the patch. Could you please verify this?

Thank you, @deborahblessy!

Next step: test coverage.

For #5

Catching up on long-forgotten tabs … from last year’s European DrupalCon just before this year’s starts 😄

I think Fabian is best suited to determine what is next here.

Brought this up in #contribute in slack https://drupal.slack.com/archives/C1BMUQ9U6/p1727876410850979

@fabianx agreed on the approach but mentioned still needs tests.

Moved to MR so tests can run and be more easily written.

Hey, just created a new test to validate the proposed solution, moving this to NR and removing the tag for now, thanks!

1) Drupal\Tests\Core\Cache\ChainedFastBackendTest::testSetInvalidDataFastBackend
Invalid data was not saved on the fast cache.
stdClass Object (...) does not match expected type "NULL".
/builds/issue/drupal-3395212/core/tests/Drupal/Tests/Core/Cache/ChainedFastBackendTest.php:103
FAILURES!
Tests: 3, Assertions: 7, Failures: 1.
Exiting with EXIT_CODE=1

Shows test coverage.

Not sure if this needs any kind of release note (assuming not)

Looking at the change to core/lib/Drupal/Core/Cache/ChainedFastBackend.php and don't see anything glaringly wrong. Think adding the condition makes sense.

believe this one is good.

Gave the IS some thought and at first felt like this was wrong:

See that fastBackend is happily setting invalid data, which will never be useful

After all, if you're getting the data while note caring about its validity, you may as well store that in memory for future gets which do not care about validity. However, then I realized that a cache get which allows invalid could the poison the memory cache for a subsequent cache get which does not allow invalid. And that would be really bad.

I still have the eerie feeling that there are edge cases we're not seeing here, but for now I'd say it makes more sense to commit this change rather than keep the old behavior.

So RTBC +1.

I think this is worth skipping just from a cache efficiency point of view if nothing else - allow_invalid is most likely to be used in the case that stale content is served and then a cache refresh job is queued (or done end of request or similar), so there'll be a new cache write pretty fast after this gets set anyway. Cache poisoning seems theoretical because the fast backend should handle invalid items correctly, but it doesn't hurt.

Committed/pushed to 11.x/11.1.x/10.5.x/10.4.x, thanks!

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