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

Fix looks good to me, but the patch fails to apply.

Fixed the patch in #1. Issue was with the file path, changed it from a/lib/Drupal/Component/Datetime/DateTimePlus.php to a/core/lib/Drupal/Component/Datetime/DateTimePlus.php.

Thanks!

I think that it is better change the first line of the method and throw the exception when the $timestamp is not an integer instead of not numeric. We also need a test.

Just to add that I believe this issue comes from ultimate_cron module.
Created an issue for it:
#3281350: Deprecated function: Implicit conversion from float-string "<float>" to int loses precision

Though i do think that @daffie is correct this should be changed to throw and error with invalid int though I'm sure that will need more testing and possible cause issues in other places.

PHP 8.1 and Drupal 9.4.5

How to reproduce

Go to /admin/structure/migrate/manage/migrate_drupal_7/migrations

Seems to be coming from `migrate_tools`.

Installed 3-part migrate_tools

"name": "drupal/migrate_tools",
"version": "5.1.0",

Dependencies prevent me from testing migrate_tools 6.x that have improved PHP 8.1 support. The wider question is perhaps how core should handle these type of situations?

Deprecated function: Implicit conversion from float 1658930740.677 to int loses precision in Drupal\Component\Datetime\DateTimePlus->__call() (line 360 of core/lib/Drupal/Component/Datetime/DateTimePlus.php).

Drupal\Component\Datetime\DateTimePlus->__call('setTimestamp', Array) (Line: 204)
Drupal\Component\Datetime\DateTimePlus::createFromTimestamp(1658930740.677, Object, Array) (Line: 122)
Drupal\Core\Datetime\DateFormatter->format(1658930740.677, 'custom', 'Y-m-d H:i:s') (Line: 201)
Drupal\migrate_tools\Controller\MigrationListBuilder->buildRow(Object) (Line: 219)
Drupal\Core\Entity\EntityListBuilder->render() (Line: 23)
Drupal\Core\Entity\Controller\EntityListController->listing('migration')
call_user_func_array(Array, Array) (Line: 123)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 564)
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object) (Line: 124)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext(Array, Array) (Line: 97)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 158)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1) (Line: 80)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 58)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 48)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 106)
Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1) (Line: 85)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1) (Line: 48)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 51)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 23)
Stack\StackedHttpKernel->handle(Object, 1, 1) (Line: 709)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)

It works with drupal 9.5-beta.2
Thank you

Hi @daffie, I have changed is_numeric to is_int to verify whether the number is an integer, perhaps we could remove the typecast below as logically it would never reach that point if it were not an integer. Testing still needs work.

I agree that the type cast to int can be removed.

Appears there are errors in #10

Also was tagged for tests so moving back to NW for both.

Seems like #10 does not work for other scenarios like:

InvalidArgumentException: The timestamp must be numeric. in Drupal\Component\Datetime\DateTimePlus::createFromTimestamp() (line 201 of core/lib/Drupal/Component/Datetime/DateTimePlus.php).
Drupal\Core\Datetime\DateFormatter->format('1670531360', 'medium', '', 'America/New_York', 'en') (Line: 183)
Drupal\Core\Field\Plugin\Field\FieldFormatter\TimestampFormatter->viewElements(Object, NULL) (Line: 89)

The timestamp is valid, but it's a string. So we can check if it's numeric a if it's cast the value to (int)

Same as #14, fixed to work with a new Drupal installation.

Rerolled to 10.x and queued for retest.
Still needs tests.

#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::testTimestamp. 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: Improve text for parameter type hinting in function declaration