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

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

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.

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.

The last submitted patch, 61: 3281557-61.patch, failed testing. View results →

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.

The last submitted patch, 61: 3281557-61.patch, failed testing. View results →

again...

Re #64 📌 Improve StringItem::generateSampleValue() Fixed 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

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

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.