DateFormatter should not attempt to load a format from storage if the format is "custom"

Nice catch; that was one of the thoughts I wanted to look into for the parent problem.

Are there no existing unit tests for datetime module?

Apparently, there is no direct test coverage of that service (which is really part of core and not the datetime.module, which just provides the fields/widgets/formatters, but that is splitling hairs) :/ The service is indirectly tested in the timestamp tests, and the field tests.

It should have its own test coverage. Given the number of services that get injected, a kernel test (rather than a unit test) would probably be best (and easiest, so we don't need to mock five services).

I'm wondering whether on top of that we want to also static cache the result of loading the date format. I guess custom is just particulally horrible because we don't actually have that, so we cannot cache the result of the empty result?

I thought entity loads were already static cached?

Also, DateFormatter::dateFormat() already does static caching by (format,langcode). The docs say it returns the format string, but it looks like is returns the date format entity. I think that can be tightened up to call ->getPattern() that and static caching that instead of the entity, and then getting rid of the ->getPattern calls in dateFormat().

If doing the entity lookup for 'custom' is failing and causing a big performance problem, then this means that we have a problem with searching for config entities by label in general? There really aren't that may default format that the system provides.

This makes DateFormatter::dateFormat() match the docblock, which should also speed things up.

Adding Needs Tests. We need a kernel test to prevent regression here. Should be able to pull all of the format entities, grab their patterns, render out something, and compare against what date() would do with the same thing (being careful w/ timezones...).

\Drupal\KernelTests\Core\Datetime\FormatDateTest has plenty of explicit coverage for this.

Here is a fix for my last oopsie.

Let's see if this is better.

This looks good to go.

1. +++ b/core/lib/Drupal/Core/Datetime/DateFormatter.php
   @@ -121,14 +121,15 @@ public function format($timestamp, $type = 'medium', $format = '', $timezone = N
   +      // Fall back to medium if a format was not found.
   +      if (empty($format)) {
   +        $format = $this->dateFormat('fallback', $langcode);
   +      }
   
   @@ -324,8 +325,14 @@ protected function dateFormat($format, $langcode) {
   +        $this->dateFormats[$format][$langcode] = NULL;
   

   If the NULL was the fallback pattern we'd save quite a few method calls.

2. +++ b/core/lib/Drupal/Core/Datetime/DateFormatter.php
   @@ -324,8 +325,14 @@ protected function dateFormat($format, $langcode) {
          $this->languageManager->setConfigOverrideLanguage(new Language(array('id' => $langcode)));
   -      $this->dateFormats[$format][$langcode] = $this->dateFormatStorage->load($format);
   -      $this->languageManager->setConfigOverrideLanguage($original_language);
   +      $date_format = $this->dateFormatStorage->load($format);
   +      if ($date_format) {
   +        $this->dateFormats[$format][$langcode] = $date_format->getPattern();
   +        $this->languageManager->setConfigOverrideLanguage($original_language);
   

   The changes to where $this->languageManager->setOverrideLanguage() is called don't make sense. If we call it to change the language regardless or not if we have loaded a format we need to set it back afterwards. Obviously we're missing test coverage of that but that's tricky. Also if we were to load the fallback here then the fallback pattern could vary per language - which actually makes sense.

3. +++ b/core/lib/Drupal/Core/Datetime/DateFormatter.php
   @@ -324,8 +325,14 @@ protected function dateFormat($format, $langcode) {
        return $this->dateFormats[$format][$langcode];
   

   We changing the return type here. This makes this fix unnecessarily not bugfix safe. Also if we decide we only want this to be fixed in a minor (which I'm not sure is right) then we need to update the docs.

But to be honest the entire scope of this change could be achieved by just testing what $type is before trying to load the format. Also there is another unintended logic change. In HEAD calling \Drupal\Core\Datetime\DateFormatter::format(0, 'custom', '') whould fallback to the fallback - with the patch in #11 it looks as if that behaviour is broken.

Thanks for looking at this.

#13-1, not totally sure what you mean here? That hunk you referenced is mostly an indentation change; but

    // Lookup the date format pattern to use unless we were passed in a custom
    // format pattern.
    if ($type !== 'custom') {
      $format = $this->dateFormat($type, $langcode);

      // Fall back to medium if a format was not found.
      if ($format !== NULL) {
        $format = $this->dateFormat('fallback', $langcode);
      }
    }

would probably be better, compared to what is in HEAD:

    // If we have a non-custom date format use the provided date format pattern.
    if ($date_format = $this->dateFormat($type, $langcode)) {
      $format = $date_format->getPattern();
    }

    // Fall back to medium if a format was not found.
    if (empty($format)) {
      $format = $this->dateFormat('fallback', $langcode)->getPattern();
    }

#13-2, yup. That is borked, and we are missing test coverage somewhere (likely b/c mocking).

#13-3, we are changing the return type, but it matches the advertised API. IOW, in HEAD the docblock says a string is returned but a config entity is actually returned.

#14, also a little confused by this comment, too. The patch does check for `if ($type != 'custom')` before loading the config entity / format. We don't know if the 'type' actually exists until we try to load it.

OK, here is the minimally invasive way to do it to be BC, and a doc fix.

Confused about the 2846643-should-fail.patch b/c commenting out the save/restore of the ConfigOverrideLanguage doesn't seem to make a difference in the test.

I'm confused, the should fail patch is failing, but it wasn't expected to, or wasn't locally?

+++ b/core/lib/Drupal/Core/Datetime/DateFormatter.php
@@ -122,13 +122,15 @@ public function format($timestamp, $type = 'medium', $format = '', $timezone = N
+    if ($type !== 'custom') {
+      if ($date_format = $this->dateFormat($type, $langcode)) {
+        $format = $date_format->getPattern();
+      }
 
-    // Fall back to medium if a format was not found.
-    if (empty($format)) {
-      $format = $this->dateFormat('fallback', $langcode)->getPattern();
+      // Fall back to medium if a format was not found.
+      if (empty($format)) {
+        $format = $this->dateFormat('fallback', $langcode)->getPattern();
+      }
     }

This is still an unnecessary behaviour change...
HEAD does this:

>>> \Drupal::service('date.formatter')->format(0, 'custom', '');
=> "Thu, 01/01/1970 - 01:00"

With patch...

>>> \Drupal::service('date.formatter')->format(0, 'custom', '');
=> ""

All that we need to change is to not load the fallback with the $type check...

+      // Fall back to medium if a format was not found.
+      if (empty($format)) {
+        $format = $this->dateFormat('fallback', $langcode)->getPattern();
+      }

^^ This bit needs to happen regardless of type.

Sorry for being dense here. Update to logic, expanded comment, and explicit assertion for the case you identified.

I think #20 has addressed the remaining issues.

Committed and pushed 2736deb to 8.4.x and 53b2726 to 8.3.x. Thanks!

diff --git a/core/lib/Drupal/Core/Datetime/DateFormatter.php b/core/lib/Drupal/Core/Datetime/DateFormatter.php
index 6842022..421b03b 100644
--- a/core/lib/Drupal/Core/Datetime/DateFormatter.php
+++ b/core/lib/Drupal/Core/Datetime/DateFormatter.php
@@ -320,7 +320,7 @@ public function formatDiff($from, $to, $options = array()) {
    * @param string $langcode
    *   The langcode of the language to use.
    *
-   * @return \Drupal\Core\Datetime\DateFormatInterface|NULL
+   * @return \Drupal\Core\Datetime\DateFormatInterface|null
    *   The configuration entity for the date format in the given language for
    *   non-custom formats, NULL otherwise.
    */

Fixed coding standard on commit.

Well, seems something is wrong with the 53b27262cec56eaeca70f90204ce9822f87db1a1 commit.

Can't figure out how the patch below could be related with LocaleTranslatedSchemaDefinitionTest and DateFormatter....

https://www.drupal.org/node/2745123#comment-11949523

https://www.drupal.org/pift-ci-job/604934

UPD: somehow the LocaleTranslatedSchemaDefinitionTest takes a system message displayed after the previous test directory was deleted by simpletest_clean_temporary_directories(), specifically this:

'Removed 1 temporary directory.', 'Removed @count temporary directories.'

...and then uses it as a placeholder in DB query.

Is it expected behaviour? OK, but why it takes not only message from HTML markup but placeholders for the message too?

@drugan 8.3.x HEAD and that test is passing when run locally. If I apply the patch on #2745123: Simpletest module crashes on cleanup, uninstall it fails. Therefore there is a problem with the patch.

OK, I'll try to avoid emitting system messages on the patch when test site directory is successfully removed.