From ca720ad7eadfd5fb7aae3fde62c1ef274bea7da6 Mon Sep 17 00:00:00 2001 From: dualfroz Date: Fri, 4 Sep 2026 09:03:39 +0200 Subject: [PATCH] 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. --- PR.md | 104 +++++++++++++++++++++++++++++++++++++++++++ src/Duration.php | 9 ++-- src/DurationTest.php | 67 +++++++++++++++++++++++++--- 3 files changed, 171 insertions(+), 9 deletions(-) create mode 100644 PR.md diff --git a/PR.md b/PR.md new file mode 100644 index 0000000..e7e160e --- /dev/null +++ b/PR.md @@ -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 +`(?\+|-)?` 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`. diff --git a/src/Duration.php b/src/Duration.php index a8015cb..d0b2f32 100644 --- a/src/Duration.php +++ b/src/Duration.php @@ -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' ); } diff --git a/src/DurationTest.php b/src/DurationTest.php index 487204c..fd71bbb 100644 --- a/src/DurationTest.php +++ b/src/DurationTest.php @@ -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 + */ + 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', ], ]; }