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:

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