Date tokens do not handle all date types

This is somewhat related to the (D6) issue at #593840: Include tokens for custom date formats for date fields which created new tokens for each of the custom date formats and date format types.

#2: 1173706-date-custom-formats.patch queued for re-testing.

...tagging ;)

Will this work for D8? Used this issue as guide: https://drupal.org/node/1876852

Replaced: D7 system_get_date_types()

With: D8 system_get_date_formats()

Warning: I am a novice

#12 passed and ready for review, testing, rework

Can someone please update the issue summary with steps to reproduce this bug so I can test the patch provided and report back.

The truth is that I really want to see this be backported to date 7.x ASAP and the way to do that is fix it in D8 first.

Ok, I just checked and feel really stupid about the fact I never noticed that before. Thanx Dave (not for making me feel stupid - for pointing the thing).

Still I wish to help with testing so we can get this in D8, so STR in the issue summary would help.

sorry @Dave Reid thought your response to #15 meant we already had it in D7 and didn't need backport.

not sure what needs to be tested on this.

Patch needs rerolling. For adding a test, hook_token doesn't appear to be tested at all at this point. There are integration tests for token replacement though, so the system_tokens() function could potentially be tested from there.

Rerolled the patch as per the latest D8 release.

Removing Need reroll tag.

Yes, we need to create and test an additional date format.

We also need to make sure that adding a new one clears the token cache, see token_form_alter() in token.module. So create one in the UI and then ensure that the token info for it exists.

I think we also need to add cacheability metadata for the replaced tokens based on the used date format.

All,

any updates on this issue please?

Thanks,
Sukanya

Here's a re-roll + I replaced the t('@name' because the string is not translatable like that, the label would need to be translated already.

Also changed the logic a bit in system_tokens and adds the cachable metadata that @berdir mentioned.

+++ b/core/modules/system/system.tokens.inc
@@ -161,17 +174,17 @@ function system_tokens($type, $tokens, array $data, array $options, BubbleableMe
+    $date_format_storage = \Drupal::entityTypeManager()->getStorage('date_format');
     foreach ($tokens as $name => $original) {
+      // Date type token replacement.
+      $date_format = $date_format_storage->load($name);
+      if ($date_format) {
+        $bubbleable_metadata->addCacheableDependency($date_format);
+        $replacements[$original] = format_date($date, $name, '', NULL, $langcode);
+        continue;

This is also needed, but it's only about caching the results of a token replacement.

It does not affect what is cached in the token info (which is based on system_token_info()). That doesn't support cache tags, you need to explicitly call \Drupal::token()->resetInfo();. See token_date_format_insert()/token_date_format_delete()

What we should do is extend \Drupal\Tests\system\Kernel\Token\TokenReplaceKernelTest::testSystemDateTokenReplacement(), with token replacement calls for configurable date formats, but also test \Drupal::token()->getInfo(), make sure it contains the date formats, then create a new one, then call getInfo() again and it must return the new one. additionally, we could also test the cacheable metadata that is returned, similar to to testSystemSiteTokenReplacement().

I've extended \Drupal\Tests\system\Kernel\Token\TokenReplaceKernelTest::testSystemDateTokenReplacement to test all date format tokens and a small assert to make sure we are testing "something" with a check for the 11 date formats.

I added a couple system_date_format_insert/delete() with the \Drupal::token()->resetInfo() call. And a test for

test \Drupal::token()->getInfo()

Hope I got all the things, thanks for the review @Berdir.

1. +++ b/core/modules/system/system.module
   @@ -1424,3 +1424,17 @@ function system_element_info_alter(&$type) {
   + * Implements hook_date_format_insert().
   + */
   +function system_date_format_insert() {
   +  \Drupal::token()->resetInfo();
   +}
   +
   +/**
   + * Implements hook_date_format_delete().
   + */
   +function system_date_format_delete() {
   +  \Drupal::token()->resetInfo();
   

   unlike the token module, you can actually implement that directly in the date format entity class instead of adding hooks.

2. +++ b/core/modules/system/tests/src/Kernel/Token/TokenReplaceKernelTest.php
   @@ -5,7 +5,10 @@
    use Drupal\Core\Render\BubbleableMetadata;
   +use mysql_xdevapi\Exception;
   +use phpDocumentor\Reflection\DocBlock\Tags\Example;
    
   

   those seem to be wrong/unrelated includes.

3. +++ b/core/modules/system/tests/src/Kernel/Token/TokenReplaceKernelTest.php
   @@ -5,7 +5,10 @@
   +use mysql_xdevapi\Exception;
   

   that doesn't seem to be the right exception class ;)

Whoops, that's like leaving `dpm()` in the code... got a bit alt-enter happy in PHPStorm. Oh nice I don't have to use the hooks!

Re 1. Is it on DateFormat entity that I add the hooks or do I need to extend ConfigEntityStorage?

DateFormat

Took a stab at it, not sure if I should use dependency injection here and stuck the code on the save() method instead of insert() which doesn't exist on that class.

You want postSave() (with if (!$update)) and postDelete(). save() is just a wrapper for $storage->save($entity) and isn't guaranteed to be called.

DI is not possible in entity classes.

postDelete is static and postSave() is not 😖

There was a slack request to review this, and I had a few minutes to spare, so I took a quick look at this patch. A few comments -- just noting that I didn't test the patch, I only looked at the code.:

a) The latest test found a coding standards violation:

source/core/modules/system/system.tokens.inc
line 10	Unused use statement

b) I think this should most likely be on 8.8.x. It may be backported on commit, but my understanding is all issues are 8.8 now.

c) Nitpick -- in the system_token_info() function:

+      'description' => t("A date in the %name format. (%date)", [

Why use double quotes here? I think we usually want to use single quotes unless double are really needed, and I don't see that they're needed here. I guess it is just left over from the copy/paste of existing lines from that function, such as

 'description' => t("A date in 'medium' format. (%date)", [

d) In the system_tokens() function: It looks like there could be a problem in the unlikely event that a date format ID is either 'since' or 'raw', because it would load the format instead of getting to the switch statement cases that are still in the code. Is it possible for either of those to be a date format ID?

e) Also, what happens if you call ->load($id) on an ID that doesn't exist? Because 'since' and 'raw' would be passed in... let's see... I guess it would be OK as long as those formats don't exist in the database. Looks like ->load() returns nothing if the ID doesn't exist. So, you can ignore this. ;)

f) In the test, nitpick:

+  public function testSystemDateTokenGetInfo() {
+    $tokens = \Drupal::token()->getInfo();
+    $this->assertArrayNotHasKey('y2k', $tokens['tokens']['date']);
+
+    // Add new tokens.
+    $date_format = DateFormat::create([
+      'id' => 'y2k',
+      'label' => 'Y2K',
+      'pattern' => 'y',
+    ]);

That comment should be "Add new formats" not "Add new tokens", and then a bit farther down:

+    // Delete new token.
+    DateFormat::load('y2k')->delete();

Same, should be "Delete a format" I think?

Thanks for the review @jhodgdon!

1. Removed, thanks, though hope that's not out of scope since it was there before.
2. You're likely right, I'll let the committer decide
3. Fixed
4. It's not possible, because of their specialness they've become "reserved".
5. You got it
6. Fixed, updated comments.

Looking good!

I realize I have a few more questions:

a) In the DateFormat entity class:

+  public function postSave(EntityStorageInterface $storage, $update = TRUE) {
+    parent::postSave($storage, $update);
+    if (!$update) {
+      \Drupal::token()->resetInfo();
+    }
+  }

Is it possible for an update to change the ID of the format? If so, then even on an update, we would still want the token info to be replaced.

b) Same method -- this method is not static, so shouldn't the token service be injected rather than using a call to \Drupal::token()?

c) In the info() hook -- can the existing code that added the tokens for short, medium, and long formats be removed? Because it seems like this part you added

+  $date_formats = \Drupal::entityTypeManager()->getStorage('date_format')->loadMultiple();
+  foreach ($date_formats as $date_format) {

should include those 3 built-in formats? [If not, then the code in the hook_tokens() implementation wouldn't work either?]

d) One last comment nitpick, at the bottom of the test:

+    // Get the count of removed tokens.
+    $tokens = \Drupal::token()->getInfo();
+    $this->assertArrayNotHasKey('y2k', $tokens['tokens']['date']);

That doesn't look like a count to me. :) Probably better "Verify the token was removed."

@jhodgdon Sorry for the delay getting back to this. Thanks again for the review.

1. I don't think it's possible to change the machine name without delete.
2. I'm not sure how to inject this because there is no create() method or a access to the container, any ideas?
3. I think you're right, removed!
4. Thanks for spotting this, fixed

I just did a quick test, and you are correct -- at least from admin/config/regional/date-time there is no way to change a date format ID once the format has been created.

And I agree about the injection... at least, I see in methods like EntityBase::create() it calls \Drupal methods. Ugh. But at least consistent.

So, it looks like all of my review comments have been addressed. I think the code looks good!

It seems all the feedback above has been addressed. I think this looks good to go! Will be great to get this long-standing issue fixed!

1. +++ b/core/lib/Drupal/Core/Datetime/Entity/DateFormat.php
   @@ -100,4 +101,22 @@ public function getCacheTagsToInvalidate() {
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public static function postDelete(EntityStorageInterface $storage, array $entities) {
   +    parent::postDelete($storage, $entities);
   +    \Drupal::token()->resetInfo();
   +  }
   +
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function postSave(EntityStorageInterface $storage, $update = TRUE) {
   +    parent::postSave($storage, $update);
   +    if (!$update) {
   +      \Drupal::token()->resetInfo();
   +    }
   +  }
   

   I'm not sure I agree with @Berdir in #44 - I think this should be done as entity hooks as the tokens are defined by system module and this is core library entity.

2. +++ b/core/modules/system/system.tokens.inc
   @@ -53,18 +52,18 @@ function system_token_info() {
   +    $date[$date_format->id()] = [
   

   We need to manipulate the name here because we have potential clashes. I.e. is you use custom, since or raw as data format names.

   Another tricky thing is that we can't rename short, medium or long.

   Fun times.

   Earlier comments say
   

   It's not possible, because of their specialness they've become "reserved".

   Unfortunately the following code works.

   >>> $date_format = \Drupal\Core\Datetime\Entity\DateFormat::create(['id' => 'since', 'label' => 'Since', 'pattern' => 'y']);
   >>> $date_format->save();
   => 1
   >>> \Drupal\Core\Datetime\Entity\DateFormat::load('since')->label();
   => "Since"
   

3. +++ b/core/modules/system/system.tokens.inc
   @@ -165,16 +164,17 @@ function system_tokens($type, $tokens, array $data, array $options, BubbleableMe
   +    /** @var \Drupal\Core\Entity\EntityStorageInterface $date_format_storage */
   +    $date_format_storage = \Drupal::entityTypeManager()->getStorage('date_format');
   ...
   +      // Date type token replacement.
   +      $date_format = $date_format_storage->load($name);
   +      if ($date_format) {
   +        $bubbleable_metadata->addCacheableDependency($date_format);
   +        $replacements[$original] = \Drupal::service('date.formatter')->format($date, $name, '', NULL, $langcode);
   +        continue;
   +      }
   

   Better to load all the date formats outside of the loops and check if the token name is in the array.

Doing a rebase first.

Since this is a bug-report, I am not sure if I should continue to target 8.7.x. If you think we should, please change it back. I only changed it because I rebased this on 8.8.x

Needs patch (or MR) against 9.3.x

Patch for 9.3.x. Please review.

In the last patch #67, Forgot to add class "Drupal\Core\Datetime\Entity\DateFormat" in "TokenReplaceKernelTest.php" file.

Fixed the fail test issue because of the above miss. Please have a look.

Feedback at #59 from framework manager yet to be addressed.

We've hit a problem with Metatag that appears to be related to this: #3271006. The error is:

exception: [User warning] Line 912 of sites/all/modules/token/token.tokens.inc:
Attempting to perform token replacement for token type date without required data

FeyP was able to reproduce it locally and saw it stemmed from the image field on the Article content type that comes with the standard install profile having a default path which creates a per-date directory for storing files, and for some reason it hits this error in certain circumstances.

Reroll for 9.5.x

Yeah I messed that up

I re-roll it to 10.2.x.

Could the issue summary be updated to the standard template.

Also recommend opening an MR as those are quicker to review and test.

Changing back to 11.x as the current development branch.