dualfroz
ca720ad7ea
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.
105 lines
3.8 KiB
Markdown
105 lines
3.8 KiB
Markdown
# 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`.
|