# 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).