fix: apply sign of chronometer and time strings to whole Duration
Duration::fromChronoString() and Duration::fromTimeString() only negated the hour component of a signed input. When the hour was zero or absent the sign was silently dropped, and when it was present the minute/second/fraction components stayed positive, producing a duration that did not match the input. Prefix every unit token with the sign so the whole duration is negated consistently.
This commit is contained in:
dualfroz committed
1 parent
3afc737d52
commit
ca720ad7ea
3 files changed
+171
-9
No files matched your search
@@ -0,0 +1,104 @@
|
||||
# fix: apply the sign of chronometer/time strings to the whole Duration
|
||||
|
||||
## Problem
|
||||
|
||||
`Duration::fromChronoString()` and `Duration::fromTimeString()` accept an optional
|
||||
leading sign so callers can build negative durations (the grammar defines a
|
||||
`(?<sign>\+|-)?` group). The sign was mishandled in two ways:
|
||||
|
||||
1. When the hour component is zero or absent, the sign is silently lost and a
|
||||
positive duration is returned.
|
||||
2. When the hour component is present, only the hour is negated while the minute,
|
||||
second and fraction components stay positive, producing a duration with mixed
|
||||
sign components that does not represent the input.
|
||||
|
||||
Examples on the current code:
|
||||
|
||||
```
|
||||
Duration::fromChronoString('-05:30'); // minute:second -> +5m30s (sign lost)
|
||||
Duration::fromChronoString('-1:05:30'); // -1h +5m +30s = -3270s, not -3930s
|
||||
Duration::fromTimeString('-00:30'); // -> +30m (sign lost)
|
||||
Duration::fromTimeString('-01:30'); // -1h +30m = -1800s, not -5400s
|
||||
```
|
||||
|
||||
Applying these durations to a date point therefore moves the date in the wrong
|
||||
direction or by the wrong amount.
|
||||
|
||||
## Root cause
|
||||
|
||||
`src/Duration.php`, `Duration::fromUnits()`. The negative sign was concatenated
|
||||
onto the hour string only:
|
||||
|
||||
```php
|
||||
if ('-' === $units['sign']) {
|
||||
$units['hour'] = '-'.$units['hour'];
|
||||
}
|
||||
```
|
||||
|
||||
Because the string handed to `DateInterval::createFromDateString()` only carried a
|
||||
sign on the `hours` token, `-0 hours 30 minutes` normalized to a positive
|
||||
30-minute interval, and `-1 hours 5 minutes 30 seconds` kept the minute and second
|
||||
tokens positive.
|
||||
|
||||
## The fix
|
||||
|
||||
Prefix every unit token with the sign so the whole duration is negated
|
||||
consistently:
|
||||
|
||||
```php
|
||||
$sign = '-' === $units['sign'] ? '-' : '';
|
||||
|
||||
return self::fromDateString(
|
||||
$sign.$units['hour'].' hours '
|
||||
.$sign.$units['minute'].' minutes '
|
||||
.$sign.$units['second'].' seconds '
|
||||
.$sign.$units['fraction'].' microseconds'
|
||||
);
|
||||
```
|
||||
|
||||
`DateInterval::createFromDateString()` accepts a per-token sign, so this produces
|
||||
a `DateInterval` whose components all carry the correct sign. Verified against a
|
||||
reference date:
|
||||
|
||||
```
|
||||
2021-01-01 00:10:00 + fromChronoString('-05:30') = 2021-01-01 00:04:30
|
||||
2021-01-01 02:00:00 + fromChronoString('-1:05:30') = 2021-01-01 00:54:30
|
||||
2021-01-01 00:20:00 + fromChronoString('-12:28.5') = 2021-01-01 00:07:31.500000
|
||||
2021-01-01 01:00:00 + fromTimeString('-00:30') = 2021-01-01 00:30:00
|
||||
2021-01-01 05:00:00 + fromTimeString('-01:30') = 2021-01-01 03:30:00
|
||||
```
|
||||
|
||||
## Tests
|
||||
|
||||
New data provider `signedDurationProvider` with the test
|
||||
`testSignedStringsApplySignToWholeDuration` asserts the real elapsed effect of the
|
||||
signed strings by applying them to a reference `DateTimeImmutable` (helper
|
||||
independent, covers the zero-hour, full h:m:s and fractional-second cases).
|
||||
|
||||
Two existing data-provider rows pinned the buggy output and encode the defect, so
|
||||
they were corrected:
|
||||
|
||||
- `fromChronoProvider` `negative chrono`: `-12:28.5 => PT12M28.5S` (sign dropped)
|
||||
updated to a fully negative case `-12:28 => PT-12M-28S`.
|
||||
- `fromTimeStringProvider` `negative chrono`: `-12:28 => PT-12H28M` (mixed sign)
|
||||
updated to `-12:28 => PT-12H-28M`.
|
||||
- `fromTimeStringProvider` `negative chrono with seconds`: `-00:00:28.5 => PT28.5S`
|
||||
(sign dropped) updated to `-01:02:28 => PT-1H-2M-28S`.
|
||||
|
||||
The fractional-second and seconds-only negative cases are covered by the new
|
||||
date-arithmetic test instead of the string helper, because that helper renders
|
||||
negative fractional seconds and bare seconds-only intervals with pre-existing
|
||||
quirks that are out of scope here.
|
||||
|
||||
## Gate
|
||||
|
||||
```
|
||||
composer install && vendor/bin/phpunit --no-coverage
|
||||
```
|
||||
|
||||
Result: exit code `0` - `OK (562 tests, 933 assertions)`.
|
||||
|
||||
Counterfactual: reverting `fromUnits()` to the hour-only sign makes all five
|
||||
`testSignedStringsApplySignToWholeDuration` cases fail, e.g.
|
||||
`fromTimeString('-01:30')` yields `2021-01-01 04:30:00` instead of the expected
|
||||
`2021-01-01 03:30:00`.
|
||||
+5
-4
@@ -188,12 +188,13 @@ final class Duration
|
||||
{
|
||||
$units += ['hour' => '0', 'minute' => '0', 'second' => '0', 'fraction' => '0', 'sign' => '+'];
|
||||
$units['fraction'] = str_pad($units['fraction'] ?? '000000', 6, '0');
|
||||
if ('-' === $units['sign']) {
|
||||
$units['hour'] = '-'.$units['hour'];
|
||||
}
|
||||
$sign = '-' === $units['sign'] ? '-' : '';
|
||||
|
||||
return self::fromDateString(
|
||||
$units['hour'].' hours '.$units['minute'].' minutes '.$units['second'].' seconds '.$units['fraction'].' microseconds'
|
||||
$sign.$units['hour'].' hours '
|
||||
.$sign.$units['minute'].' minutes '
|
||||
.$sign.$units['second'].' seconds '
|
||||
.$sign.$units['fraction'].' microseconds'
|
||||
);
|
||||
}
|
||||
|
||||
|
||||
+62
-5
@@ -241,8 +241,8 @@ final class DurationTest extends TestCase
|
||||
'expected' => 'PT1H2M3.0004S',
|
||||
],
|
||||
'negative chrono' => [
|
||||
'chronometer' => '-12:28.5',
|
||||
'expected' => 'PT12M28.5S',
|
||||
'chronometer' => '-12:28',
|
||||
'expected' => 'PT-12M-28S',
|
||||
],
|
||||
];
|
||||
}
|
||||
@@ -282,11 +282,68 @@ final class DurationTest extends TestCase
|
||||
],
|
||||
'negative chrono' => [
|
||||
'chronometer' => '-12:28',
|
||||
'expected' => 'PT-12H28M',
|
||||
'expected' => 'PT-12H-28M',
|
||||
],
|
||||
'negative chrono with seconds' => [
|
||||
'chronometer' => '-00:00:28.5',
|
||||
'expected' => 'PT28.5S',
|
||||
'chronometer' => '-01:02:28',
|
||||
'expected' => 'PT-1H-2M-28S',
|
||||
],
|
||||
];
|
||||
}
|
||||
|
||||
/**
|
||||
* A signed chronometer or time string must apply the sign to the whole
|
||||
* duration, not only to the hour component. Previously the sign was lost
|
||||
* entirely when the hour was zero and, when the hour was present, only the
|
||||
* hour was negated while minutes and seconds stayed positive.
|
||||
*/
|
||||
#[DataProvider('signedDurationProvider')]
|
||||
public function testSignedStringsApplySignToWholeDuration(string $method, string $input, string $reference, string $expected): void
|
||||
{
|
||||
$duration = Duration::$method($input);
|
||||
$date = new \DateTimeImmutable($reference);
|
||||
|
||||
self::assertSame(
|
||||
$expected,
|
||||
$date->add($duration->dateInterval)->format('Y-m-d H:i:s.u')
|
||||
);
|
||||
}
|
||||
|
||||
/**
|
||||
* @return iterable<string, array{method:string, input:string, reference:string, expected:string}>
|
||||
*/
|
||||
public static function signedDurationProvider(): iterable
|
||||
{
|
||||
return [
|
||||
'chrono negative minute and second without hour' => [
|
||||
'method' => 'fromChronoString',
|
||||
'input' => '-05:30',
|
||||
'reference' => '2021-01-01 00:10:00.000000',
|
||||
'expected' => '2021-01-01 00:04:30.000000',
|
||||
],
|
||||
'chrono negative hour minute second' => [
|
||||
'method' => 'fromChronoString',
|
||||
'input' => '-1:05:30',
|
||||
'reference' => '2021-01-01 02:00:00.000000',
|
||||
'expected' => '2021-01-01 00:54:30.000000',
|
||||
],
|
||||
'chrono negative fractional second' => [
|
||||
'method' => 'fromChronoString',
|
||||
'input' => '-12:28.5',
|
||||
'reference' => '2021-01-01 00:20:00.000000',
|
||||
'expected' => '2021-01-01 00:07:31.500000',
|
||||
],
|
||||
'time negative with zero hour' => [
|
||||
'method' => 'fromTimeString',
|
||||
'input' => '-00:30',
|
||||
'reference' => '2021-01-01 01:00:00.000000',
|
||||
'expected' => '2021-01-01 00:30:00.000000',
|
||||
],
|
||||
'time negative hour and minute' => [
|
||||
'method' => 'fromTimeString',
|
||||
'input' => '-01:30',
|
||||
'reference' => '2021-01-01 05:00:00.000000',
|
||||
'expected' => '2021-01-01 03:30:00.000000',
|
||||
],
|
||||
];
|
||||
}
|
||||
|
||||
Reference in new issue
Block a user