spatie-period/PR.md

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:

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:

$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

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:

$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:

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