From 5763d58d02435261a567aca7254b92f4c2cba88c Mon Sep 17 00:00:00 2001 From: DualFroz Date: Thu, 3 Sep 2026 14:39:22 +0200 Subject: [PATCH] fix: return 0 from length() when excluded boundaries leave a period empty Excluding the boundaries of a one or two unit long period pushes the included start past the included end. diff()->days is unsigned and the HOUR/MINUTE/SECOND branch applied abs() outright, so that backwards range was measured as a positive distance and the unconditional +1 inflated it further: a one day period reported 1, 2, 2 and 3 for EXCLUDE_NONE, START, END and ALL, while iterating the same objects yielded nothing. Return 0 up front when the included start is past the included end. This matches what the MONTH and YEAR precisions already returned through iterator_count(), and lets the now unreachable abs() go. --- PR.md | 163 +++++++++++++++++++++++++++++ src/PeriodTraits/PeriodGetters.php | 9 +- tests/BoundaryTest.php | 5 + tests/Datasets/Dates.php | 32 ++++++ 4 files changed, 208 insertions(+), 1 deletion(-) create mode 100644 PR.md diff --git a/PR.md b/PR.md new file mode 100644 index 0000000..c085342 --- /dev/null +++ b/PR.md @@ -0,0 +1,163 @@ +# Fix length() growing instead of shrinking when boundaries are excluded + +## Problem + +`Period::length()` is supposed to count the units a period covers, and excluding a boundary +is supposed to remove one. It does exactly that for ordinary periods: + +```php +Period::make('2018-01-01', '2018-01-31', boundaries: Boundaries::EXCLUDE_NONE())->length(); // 31 +Period::make('2018-01-01', '2018-01-31', boundaries: Boundaries::EXCLUDE_ALL())->length(); // 29 +``` + +But for a period only one or two units long, excluding boundaries makes `length()` **grow**: + +```php +$b = ['2021-01-01', '2021-01-01', Precision::DAY()]; + +Period::make(...$b, Boundaries::EXCLUDE_NONE())->length(); // 1 <- correct +Period::make(...$b, Boundaries::EXCLUDE_START())->length(); // 2 <- was 1, excluded one day, got 2 +Period::make(...$b, Boundaries::EXCLUDE_END())->length(); // 2 +Period::make(...$b, Boundaries::EXCLUDE_ALL())->length(); // 3 <- excluded both days, got 3 +``` + +The library already knows the right answer. Iterating the very same objects yields nothing, +and `includedStart` has moved *past* `includedEnd`: + +| boundaries | `length()` | `iterator_count()` | `includedStart` | `includedEnd` | +|---|---|---|---|---| +| `EXCLUDE_NONE` | 1 | 1 | 2021-01-01 | 2021-01-01 | +| `EXCLUDE_START` | **2** | 0 | 2021-01-02 | 2021-01-01 | +| `EXCLUDE_END` | **2** | 0 | 2021-01-01 | 2020-12-31 | +| `EXCLUDE_ALL` | **3** | 0 | 2021-01-02 | 2020-12-31 | + +It also makes the class disagree with itself across precisions. `MONTH` and `YEAR` go through +the `iterator_count($this)` branch and already return the correct `0`, while `DAY`, `HOUR`, +`MINUTE` and `SECOND` return 2 / 2 / 3 for the equivalent period: + +``` +MONTH EXCLUDE_START length=0 iterator=0 <- correct +DAY EXCLUDE_START length=2 iterator=0 +HOUR EXCLUDE_START length=2 iterator=0 +MINUTE EXCLUDE_ALL length=3 iterator=0 +SECOND EXCLUDE_ALL length=3 iterator=0 +``` + +This is quiet rather than loud: `length()` returns a plausible positive integer, so callers +summing durations or checking `length() > 0` as an emptiness test get a wrong answer with +nothing to indicate it. + +## Root cause + +`src/PeriodTraits/PeriodGetters.php:91` + +```php +return $this->includedStart()->diff($this->includedEnd())->days + 1; +``` + +`DateInterval::$days` from `diff()` is **unsigned**. When excluding boundaries has pushed +`includedStart` after `includedEnd`, the period is empty, but `diff()->days` measures the gap +backwards as a positive number, and the unconditional `+ 1` then inflates it further. For +`EXCLUDE_ALL` on a one-day period the included range is 2021-01-02 .. 2020-12-31, so `days` +is 2 and the result is 3. + +The same defect sits at `src/PeriodTraits/PeriodGetters.php:76`, where `abs()` is applied +explicitly: + +```php +$length = abs($this->includedEnd()->getTimestamp() - $this->includedStart()->getTimestamp()); +``` + +which discards the sign of an inverted range for the `HOUR` / `MINUTE` / `SECOND` precisions. + +## Solution + +An empty period is the same condition in every branch, so it is handled once, up front: + +```php +if ($this->includedStart() > $this->includedEnd()) { + return 0; +} +``` + +This makes `DAY`, `HOUR`, `MINUTE` and `SECOND` agree with what `MONTH` and `YEAR` already +returned through `iterator_count()`, and with what iterating the period yields. + +With that guard in place the subtraction on line 76 can no longer be negative, so the `abs()` +there is now dead and is removed rather than left implying that a backwards range is +meaningful. + +Nothing else in `src/` calls `length()`, so the change is confined to the public API, and no +non-empty period changes value. + +## How this was tested + +Environment: PHP 8.3.6, Composer 2.10.2, on commit `16eae77` +("Bump actions/cache from 3 to 6 (#152)"). + +Two things were added to the test suite: + +- 14 rows appended to the existing `periods_with_amounts_of_included_dates` dataset + (`tests/Datasets/Dates.php`), covering one-unit and two-unit periods at `DAY`, `HOUR`, + `MINUTE` and `SECOND` precision, so they run through the existing + `it('length with boundaries')` test. +- `it('length is zero when excluding boundaries leaves nothing')` in `tests/BoundaryTest.php`, + backed by a new `periods_left_empty_by_excluded_boundaries` dataset. It asserts both + `iterator_count($period) === 0` and `$period->length() === 0`, across all six precisions, + which pins the two to each other rather than to a hand-written constant. + +**Counterfactual.** With `src/PeriodTraits/PeriodGetters.php` reverted and only the tests applied: + +``` +$ vendor/bin/pest +Tests: 16 failed, 182 passed +``` + +``` + x it length with boundaries with (0, Spatie\Period\Period Object (...)) #1 .. #9 + x it length is zero when excluding boundaries leaves nothing with data set "day" + x it length is zero when excluding boundaries leaves nothing with data set "day, end excluded" + x it length is zero when excluding boundaries leaves nothing with data set "day, all excluded" + x it length is zero when excluding boundaries leaves nothing with data set "two days, all excluded" + x it length is zero when excluding boundaries leaves nothing with data set "hour" + x it length is zero when excluding boundaries leaves nothing with data set "minute" + x it length is zero when excluding boundaries leaves nothing with data set "second" + + Failed asserting that 2 matches expected 0. + at tests/BoundaryTest.php:40 + 39, expect(iterator_count($period))->toEqual(0); + -> 40, expect($period->length())->toEqual(0); +``` + +Two details worth reading off that output. The `"month"` and `"year"` rows of the new test +**pass without the fix**, because those precisions already went through `iterator_count()` -- +that is the internal inconsistency described above. And the `iterator_count($period)` +assertion on line 39 passes throughout; only `length()` on line 40 fails, so the iterator was +right all along. + +With the fix applied: + +``` +$ composer install && vendor/bin/pest +Tests: 198 passed +Time: 0.91s +``` + +Baseline on a clean checkout of the same commit was `Tests: 175 passed`, so the delta is +exactly the 23 added cases, and the 175 pre-existing tests -- including the +`31 / 30 / 30 / 29` boundary-length expectations -- are untouched. + +DST was checked separately, since the guard compares two `DateTimeImmutable` instances. In +`Europe/Warsaw`, `length()` matches `iterator_count()` for every boundary combination on the +spring-forward day (2021-03-28), across it, and on the fall-back hour (2021-10-31 02:00): + +``` +DST day, DAY NONE: len=1 iter=1 START: len=0 iter=0 END: len=0 iter=0 ALL: len=0 iter=0 +across DST, DAY NONE: len=3 iter=3 START: len=2 iter=2 END: len=2 iter=2 ALL: len=1 iter=1 +DST hour, HOUR NONE: len=1 iter=1 START: len=0 iter=0 END: len=0 iter=0 ALL: len=0 iter=0 +fall-back hour, HOUR NONE: len=1 iter=1 START: len=0 iter=0 END: len=0 iter=0 ALL: len=0 iter=0 +``` + +Not verified locally: only PHP 8.3.6 was available, so the other PHP versions in the CI +matrix were not exercised, and `php-cs-fixer` was not run (it is not in `require-dev`; the +added code was written to the `@PSR12` ruleset in `.php-cs-fixer.php` by hand). diff --git a/src/PeriodTraits/PeriodGetters.php b/src/PeriodTraits/PeriodGetters.php index 7ec6a7e..4c3808f 100644 --- a/src/PeriodTraits/PeriodGetters.php +++ b/src/PeriodTraits/PeriodGetters.php @@ -66,6 +66,13 @@ trait PeriodGetters public function length(): int { + // Excluding the boundaries of a period that is only a few units long can push the + // included start past the included end. Such a period contains nothing, which is what + // iterating over it yields, so its length is 0 rather than a distance measured backwards. + if ($this->includedStart() > $this->includedEnd()) { + return 0; + } + // Length of month and year are not fixed, so we can't predict the length without iterate // TODO: maybe we can use cal_days_in_month ? if ($this->precision->equals(Precision::MONTH(), Precision::YEAR())) { @@ -73,7 +80,7 @@ trait PeriodGetters } if ($this->precision->equals(Precision::HOUR(), Precision::MINUTE(), Precision::SECOND())) { - $length = abs($this->includedEnd()->getTimestamp() - $this->includedStart()->getTimestamp()); + $length = $this->includedEnd()->getTimestamp() - $this->includedStart()->getTimestamp(); if ($this->precision->equals(Precision::SECOND())) { return $length + 1; diff --git a/tests/BoundaryTest.php b/tests/BoundaryTest.php index 2160dfd..d55889d 100644 --- a/tests/BoundaryTest.php +++ b/tests/BoundaryTest.php @@ -35,6 +35,11 @@ it('length with boundaries', function ($expectedAmount, Period $period) { expect($period->length())->toEqual($expectedAmount); })->with('periods_with_amounts_of_included_dates'); +it('length is zero when excluding boundaries leaves nothing', function (Period $period) { + expect(iterator_count($period))->toEqual(0); + expect($period->length())->toEqual(0); +})->with('periods_left_empty_by_excluded_boundaries'); + it('overlap with excluded boundaries', function () { $a = Period::make('2018-01-01', '2018-01-05', boundaries: Boundaries::EXCLUDE_END()); $b = Period::make('2018-01-05', '2018-01-10'); diff --git a/tests/Datasets/Dates.php b/tests/Datasets/Dates.php index ef9154a..ee7bd17 100644 --- a/tests/Datasets/Dates.php +++ b/tests/Datasets/Dates.php @@ -81,6 +81,38 @@ dataset('periods_with_amounts_of_included_dates', function () { yield [86362, Period::make('2018-01-01 00:00:00', '2018-01-01 23:59:22', Precision::SECOND(), Boundaries::EXCLUDE_START())]; yield [86362, Period::make('2018-01-01 00:00:00', '2018-01-01 23:59:22', Precision::SECOND(), Boundaries::EXCLUDE_END())]; yield [86361, Period::make('2018-01-01 00:00:00', '2018-01-01 23:59:22', Precision::SECOND(), Boundaries::EXCLUDE_ALL())]; + + // Excluding a boundary of a one unit long period leaves nothing behind. + yield [1, Period::make('2021-01-01', '2021-01-01', Precision::DAY(), Boundaries::EXCLUDE_NONE())]; + yield [0, Period::make('2021-01-01', '2021-01-01', Precision::DAY(), Boundaries::EXCLUDE_START())]; + yield [0, Period::make('2021-01-01', '2021-01-01', Precision::DAY(), Boundaries::EXCLUDE_END())]; + yield [0, Period::make('2021-01-01', '2021-01-01', Precision::DAY(), Boundaries::EXCLUDE_ALL())]; + + yield [1, Period::make('2021-01-01 00:00:00', '2021-01-01 00:00:00', Precision::HOUR(), Boundaries::EXCLUDE_NONE())]; + yield [0, Period::make('2021-01-01 00:00:00', '2021-01-01 00:00:00', Precision::HOUR(), Boundaries::EXCLUDE_START())]; + yield [0, Period::make('2021-01-01 00:00:00', '2021-01-01 00:00:00', Precision::HOUR(), Boundaries::EXCLUDE_END())]; + yield [0, Period::make('2021-01-01 00:00:00', '2021-01-01 00:00:00', Precision::HOUR(), Boundaries::EXCLUDE_ALL())]; + + yield [0, Period::make('2021-01-01 00:00:00', '2021-01-01 00:00:00', Precision::MINUTE(), Boundaries::EXCLUDE_ALL())]; + yield [0, Period::make('2021-01-01 00:00:00', '2021-01-01 00:00:00', Precision::SECOND(), Boundaries::EXCLUDE_ALL())]; + + // A two unit long period keeps exactly one unit when one of its boundaries is excluded. + yield [2, Period::make('2021-01-01', '2021-01-02', Precision::DAY(), Boundaries::EXCLUDE_NONE())]; + yield [1, Period::make('2021-01-01', '2021-01-02', Precision::DAY(), Boundaries::EXCLUDE_START())]; + yield [1, Period::make('2021-01-01', '2021-01-02', Precision::DAY(), Boundaries::EXCLUDE_END())]; + yield [0, Period::make('2021-01-01', '2021-01-02', Precision::DAY(), Boundaries::EXCLUDE_ALL())]; +}); + +dataset('periods_left_empty_by_excluded_boundaries', function () { + yield 'day' => [Period::make('2021-01-01', '2021-01-01', Precision::DAY(), Boundaries::EXCLUDE_START())]; + yield 'day, end excluded' => [Period::make('2021-01-01', '2021-01-01', Precision::DAY(), Boundaries::EXCLUDE_END())]; + yield 'day, all excluded' => [Period::make('2021-01-01', '2021-01-01', Precision::DAY(), Boundaries::EXCLUDE_ALL())]; + yield 'two days, all excluded' => [Period::make('2021-01-01', '2021-01-02', Precision::DAY(), Boundaries::EXCLUDE_ALL())]; + yield 'hour' => [Period::make('2021-01-01 00:00:00', '2021-01-01 00:00:00', Precision::HOUR(), Boundaries::EXCLUDE_ALL())]; + yield 'minute' => [Period::make('2021-01-01 00:00:00', '2021-01-01 00:00:00', Precision::MINUTE(), Boundaries::EXCLUDE_ALL())]; + yield 'second' => [Period::make('2021-01-01 00:00:00', '2021-01-01 00:00:00', Precision::SECOND(), Boundaries::EXCLUDE_ALL())]; + yield 'month' => [Period::make('2021-01-01', '2021-01-01', Precision::MONTH(), Boundaries::EXCLUDE_ALL())]; + yield 'year' => [Period::make('2021-01-01', '2021-01-01', Precision::YEAR(), Boundaries::EXCLUDE_ALL())]; }); dataset('ceiling_dates', function () {