Deprecated function: Implicit conversion from float-string <float> to int loses precision in Drupal\Component\Datetime\DateTimePlus

#16 works for me on 9.4.9

We also used a patch from #10 on our project and it caused a fatal error: InvalidArgumentException: The timestamp must be numeric. in Drupal\Component\Datetime\DateTimePlus::createFromTimestamp() (line 201 of core/lib/Drupal/Component/Datetime/DateTimePlus.php).

Applying a patch from #16 fixed this issue. Thanks, @anacona16

Based on the merge in #18 the merge passed the tests.

Still needs tests though

Added test case with different datatypes of the timestamp passed to the static method.

Test does cover the scenario.
Ran locally without the fix and got

Unsilenced deprecation notices (2)

  1x: Implicit conversion from float-string "87654321.1" to int loses precision
    1x in DrupalDateTimeTest::testTimestampArgumentTypes from Drupal\Tests\system\Functional\Datetime

  1x: Implicit conversion from float 87654321.1 to int loses precision
    1x in DrupalDateTimeTest::testTimestampArgumentTypes from Drupal\Tests\system\Functional\Datetime

Given this is a public static method it is super tempting to add a typehint to the method and remove the is_numeric check... like

  public static function createFromTimestamp(int $timestamp, $timezone = NULL, $settings = []) {
    $datetime = new static('', $timezone, $settings);
    $datetime->setTimestamp($timestamp);
    return $datetime;
  }

Going to ping release managers and ask for a thought. Also FWIW the calling code could cast to an integer from a float. It shouldn't be passing floats in here. Why is a float being passed in here?

We could tidy up this code in migrate plus to not pass in a float.

        $row['last_imported'] = $date_formatter->format($last_imported / 1000,
          'custom', 'Y-m-d H:i:s');

Because PHP is not going to do the right thing... see https://3v4l.org/HDF4t

Probably would make the code cleaner to use typehints vs (int) check.

Tests should still cover that.

Minor formatting.

Can we improve the issue summary a little? such as with standard template

Per #32, update issue summary with default template and relevant details

I see that the newly added tests are in Functional tests Drupal\Tests\system\Functional\Datetime\DrupalDateTimeTest, however there are already existing unit tests in Drupal\Tests\Component\Datetime\DateTimePlusTest. This looks more appropriate to add to the unit test class as they would be better grouped and much faster?

there are a few more references to setTimestamp that have been missed here in this patch

Regarding comments #28-30 that suggest using type hinting, wouldn't this cause issues for code out in the wild that call the method and currently does not cast the first $timestamp parameter? If we go down this route, should it have a BC layer to warn and eventually enforce the parameter strict typing?

IMO the current approach in https://git.drupalcode.org/project/drupal/-/merge_requests/3254 is more accommodating and would cause less issues for existing implementations.

Thoughts?

When it's the return type, you can use the ReturnTypeWillChange attribute. I don't think there's something similar for parameters.

Also, this seems relevant: #1158720: [policy, no patch] Add parameter type hinting to function declaration coding standards →