-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix deprecation for PHP 8.1 #3449
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, since $time > 1000
, we're not interested in the decimals and can therefore safely cast $time
to int
, discarding those decimals.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, finally had a chance to have a closer look, but this fix is incorrect as it doesn't fix anything.
Division with floats works perfectly fine without deprecation notice in PHP 8.1, this line (73) is not the "problem" line.
The actual problem is in line 67:
$secs = round((($time % 60000) / 1000), 2);
As per the PHP manual on arithmetic operators:
Operands of modulo are converted to int before processing. For floating-point modulo, see
fmod()
.
And as fmod()
has been available since PHP 4.2.0, it can safely be used to maintain the behaviour as it was previously.
Ref: https://www.php.net/manual/en/function.fmod.php
In other words, line 67 should be changed to the below to solve the actual problem:
$secs = round((fmod($time, 60000) / 1000), 2);
Also see:
https://3v4l.org/r3DZZ (without fix)
https://3v4l.org/9FelR (with correct fix)
1a531cf
to
f1fc1bf
Compare
@jrfnl thank you for review, fixed feedback... it's strange how I mixed lines |
@andypost No worries, happens to the best of us, but it does very much explain my confusion when I initially looked at the PR. |
Closes #3448
With this fix I got clean reports
Steps https://3v4l.org/flZrS