Problem/Motivation

Wrong variable type declaration of DrupalDateTime class's $formatTranslationCache property. 

 /**
   * Format string translation cache.
   *
   * @var string
   */
  protected $formatTranslationCache;

Following is the relevant code snippet from the same class's format() method:

 // Translates a formatted date string.
      $translation_callback = function ($matches) use ($langcode) {
        $code = $matches[1];
        $string = $matches[2];
        if (!isset($this->formatTranslationCache[$langcode][$code][$string])) {
          $options = ['langcode' => $langcode];
          if ($code == 'F') {
            $options['context'] = 'Long month name';
          }

          if ($code == '') {
            $this->formatTranslationCache[$langcode][$code][$string] = $string;
          }
          else {
            $this->formatTranslationCache[$langcode][$code][$string] = $this->t($string, [], $options);
          }
        }
        return $this->formatTranslationCache[$langcode][$code][$string];
      };
CommentFileSizeAuthor
#16 3104071-16.patch1.36 KBjungle
#8 formatTranslationCache-3104071-8.patch4.03 KBknyshuk.vova
#2 formatTranslationCache-3104071-2.patch455 bytesjungle
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jungle created an issue. See original summary.

jungle’s picture

jungle
He/Him
Location Chongqing, China
CreditAttribution: jungle as a volunteer commented
Issue summary: View changes
Status: Active » Needs review
FileSize
formatTranslationCache-3104071-2.patch455 bytes
jungle’s picture

jungle
He/Him
Location Chongqing, China
CreditAttribution: jungle as a volunteer commented
Issue summary: View changes
jungle’s picture

jungle
He/Him
Location Chongqing, China
CreditAttribution: jungle as a volunteer commented

Trivial Patch of the Month

(stolen from JohnAlbin's user page as well.)

Am I the winner of Trivial Patch of the Month?

jungle’s picture

jungle
He/Him
Location Chongqing, China
CreditAttribution: jungle as a volunteer commented
Related issues: +#1900356: Redundant space in theme_field() function
andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented

Would be great to extend description with details of array

jungle’s picture

jungle
He/Him
Location Chongqing, China
CreditAttribution: jungle as a volunteer commented

Thanks, @andypost for commenting. It's hard for me to add a description :( I tried to understand the code, but could not. Let's wait for the one who would like helping to add more details.

knyshuk.vova’s picture

knyshuk.vova CreditAttribution: knyshuk.vova at Internetdevels commented
FileSize
formatTranslationCache-3104071-8.patch4.03 KB

@andypost, thank you for the direction of improvement. I have added more informative description for $formatTranslationCache and improved some text to make it more readable.

jungle’s picture

jungle
He/Him
Location Chongqing, China
CreditAttribution: jungle as a volunteer commented
Version: 8.8.x-dev » 8.9.x-dev

Thanks, @knyshuk.vova!

+++ b/core/lib/Drupal/Core/Datetime/DrupalDateTime.php
@@ -23,9 +24,34 @@ class DrupalDateTime extends DateTimePlus {
+   *     'en' => [
+   *       'F' => [
+   *         'November' => t('November'),
+   *         'December' => t('December'),

According to https://www.php.net/manual/en/function.sprintf.php, should 'F' be 's'?

F: The argument is treated as a float and presented as a floating-point number (non-locale aware). Available as of PHP 5.0.3.
s: The argument is treated and presented as a string.

knyshuk.vova’s picture

knyshuk.vova CreditAttribution: knyshuk.vova at Internetdevels commented

@jungle, No, because 'F' and 'd' in my code example is compatible with format character from https://www.php.net/manual/en/function.date.php

jungle’s picture

jungle
He/Him
Location Chongqing, China
CreditAttribution: jungle as a volunteer commented
Status: Needs review » Reviewed & tested by the community

Cool, you are right!

jungle’s picture

jungle
He/Him
Location Chongqing, China
CreditAttribution: jungle as a volunteer commented
Title: $formatTranslationCache in DrupalDateTime class should be array » DrupalDateTime::formatTranslationCache should be an array
jungle’s picture

jungle
He/Him
Location Chongqing, China
CreditAttribution: jungle as a volunteer commented
Title: DrupalDateTime::formatTranslationCache should be an array » DrupalDateTime::$formatTranslationCache should be an array
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Title: DrupalDateTime::$formatTranslationCache should be an array » Documentation improvements to core/lib/Drupal/Core/Datetime/DrupalDateTime.php
Component: system.module » documentation

We're doing more doc fixes here that what the title claims.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Title: Documentation improvements to core/lib/Drupal/Core/Datetime/DrupalDateTime.php » DrupalDateTime::$formatTranslationCache should be an array
Status: Reviewed & tested by the community » Needs work

Actually putting my scope hat on can we limit the fixes to the original scope

+++ b/core/lib/Drupal/Core/Datetime/DrupalDateTime.php
@@ -23,9 +24,34 @@ class DrupalDateTime extends DateTimePlus {
   /**
-   * Format string translation cache.
+   * Formatted strings translation cache.
    *
-   * @var string
+   * Translation cache represents an instance storage for formatted date
+   * strings. It contains a multidimensional array where:
+   * - first level keys - are drupal language codes;
+   * - second level keys - are each symbols of given format string (like 'F');
+   * - third level keys - are original matched strings related to the symbol;
+   * - values - are translated or not-translated original strings (depends on
+   *   if a particular symbol represents translatable value according to PHP's
+   *   date() format character).
+   *
+   * For example:
+   * @code
+   *   [
+   *     'en' => [
+   *       'F' => [
+   *         'November' => t('November'),
+   *         'December' => t('December'),
+   *       ],
+   *       'd' => [
+   *         '10' => '10',
+   *         '31' => '31',
+   *       ],
+   *     ],
+   *   ]
+   * @endcode
+   *
+   * @var array
    */
   protected $formatTranslationCache;

Ie. this bit. The other changes clash with other issues trying to make similar changes on a global code base level. Allowing such changes here weakens the message that such changes should be fixed in the entire codebase first.

jungle’s picture

jungle
He/Him
Location Chongqing, China
CreditAttribution: jungle as a volunteer commented
Status: Needs work » Needs review
FileSize
3104071-16.patch1.36 KB

Only borrowed the comment for formatTranslationCache from #8, dropped other changes.

knyshuk.vova’s picture

knyshuk.vova CreditAttribution: knyshuk.vova at Internetdevels commented
Status: Needs review » Reviewed & tested by the community

I think now it is correct.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Version: 8.9.x-dev » 8.8.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed ceaf783d0b to 9.0.x and d7b34a94d7 to 8.9.x and 456d570452 to 8.8.x. Thanks!

Backported to 8.8.x as a docs fix.

  • alexpott committed ceaf783 on 9.0.x 
    Issue #3104071 by jungle, knyshuk.vova: DrupalDateTime::$...

  • alexpott committed d7b34a9 on 8.9.x 
    Issue #3104071 by jungle, knyshuk.vova: DrupalDateTime::$...

  • alexpott committed 456d570 on 8.8.x 
    Issue #3104071 by jungle, knyshuk.vova: DrupalDateTime::$...

Status: Fixed » Closed (fixed)

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