Fix #81500: Interval serialization regression since 7.3.14 / 7.4.2

[cmb69]

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.

[kylekatarnls]

Why is it int(-1) while we asked for float(-0.000001)? 🤔

[cmb69]

Negative values are not supposed to be supported; -1 marks invalid µs.

[kylekatarnls]

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.

[kylekatarnls]

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?

[kylekatarnls]

What happen then to the actual value, I ->f is -0.123456 then we just replace it with -1?

[kylekatarnls]

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.

[cmb69]

Setting ::$f to a value which is not in the valid range shouldn't have been allowed in the first place, and according to @derickr negative values are not supported. timelib's marker for invalid µs is -1000000. I'm also fine with throwing an exception or raising a warning if ::$f is set to an invalid value, but since unserialization silently uses -1000000, I've went with this for now. Let's wait on @derickr's review. :)

[kylekatarnls]

So why are we still able to put ->s = 70 or ->s = -6 this is inconsistent to say there is a range validation for microseconds but not for all other units.

DateInterval allows to put any value in any unit (and BTW serialize properly):

<?php

$i = new DateInterval('PT1S');

$i->d = 9985856;
$i->h = -856647;

$i = unserialize(serialize($i));

var_dump($i->d, $i->h);

https://3v4l.org/HTfLd

Why only microseconds would suddenly be different in PHP 8.1?

Due to DateInterval accepting almost anything, I always though it was intended to be responsible for carrying values but that value validation was left to implementation.

If PHP wants to give this second responsibility to DateInterval

• please prefer explicit exception (or at least a null / false signal) which would let a chance to user to know it was actually refused. Because replacing all 1, 2, -2, -1 with -1 just sounds to me like the most error-prone possible way to flag invalid value.
• please apply the same rules to all units so it makes results reliably predictable.

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.

[cmb69]

I get your point; need to check more thoroughly. Thanks! Changing to draft for now.

[cmb69]

Closing in favor of PR #7575.