Problem/Motivation

This condition is basically useless.

// Check expire time.
$prepared->valid = $prepared->expire == Cache::PERMANENT || $prepared->expire >= $this->time->getRequestTime();

The cache will never expire within a single HTTP request in fpm-fcgi or cli SAPI.

Proposed resolution

Use $this->time->getCurrentTime() instead.

CommentFileSizeAuthor
#2 483370-2-memory_backend-current_time.patch706 byteschi

Issue fork drupal-3483370

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

chi created an issue. See original summary.

chi’s picture

chi commented
Status: Active » Needs review
StatusFileSize
new 483370-2-memory_backend-current_time.patch706 bytes

chi changed the visibility of the branch 3483370-drupalcorecachememorybackend-should-not to hidden.

chi opened merge request !9945

chi’s picture

chi commented

I actually think all cache backends should use currentTime.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Appears to have test failures. Probably would need test coverage to show the issue?

marcusml made their first commit to this issue’s fork.

marcusml’s picture

marcusml commented
Status: Needs work » Needs review
Issue tags: +Vienna2025

Tests should pass now. 🤞

We've been using this patch for a while now in order to fix issues with cache invalidation on long running jobs .

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Needs work

The child class \Drupal\Core\Cache\MemoryCache\MemoryCache should probably also be considered as it has the same check.

FWIW, this might come with a non-trivial cost, getting the current time is more expensive than the request time, that's why we default to request time.

Also, per #3047289: Standardize how we implement in-memory caches, MemoryBackend isn't really a good backend for in-memory caching as it is designed to be a drop-in replacement for real backends that has the same serialize behavior.

parth_sinha made their first commit to this issue’s fork.

parth_sinha’s picture

parth_sinha commented

My apologies for the initial confusion - I mistakenly pushed to the 11.x.c branch instead of 11.x due to a typo.

Added unit tests to verify the fix works correctly.

Files added:
- core/tests/Drupal/Tests/Core/Cache/MemoryBackendExpirationTest.php
- core/tests/Drupal/Tests/Core/Cache/MemoryCacheExpirationTest.php

Test coverage includes:
- testCacheExpirationWithCurrentTime(): Verifies cache items properly expire after their TTL passes, using mocked time progression (1000 → 1005 seconds) to simulate elapsed time in long-running processes
- testPermanentCacheDoesNotExpire(): Confirms permanent cache entries never expire regardless of time, even with `PHP_INT_MAX` timestamp

Both tests use mocked TimeInterface to control time progression without relying on sleep(), ensuring fast and reliable test execution.

All tests pass with the getCurrentTime() fix applied.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.

kksandr made their first commit to this issue’s fork.

kksandr changed the visibility of the branch 11.x.c to hidden.

kksandr’s picture

kksandr commented
Status: Needs work » Needs review

I’ve updated the merge request to be compatible with the current core.

Some notes on the changes:

I introduced \Drupal\Core\Cache\CacheTimeInterface with a single getTime(): int method. Cache backends that don’t use the request time for expiration (e.g. MemoryBackend and MemoryCache, which now rely on the current time) implement it, so that consumers can find out which time the backend actually uses.
This is needed because some code assumes that all cache backends use the request time. For example, \Drupal\Core\Cache\VariationCache duplicated the expiration logic and relied on that assumption, which is no longer true. It now asks the backend for its time instead.