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

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:

Comments

ajits created an issue. See original summary.

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

hoxton’s picture

Added MR for cache invalidation to date formatter to track changes in date format entity

hoxton’s picture

Status: Active » Needs review

dcam’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

I'm setting the issue status to Needs Work for a few reasons:

  • The MR doesn't pass the linting tests.
  • The changes aren't complete. The new $dateFormatStorage property isn't initialized anywhere.
  • Bug fixes require regression tests to be written, so I'm tagging the issue for that.
nicxvan’s picture

Also shouldn't we use an actual memory cache like here: https://git.drupalcode.org/project/drupal/-/blob/main/core/modules/local...

hoxton’s picture

@nicxvan, I tried adding the suggested caching service, but it started throwing circular dependency issue.

hoxton’s picture

@dcam, thanks for pointing out the issues. Have added the missing tests.

hoxton’s picture

Status: Needs work » Needs review
nicxvan’s picture

I do think we should take another look, in most cases we try to avoid adding storage to properties in constructors.

amateescu’s picture

Status: Needs review » Needs work

Yes, 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.