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

Can you confirm this is caused by module openy_custom passing a null?

The problem comes from using the DateHelper::daysInYear() method without specifying date. It can be reproduced by creating a simple controller with a call to the method daysInYear.

Proposed solution is to not pass NULL value to the DrupalDateTime constructor within daysInYear method:

if (!$date instanceof DrupalDateTime) {
  $date = new DrupalDateTime();
}

We are using #2 in Open Y - Drupal core 9.4.1, PHP 8.1.4 - no more overwhelming log messages in Grafana

@podarok, could you please have a look at the following https://git.drupalcode.org/project/drupal/-/merge_requests/2300/diffs to see if this alleviates the log messages problem? I think it's a better solution since the problem comes from the
#5 /var/www/docroot/modules/contrib/openy_custom/openy_repeat_entity/src/Entity/Repeat.php(230): Drupal\Core\Datetime\DateHelper::daysInYear()
The actual method daysInYear() performs inaccurate construction of the DrupalDateTime object.

I've tested the patch from https://git.drupalcode.org/project/drupal/-/merge_requests/2300/diffs and it works for me on Drupal 9.3.17 with PHP 8.1. It looks like the MR needs to be rebased now that it's on 9.5 and also I see @larowlan flagged that it needs tests. I'm not super familiar with core testing but if someone points me in the right direction I should be able to try.

Can knock out the tests today I think. Question though should we do the same for the other functions in DateHelper that use the same code?

So the PR seemed the break the ability to pass in strings to the functions and would just result in NULL being returned.

Fixed those for all 4 functions and added test cases

Wrong patch

This is the same patch as #17 but adding a tests-only patch also

Looks good

foo.php
<?php
use Drupal\Core\Datetime\DateHelper;

print_r(DateHelper::daysInMonth(FALSE));

drush scr foo.php
31

curl -0 https://www.drupal.org/files/issues/2022-07-14/3281557-18.patch | git apply --index

drush scr foo.php

+++ b/core/lib/Drupal/Core/Datetime/DateHelper.php
@@ -453,10 +453,10 @@ public static function ampm($required = FALSE) {
    */
   public static function daysInMonth($date = NULL) {
-    if (!$date instanceof DrupalDateTime) {
+    if ($date && !$date instanceof DrupalDateTime) {
       $date = new DrupalDateTime($date);

I think this should either be !isset($date) or \is_null($date), or we should add explicit test coverage for 0, '', FALSE etc.

Adding a patch which handles NULL & empty condition.

Not sure if #24 was tested before being uploaded but it didn't wrap things correctly so ignoring and updating #18

#27 is what I was thinking of, thanks!

Last questions here:

I don't think we should do it in this issue, but I think it's worth a follow-up to consider deprecating passing NULL (and possibly any falsy value at all).

In this issue, I think it would be good to add test coverage for FALSE, '', 0, '0' with a @todo pointing to that issue, so that we pick up on any further type changes in PHP in the future.

+++ b/core/tests/Drupal/Tests/Core/Datetime/DateHelperTest.php
@@ -128,4 +159,76 @@ public function providerTestWeekDaysOrdered() {
+   */
+  public function testDaysInMonth() {
+    // Pass nothing and expect to get NULL.
+    $this->assertNull(DateHelper::daysInMonth());
+

For example adding some extra assertions just after the NULL one.

Updated the tests which required adding a !empty to the DateTimeHelper also. Opened up https://www.drupal.org/project/drupal/issues/3299788 too.

isset() && !empty() is redundant - just use the latter. Also not seeing a todo with a link?

Reason I added both is that if '' or 0 is passed the empty check will catch it.

+++ b/core/lib/Drupal/Core/Datetime/DateHelper.php
@@ -453,7 +453,7 @@
+    if (isset($date) && !empty($date)) {

These can all just be !empty

https://stackoverflow.com/a/4559976/2259943

Made changes as per comment #34.

Setting status to needs review as patch #35 passes the tests.

All we're missing now is the @todo asked for in #28

Not sure I follow what the todo should go to?

Shouldn't NULL/nothing be a valid $date param value, and return current month number of days like stated in param docstring?

I have not reviewed the code changes myself, but here is a patch based on the one in #35 that adds the @todo comment requested in #28.

It is too late for 9.4.x. I am changing the target branch to 10.1.x.

@igorbiki, the current behavior matches the documented parameter string. The suggestion is that we change both. I do not have an opinion on that, but the right place to discuss it is the follow-up issue: #3299788: Follow up to 3281557 - Consider deprecating passing null to DateTime Helper.

Seems all the points have been addressed.

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 a difference to the tests for 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.

Tagging for novice as this should be easy.

Addressed Point-1 from Comment #44. Point-2 could have been a little more elaborate to act upon

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.

Simplified return of NULL.

Why are we removing the comments now?

Also not the solution has this removed the functionality.

Simplified return of NULL.

Please review.

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.

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

Re-create patch, with simplified return

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.

Fixed stan failure in #54 and also added missing null return types and docs. Hiding all old patches as they're stacking up.

Thank you @acbramley

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.

#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.

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).

Change looks good.

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

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.

again...

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

Queued a 9.5 run

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

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 💪

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

Backported to 9.5.x - thanks all

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.