Fix #81500: Interval serialization regression since 7.3.14 / 7.4.2#7572
Fix #81500: Interval serialization regression since 7.3.14 / 7.4.2#7572cmb69 wants to merge 1 commit intophp:PHP-7.4from
Conversation
On unserialization the valid range of `DateInterval::$f` is checked, and values outside this range are explicitly marked as invalid (`-1000000`). To avoid confusion, we should do the same when setting the property in the first place.
| ?> | ||
| --EXPECT-- | ||
| int(1) | ||
| int(-1) |
There was a problem hiding this comment.
Why is it int(-1) while we asked for float(-0.000001)? 🤔
There was a problem hiding this comment.
Negative values are not supposed to be supported; -1 marks invalid µs.
There was a problem hiding this comment.
Sorry but -1 is not a proper way to mark something as invalid, just throw an exception if it's forbidden. Here it will just become muted and unnoticed then doing calculation with it assuming it's a relevant value.
There was a problem hiding this comment.
PHP 7 behavior just completely feel like the correct behavior, you pass a value to ->f serialize/unserialize and find there what you passed, exactly like any other units still do in PHP 8: https://3v4l.org/r0i54
Why would ->f would be a particular case and proceed this insane any-out-of-range-value to -1 transformation?
| if (val >= 0 && val < 1000000) { | ||
| obj->diff->us = val; | ||
| } else { | ||
| obj->diff->us = -1000000; |
There was a problem hiding this comment.
What happen then to the actual value, I ->f is -0.123456 then we just replace it with -1?
kylekatarnls
left a comment
There was a problem hiding this comment.
I don't understand how this can be the expected behavior.
It does not match the expected behavior described on https://bugs.php.net/bug.php?id=81500 which is what we had in PHP 7 and what is actually passed in the beginning to the f property.
|
Setting |
|
So why are we still able to put
<?php
$i = new DateInterval('PT1S');
$i->d = 9985856;
$i->h = -856647;
$i = unserialize(serialize($i));
var_dump($i->d, $i->h);Why only microseconds would suddenly be different in PHP 8.1? Due to If PHP wants to give this second responsibility to
Remember than in real life we're working with interval which may be the result of input/calculation that we don't know the contents: $units = [
'millisecond' => ['f', 1000],
'second' => ['s', 1],
'minute' => ['i', 1],
'hour' => ['h', 1],
];
function getRawDuration(
int $iterations,
string $totalUnit,
int $totalValue,
string $subtractedUnit,
int $subtractedValue
): DateInterval {
global $units;
$interval = new DateInterval('PT0S');
[$key, $factor] = $units[$totalUnit];
$interval->$key += $totalValue / $factor;
[$key, $factor] = $units[$subtractedUnit];
$interval->$key -= $subtractedValue / $factor;
foreach ($units as $unit => [$key]) {
$interval->$key *= $iterations;
}
return $interval;
}
$now = new DateTimeImmutable();
echo $now->add(getRawDuration(3, 'second', 5, 'millisecond', 2589))->format('Y-m-d H:i:s.u');This properly adds to "now" the result of 3 times (5 seconds - 2589 millisecond) and allow dynamically any unit and value, and this will just stop working if we merge. |
|
I get your point; need to check more thoroughly. Thanks! Changing to draft for now. |
|
Closing in favor of PR #7575. |
On unserialization the valid range of
DateInterval::$fis checked,and values outside this range are explicitly marked as invalid
(
-1000000). To avoid confusion, we should do the same when settingthe property in the first place.