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.
This commit is contained in:
dualfroz committed
1 parent
79eb344eb0
commit
5763d58d02
4 files changed
+208
-1
No files matched your search
@@ -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).
|
||||
Reference in new issue
Block a user