Problem/Motivation

When you call \Drupal::service('date.formatter')->format($timestamp, $type = '', $format = 'd.d.d.d') Drupal tries to load the date format based on the type because it is not custom but will not found any format for empty string. This can lead to unnecessary calls.
See profiler images before/after in #3519986: Set missing date format type.

Steps to reproduce

Proposed resolution

Teach people via phpdoc non-empty-string (https://phpstan.org/writing-php-code/phpdoc-types#other-advanced-string-...) and verify the type is not falsely before try to load the format.

Remaining tasks

Ensure test-only test is failing.

User interface changes

N/A

Introduced terminology

N/A

API changes

N/A-

Data model changes

N/A-

Release notes snippet

N/A-

Issue fork drupal-3520188

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:

About issue forks

Comments

tobiasb created an issue. See original summary.

tobiasb opened merge request !11893

tobiasb’s picture

tobiasb
Primary language German
Location Berlin
commented
Assigned: tobiasb » Unassigned
Issue summary: View changes
Status: Active » Needs review
tobiasb’s picture

tobiasb
Primary language German
Location Berlin
commented
Issue summary: View changes
smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Ran the test-only feature and unfortunately it passed when it was expected to fail. So think the test needs a little tweaking

tobiasb’s picture

tobiasb
Primary language German
Location Berlin
commented
Status: Needs work » Needs review

With NULL as value, we get an error.

Cannot load the "date_format" entity with NULL ID.

oily’s picture

oily commented

Here is the test-only output:

PHPUnit 11.5.42 by Sebastian Bergmann and contributors.
Runtime:       PHP 8.4.13
Configuration: /builds/issue/drupal-3520188/core/phpunit.xml.dist
F.                                                                  2 / 2 (100%)
Time: 00:01.765, Memory: 8.00 MB
There was 1 failure:
1) Drupal\KernelTests\Core\Datetime\DateFormatterTest::testFormat
Cannot load the "date_format" entity with NULL ID.
/builds/issue/drupal-3520188/core/lib/Drupal/Core/Entity/EntityStorageBase.php:265
/builds/issue/drupal-3520188/core/lib/Drupal/Core/Datetime/DateFormatter.php:344
/builds/issue/drupal-3520188/core/lib/Drupal/Core/Datetime/DateFormatter.php:129
/builds/issue/drupal-3520188/core/tests/Drupal/KernelTests/Core/Datetime/DateFormatterTest.php:75
FAILURES!
Tests: 2, Assertions: 10, Failures: 1.
Exiting with EXIT_CODE=1
oily’s picture

oily commented

Re #5, as stated in #6 the test-only test is failing as described in #6.

Updated the IS.

Moving to RTBTC, but not sure. With the test failing does that mean that it does not run the assertions later in the file? It stops at the failure? If so, is there another assertion that could be used that would not stop the test?

The error message comes from line 261 in EntityStorageBase.php:

assert(!is_null($id), sprintf('Cannot load the "%s" entity with NULL ID.', $this->entityTypeId));

Could an assertion like this one be used instead from line 566 in ConfigEntityStorageTest.php: 

$this->expectExceptionMessage(sprintf('Cannot load the "%s" entity with NULL ID.', $this->entityTypeId));
oily’s picture

oily commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
oily’s picture

oily commented
Status: Reviewed & tested by the community » Needs review
oily’s picture

oily commented

After trying things mentioned in #8 have abandoned the attempt.

tobiasb’s picture

tobiasb
Primary language German
Location Berlin
commented
Issue summary: View changes
tobiasb’s picture

tobiasb
Primary language German
Location Berlin
commented

I replaced the kernel test with a unit test.

A test only would result into:

❯ ddev phpunit core/tests/Drupal/Tests/Core/Datetime/DateFormatterTest.php
Clearing old webdriver sessions
PHPUnit 11.5.42 by Sebastian Bergmann and contributors.

Runtime:       PHP 8.3.25
Configuration: /var/www/html/core/phpunit.xml.dist

F                                                                   1 / 1 (100%)

Time: 00:00.032, Memory: 6.00 MB

There was 1 failure:

1) Drupal\Tests\Core\Datetime\DateFormatterTest::testFormat
Drupal\Core\Language\LanguageManagerInterface::getConfigOverrideLanguage() was not expected to be called.

/var/www/html/core/lib/Drupal/Core/Datetime/DateFormatter.php:342
/var/www/html/core/lib/Drupal/Core/Datetime/DateFormatter.php:129
/var/www/html/core/tests/Drupal/Tests/Core/Datetime/DateFormatterTest.php:39
mstrelan’s picture

mstrelan commented
Status: Needs review » Needs work

Can we possibly use the existing unit test at \Drupal\Tests\Core\Datetime\DateTest?

tobiasb’s picture

tobiasb
Primary language German
Location Berlin
commented
Status: Needs work » Needs review

@mstrelan

Done. Somehow I could not believe that we do not have a unit test for the date formatter. :D

mstrelan’s picture

mstrelan commented
Status: Needs review » Reviewed & tested by the community

Only other suggestion is that we could also update the @param doc in DateFormatter::dateFormat to non-empty-string and potentially also expand the doc for $dateFormats property to something like array<non-empty-string, array<string, \Drupal\Core\Datetime\DateFormatInterface>>, but not sure we want to include that here, so setting to RTBC.

  • catch committed 354ff717 on 11.3.x 
    fix: #3520188 Do not try to load date format when custom date format is...

  • catch committed 2d616ade on 11.x 
    fix: #3520188 Do not try to load date format when custom date format is...

catch closed merge request !11893

catch’s picture

catch
he/him
Primary language English
commented
Version: 11.x-dev » 11.3.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 11.x and cherry-picked to 11.3.x, thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

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