Problem/Motivation
Once a date format is used during a request, the entity object stays with the formatter till the end of the request. If the date format is changed afterwards, the formatter does not pick up the change and continues using the old pattern. This can affect processes which updates a date format and then formats a date in the same process. Eg. Config import, update hooks, batches, migrations, or Drush commands. I encountered it in workspace switching while working on the workspace_config issue.
Steps to reproduce
Run the following script
$f = \Drupal::service('date.formatter');
$c = \Drupal\Core\Datetime\Entity\DateFormat::load('new_format');
if (!$c) {
$c = \Drupal\Core\Datetime\Entity\DateFormat::create([
'id' => 'new_format', 'label' => 'Probe', 'locked' => FALSE, 'pattern' => 'j M Y',
]);
$c->save();
}
print $f->format(0, 'new_format', '', 'UTC') . PHP_EOL; // 1 Jan 1970
\Drupal\Core\Datetime\Entity\DateFormat::load('new_format')->setPattern('Y')->save();
print \Drupal\Core\Datetime\Entity\DateFormat::load('new_format')->getPattern() . PHP_EOL; // Y
print $f->format(0, 'new_format', '', 'UTC'); // 1 Jan 1970 (expected - 1970)
I saved it in a php file and used drush scr to execute it.
Proposed resolution
Replace static cache with in memory cache
Remaining tasks
MR & Test
User interface changes
None
Introduced terminology
None
API changes
TBD
Data model changes
None
Release notes snippet
TBD
Issue fork drupal-3622289
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:
Comments
Comment #3
hoxton commentedAdded MR for cache invalidation to date formatter to track changes in date format entity
Comment #4
hoxton commentedComment #6
dcam commentedI'm setting the issue status to Needs Work for a few reasons:
$dateFormatStorageproperty isn't initialized anywhere.Comment #7
nicxvan commentedAlso shouldn't we use an actual memory cache like here: https://git.drupalcode.org/project/drupal/-/blob/main/core/modules/local...
Comment #8
hoxton commented@nicxvan, I tried adding the suggested caching service, but it started throwing circular dependency issue.
Comment #9
hoxton commented@dcam, thanks for pointing out the issues. Have added the missing tests.
Comment #10
hoxton commentedComment #11
nicxvan commentedI do think we should take another look, in most cases we try to avoid adding storage to properties in constructors.
Comment #12
amateescu commentedYes, we shouldn't store the entity storage in a property, and the whole point of this issues is to use the memory cache service (as @nicxvan pointed out in #7), so it can be reset from the "outside", for example when switching workspaces.