DateTime::__construct(): Passing null to parameter #1 ($datetime) of type string is deprecated

Created on 20 May 2022, over 2 years ago
Updated 13 June 2024, 6 months ago

Problem/Motivation

After update Drupal code to 9.3.13 and PHP 8.1 there are DateTime warnings.

Deprecated function: DateTime::__construct(): Passing null to parameter #1 ($datetime) of type string is deprecated in Drupal\Component\Datetime\DateTimePlus->__construct() (line 309 of /var/www/docroot/core/lib/Drupal/Component/Datetime/DateTimePlus.php)
#0 /var/www/docroot/core/includes/bootstrap.inc(346): _drupal_error_handler_real(8192, 'DateTime::__con...', '/var/www/docroo...', 309)
#1 [internal function]: _drupal_error_handler(8192, 'DateTime::__con...', '/var/www/docroo...', 309)
#2 /var/www/docroot/core/lib/Drupal/Component/Datetime/DateTimePlus.php(309): DateTime->__construct(NULL, Object(DateTimeZone))
#3 /var/www/docroot/core/lib/Drupal/Core/Datetime/DrupalDateTime.php(88): Drupal\Component\Datetime\DateTimePlus->__construct(NULL, NULL, Array)
#4 /var/www/docroot/core/lib/Drupal/Core/Datetime/DateHelper.php(477): Drupal\Core\Datetime\DrupalDateTime->__construct(NULL)
#5 /var/www/docroot/modules/contrib/openy_custom/openy_repeat_entity/src/Entity/Repeat.php(230): Drupal\Core\Datetime\DateHelper::daysInYear()
#6 /var/www/docroot/core/lib/Drupal/Core/Entity/EntityFieldManager.php(228): Drupal\openy_repeat_entity\Entity\Repeat::baseFieldDefinitions(Object(Drupal\Core\Entity\ContentEntityType))
#7 /var/www/docroot/core/lib/Drupal/Core/Entity/EntityFieldManager.php(193): Drupal\Core\Entity\EntityFieldManager->buildBaseFieldDefinitions('repeat')
#8 /var/www/docroot/core/lib/Drupal/Core/Entity/EntityFieldManager.php(448): Drupal\Core\Entity\EntityFieldManager->getBaseFieldDefinitions('repeat')
#9 /var/www/docroot/modules/contrib/address/address.tokens.inc(31): Drupal\Core\Entity\EntityFieldManager->getFieldStorageDefinitions('repeat')
#10 [internal function]: address_token_info()

Steps to reproduce

Drupal Core version - 9.3.13
PHP version - 8.1
See warning messages.

🐛 Bug report
Status

Fixed

Version

9.5

Component
Datetime 

Last updated 2 days ago

Created by

🇺🇦Ukraine rollins

Live updates comments and jobs are added and updated live.
  • PHP 8.1

    The issue particularly affects sites running on PHP version 8.1.0 or later.

Sign in to follow issues

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • 🇺🇸United States smustgrave

    Seems all the points have been addressed.

  • Status changed to Needs work almost 2 years ago
  • 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
    1. +++ b/core/lib/Drupal/Core/Datetime/DateHelper.php
      @@ -453,11 +453,13 @@ public static function ampm($required = FALSE) {
      +    if (!empty($date)) {
      +      if (!$date instanceof DrupalDateTime) {
      +        $date = new DrupalDateTime($date);
      +      }
      +      if (!$date->hasErrors()) {
      +        return $date->format('t');
      +      }
           }
           return NULL;
      

      Let's simplify all of these with an early return

      if (empty($date)) {
        return NULL;
      }
      

      Instead of the extra nesting

    2. +++ b/core/tests/Drupal/Tests/Core/Datetime/DateHelperTest.php
      @@ -128,4 +159,94 @@ public function providerTestWeekDaysOrdered() {
      +    // December 31st 2022 is a Saturday.
      ...
      +    // November 30th 2020 is a Monday.
      ...
      +    // December 31st 2022 is a Saturday.
      ...
      +    // November 30th 2020 is a Monday. 2020 is a leap year.
      

      Nit: I don't think it being a Saturday, a Monday, or a leap year make to the test of the number of days in a month or year? Let's keep those comments for the tests of the day of the week name/ID, but not for the days in year/days in month tests.

  • 🇺🇸United States smustgrave

    Tagging for novice as this should be easy.

  • Status changed to Needs review almost 2 years ago
  • Addressed Point-1 from Comment #44. Point-2 could have been a little more elaborate to act upon

  • Status changed to Needs work almost 2 years ago
  • 🇦🇺Australia acbramley

    Changes in #46 are not correct.

    +++ b/core/lib/Drupal/Core/Datetime/DateHelper.php
    @@ -453,11 +453,13 @@ public static function ampm($required = FALSE) {
    +    if (!empty($date)) {
    +      if (!$date instanceof DrupalDateTime) {
    +        $date = new DrupalDateTime($date);
    +      }
    +      if (!$date->hasErrors()) {
    +        return $date->format('t');
    +      }
    

    All of this logic needs to remain, but instead of wrapping the entire block in the if (!empty....

    You do if (empty($date)) and return NULL inside that. Then the remaining logic can be underneath that, un-nested.

  • Status changed to Needs review almost 2 years ago
  • 🇮🇳India adeshsharma Bhopal

    Simplified return of NULL.

  • Status changed to Needs work almost 2 years ago
  • 🇺🇸United States smustgrave

    Why are we removing the comments now?

    Also not the solution has this removed the functionality.

  • 🇺🇦Ukraine ipo4ka704

    Simplified return of NULL.

  • Status changed to Needs review almost 2 years ago
  • 🇺🇦Ukraine ipo4ka704

    Please review.

  • Status changed to Needs work almost 2 years ago
  • The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

    Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

    Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

  • 🇮🇳India TanujJain-TJ

    adding a reroll for drupal 10.1.x with simplified return of NULL

  • Status changed to Needs review almost 2 years ago
  • 🇺🇦Ukraine ipo4ka704

    Re-create patch, with simplified return

  • Status changed to Needs work almost 2 years ago
  • The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

    Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

    Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

  • Status changed to RTBC over 1 year ago
  • 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
  • Status changed to Needs work over 1 year ago
  • 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10

    I think this is a behaviour change here.

    With HEAD, passing NULL would default to 'now' per the constructor of \DrupalDateTime

    e.g.

    > \Drupal\Core\Datetime\DateHelper::daysInMonth()
    
       DEPRECATED  DateTime::__construct(): Passing null to parameter #1 ($datetime) of type string is deprecated in core/lib/Drupal/Component/Datetime/DateTimePlus.php on line 309.
    
    = "31"
    
    

    So yes, there's a deprecation, but it still defaults to today and outputs 31 (Its March when I wrote this comment).

    So I think we probably need a different fix here - one that involves falling back to the current date.

  • 🇦🇺Australia acbramley

    #59 is right - all of the doc blocks state that passing NULL to these functions will use the current date, e.g

       * @param mixed $date
       *   (optional) A DrupalDateTime object or a date string.
       *   Defaults to NULL, which means use the current date.
    
  • Status changed to Needs review over 1 year ago
  • 🇦🇺Australia acbramley

    So to keep parity with the current implementation - NULL, FALSE, and an empty string all default to now. 0 and '0' return NULL.

    To avoid copying the implementation of each into the unit test (because we would need to construct a DateTime object at 'now' and format it, which then means for things like daysInYear we start to just copy the whole implementation since there would need to be the leap year check as well) I've opted with just making sure those 3 values are NOT null. Then checking for nulls in the other 2, then there is an actual implementation check (i.e a date string gives the correct output).

  • Status changed to RTBC over 1 year ago
  • 🇺🇸United States smustgrave

    Change looks good.

    There may be a few duplicates of this but I couldn't find them, but feel I saw one earlier.

  • 🇺🇸United States benjifisher Boston area

    This is unusual: the failing test is not one of the usual FunctionalJavascript (FJS) tests:

    User.Drupal\Tests\user\Kernel\Migrate\MigrateUserStubTest
    	✗	
    Drupal\Tests\user\Kernel\Migrate\MigrateUserStubTest
    
    fail: [Other] Line 0 of sites/default/files/simpletest/phpunit-1178.xml:
    PHPUnit Test failed to complete; Error: PHPUnit 9.5.28 by Sebastian Bergmann and contributors.
    
    Testing Drupal\Tests\user\Kernel\Migrate\MigrateUserStubTest
    F                                                                   1 / 1 (100%)
    
    Time: 00:01.530, Memory: 4.00 MB
    
    There was 1 failure:
    
    1) Drupal\Tests\user\Kernel\Migrate\MigrateUserStubTest::testStub
    Failed asserting that an object is empty.
    
    /var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:122
    /var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:55
    /var/www/html/core/modules/migrate_drupal/src/Tests/StubTestTrait.php:22
    /var/www/html/core/modules/user/tests/src/Kernel/Migrate/MigrateUserStubTest.php:35
    /var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
    
    FAILURES!
    Tests: 1, Assertions: 8, Failures: 1.
    

    I do not see why it would fail, and it passes when I run the test locally. Neither the user test nor the trait that does most of the work has changed in almost two years.

    I am setting the status back to RTBC.

  • 🇦🇺Australia acbramley

    again...

  • 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10

    Re #64 📌 Improve StringItem::generateSampleValue() Fixed this was the cause of the random fail in the migrate test

  • 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10

    Queued a 9.5 run

  • 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10

    Crediting @igorbiki who pointed out the same thing as me in #40

  • 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10

    Adding credits for @adeshsharma, @TanujJain-TJ and @Rishabh Vishwakarma - whilst their patches were not part of the final solution, they were trying to move the issue forward. Thanks folks, hopefully you picked up some new learning as part of contributing to this issue 💪

    • larowlan committed d8436d19 on 10.0.x
      Issue #3281557 by smustgrave, ipo4ka704, acbramley, ravi.shankar,...
    • larowlan committed 2ce27f62 on 10.1.x
      Issue #3281557 by smustgrave, ipo4ka704, acbramley, ravi.shankar,...
  • 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10

    Pushed to 10.1.x and backported to 10.0.x

    Waiting for a test run on 9.5.x before backporting there, so leaving at RTBC

  • 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
    • larowlan committed 68c54bf3 on 9.5.x
      Issue #3281557 by smustgrave, ipo4ka704, acbramley, ravi.shankar,...
  • Status changed to Fixed over 1 year ago
  • 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10

    Backported to 9.5.x - thanks all

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

  • Status changed to Fixed 6 months ago
  • Can someone explain why we return untranslated string, though the comment says that it should be translated?

      /**
       * Returns translated name of the day of week for a given date.
       *
    
    return $days[$dow]->getUntranslatedString();
    

    Seems it was brought by these changes.

Production build 0.71.5 2024