fix: keep intervals built by createFromDateString() when deep copying
Since PHP 8.2 a DateInterval created by DateInterval::createFromDateString() exposes only "from_string" and "date_string" instead of the y/m/d/h/i/s fields, and "from_string" is ignored when assigned to. Rebuilding such an interval by copying its visible properties onto a new DateInterval therefore produced a zero interval, so a copied interval silently stopped shifting dates. Read the properties once and rebuild a date-string interval from the string it was created from, leaving the existing property copy for every other interval. Cloning would also fix it but would invoke __clone() on userland DateInterval subclasses, which #76 explicitly rules out.
This commit is contained in:
dualfroz committed
1 parent
e23bc8d851
commit
0ca75d87eb
3 files changed
+241
-1
No files matched your search
@@ -0,0 +1,187 @@
|
||||
# Fix DateIntervalFilter silently zeroing intervals made by createFromDateString()
|
||||
|
||||
Fixes #188.
|
||||
|
||||
## Problem
|
||||
|
||||
On PHP 8.2+, deep copying a `DateInterval` built by `DateInterval::createFromDateString()`
|
||||
silently returns a **zero** interval. No exception, no warning -- the copy simply stops
|
||||
moving dates.
|
||||
|
||||
```php
|
||||
$interval = DateInterval::createFromDateString('2 days');
|
||||
$copy = (new DeepCopy())->copy($interval);
|
||||
|
||||
$base = new DateTimeImmutable('2026-01-01');
|
||||
echo $base->add($interval)->format('Y-m-d'); // 2026-01-03
|
||||
echo $base->add($copy)->format('Y-m-d'); // 2026-01-01 <-- the interval is gone
|
||||
```
|
||||
|
||||
## Root cause
|
||||
|
||||
`src/DeepCopy/TypeFilter/Date/DateIntervalFilter.php:25`
|
||||
|
||||
```php
|
||||
$copy = new DateInterval('P0D');
|
||||
|
||||
foreach ($element as $propertyName => $propertyValue) {
|
||||
$copy->{$propertyName} = $propertyValue;
|
||||
}
|
||||
```
|
||||
|
||||
The filter rebuilds the interval by copying the visible properties onto a fresh
|
||||
`new DateInterval('P0D')`. Since PHP 8.2 that does not work for date-string intervals, for
|
||||
two compounding reasons:
|
||||
|
||||
1. An interval created by `createFromDateString()` **does not expose the numeric fields at
|
||||
all**. `get_object_vars()` on it returns exactly two entries:
|
||||
|
||||
```
|
||||
['from_string' => true, 'date_string' => '2 days']
|
||||
```
|
||||
|
||||
whereas a spec-created interval returns `y, m, d, h, i, s, f, invert, days, from_string`.
|
||||
So there is no `d => 2` for the loop to copy; the duration lives only in `date_string`.
|
||||
|
||||
2. **`from_string` is ignored when assigned to.** The loop does run
|
||||
`$copy->from_string = true`, but the value does not stick:
|
||||
|
||||
```php
|
||||
$copy = new DateInterval('P0D');
|
||||
foreach (DateInterval::createFromDateString('2 days') as $k => $v) {
|
||||
$copy->{$k} = $v;
|
||||
}
|
||||
var_dump(get_object_vars($copy)['from_string']); // bool(false)
|
||||
```
|
||||
|
||||
The result is a hybrid object: `from_string` stayed `false`, so the engine treats it as an
|
||||
ordinary interval with all-zero fields, while `date_string` sits on it as a dynamic property
|
||||
that is never re-interpreted. Hence a zero interval.
|
||||
|
||||
(Worth noting for reviewers: `from_string` and `date_string` are **not** readable through
|
||||
`$interval->from_string`. They only surface through iteration / `get_object_vars()`, which is
|
||||
why the check below reads them from the property array rather than directly.)
|
||||
|
||||
## Solution
|
||||
|
||||
Read the properties once, and when the interval is a date-string one, rebuild it from the
|
||||
string it was created from instead of copying fields that do not exist:
|
||||
|
||||
```php
|
||||
$properties = get_object_vars($element);
|
||||
|
||||
if (!empty($properties['from_string'])) {
|
||||
return DateInterval::createFromDateString($properties['date_string']);
|
||||
}
|
||||
|
||||
$copy = new DateInterval('P0D');
|
||||
|
||||
foreach ($properties as $propertyName => $propertyValue) {
|
||||
$copy->{$propertyName} = $propertyValue;
|
||||
}
|
||||
|
||||
return $copy;
|
||||
```
|
||||
|
||||
The guard is safe across supported versions: on a spec-created interval `from_string` is
|
||||
`false`, and on PHP 8.0/8.1 the key is absent entirely -- both fall through to the original
|
||||
path, which is unchanged.
|
||||
|
||||
### Why not just `clone $element`
|
||||
|
||||
That was my first attempt, and it is worth recording why it is wrong, because it looks like
|
||||
the obvious fix given the class is marked
|
||||
`@deprecated Will be removed in 2.0. This filter will no longer be necessary in PHP 7.1+.`
|
||||
|
||||
`clone` does produce a correct copy for every interval shape I tested. But it breaks
|
||||
`DeepCopyTest::test_it_skips_the_copy_for_userland_dateinterval` (the test guarding #76),
|
||||
which asserts that deep copying a userland `DateInterval` subclass must **not** invoke its
|
||||
`__clone()`:
|
||||
|
||||
```
|
||||
1) DeepCopyTest\DeepCopyTest::test_it_skips_the_copy_for_userland_dateinterval
|
||||
Failed asserting that true is false.
|
||||
```
|
||||
|
||||
The approach in this PR keeps that contract intact: it never clones, and the rebuild branch
|
||||
can only ever be reached by a plain `DateInterval`, since `createFromDateString()` is not
|
||||
late-static-bound (`FooDateInterval::createFromDateString('2 days')` returns a plain
|
||||
`DateInterval` on PHP 8.3).
|
||||
|
||||
### On the `@deprecated` marker
|
||||
|
||||
The class says the filter "will no longer be necessary in PHP 7.1+", so an alternative
|
||||
resolution would be to drop the filter rather than fix it, and I want to flag that in case
|
||||
you would prefer it. Two data points against dropping it right now:
|
||||
|
||||
- With the type filter removed, `DeepCopy::copyObject()` falls back to `clone $object`
|
||||
(`src/DeepCopy/DeepCopy.php:207`). That would invoke userland `__clone()` and regress #76,
|
||||
exactly as above.
|
||||
- Removing the class is a BC break for anyone constructing `DateIntervalFilter` directly.
|
||||
|
||||
So this PR fixes the filter in place and leaves the deprecation decision for 2.0. Happy to
|
||||
switch to removal instead if that is the direction you want.
|
||||
|
||||
The issue text also states "Removing the `DateIntervalFilter` doesn't work." I could not
|
||||
reproduce that part on PHP 8.3.6 -- with the filter bypassed, all seven interval shapes I
|
||||
tested copied correctly. The `__clone()` contract above is the real blocker, not correctness.
|
||||
|
||||
## How this was tested
|
||||
|
||||
Environment: PHP 8.3.6, Composer 2.10.2, on commit `958a080`
|
||||
("Merge pull request #212 from myclabs/add-types").
|
||||
|
||||
Two tests were added to `tests/DeepCopyTest/TypeFilter/Date/DateIntervalFilterTest.php`,
|
||||
following the existing `test_it_*` naming there:
|
||||
|
||||
- `test_it_deep_copies_a_DateInterval_created_from_a_date_string` -- the regression
|
||||
- `test_it_deep_copies_an_inverted_DateInterval_with_microseconds` -- guards the
|
||||
property-copy branch (`invert`, `f`) that the new branch must not disturb
|
||||
|
||||
**Counterfactual.** With `src/DeepCopy/TypeFilter/Date/DateIntervalFilter.php` reverted and
|
||||
only the new tests applied:
|
||||
|
||||
```
|
||||
$ vendor/bin/phpunit --filter 'DateIntervalFilterTest'
|
||||
1) DeepCopyTest\TypeFilter\Date\DateIntervalFilterTest::test_it_deep_copies_a_DateInterval_created_from_a_date_string
|
||||
Failed asserting that two strings are identical.
|
||||
--- Expected
|
||||
+++ Actual
|
||||
@@ @@
|
||||
-'2026-01-03 00:00:00'
|
||||
+'2026-01-01 00:00:00'
|
||||
|
||||
FAILURES!
|
||||
Tests: 3, Assertions: 6, Failures: 1.
|
||||
```
|
||||
|
||||
With the fix applied:
|
||||
|
||||
```
|
||||
$ vendor/bin/phpunit --filter 'DateIntervalFilterTest'
|
||||
OK (3 tests, 8 assertions)
|
||||
```
|
||||
|
||||
In full, before the change:
|
||||
|
||||
```
|
||||
$ composer install && vendor/bin/phpunit
|
||||
Tests: 84, Assertions: 170, Warnings: 1.
|
||||
```
|
||||
|
||||
after the change:
|
||||
|
||||
```
|
||||
$ vendor/bin/phpunit
|
||||
Tests: 86, Assertions: 176, Warnings: 1.
|
||||
```
|
||||
|
||||
The single warning is pre-existing on a clean checkout and unrelated -- it is
|
||||
`ArrayObjectFilterTest::test_it_deep_copies_an_array_object` reporting that
|
||||
`TestCase::prophesize()` is deprecated. The delta is exactly the 2 added tests
|
||||
and their 6 assertions. There are no failures in either run.
|
||||
|
||||
Not verified locally: only PHP 8.3.6 was available, so the PHP 8.0 / 8.1 branch of the
|
||||
`!empty($properties['from_string'])` guard (where the key is absent) was reasoned about from
|
||||
the property layout rather than executed, and the PHP 8.4/8.5 rows of the matrix were not
|
||||
run either. CI covers those.
|
||||
@@ -22,9 +22,20 @@ class DateIntervalFilter implements TypeFilter
|
||||
*/
|
||||
public function apply(mixed $element)
|
||||
{
|
||||
$properties = get_object_vars($element);
|
||||
|
||||
// Since PHP 8.2 an interval built by DateInterval::createFromDateString() exposes
|
||||
// "from_string" and "date_string" instead of the y/m/d/h/i/s properties, and
|
||||
// "from_string" is ignored when it is assigned to. Copying the properties over one
|
||||
// by one would therefore turn such an interval into a zero interval, so it has to be
|
||||
// rebuilt from the date string it was created from.
|
||||
if (!empty($properties['from_string'])) {
|
||||
return DateInterval::createFromDateString($properties['date_string']);
|
||||
}
|
||||
|
||||
$copy = new DateInterval('P0D');
|
||||
|
||||
foreach ($element as $propertyName => $propertyValue) {
|
||||
foreach ($properties as $propertyName => $propertyValue) {
|
||||
$copy->{$propertyName} = $propertyValue;
|
||||
}
|
||||
|
||||
|
||||
@@ -3,6 +3,7 @@
|
||||
namespace DeepCopyTest\TypeFilter\Date;
|
||||
|
||||
use DateInterval;
|
||||
use DateTimeImmutable;
|
||||
use DeepCopy\TypeFilter\Date\DateIntervalFilter;
|
||||
use PHPUnit\Framework\TestCase;
|
||||
|
||||
@@ -22,4 +23,45 @@ class DateIntervalFilterTest extends TestCase
|
||||
$this->assertEquals($object, $copy);
|
||||
$this->assertNotSame($object, $copy);
|
||||
}
|
||||
|
||||
public function test_it_deep_copies_a_DateInterval_created_from_a_date_string()
|
||||
{
|
||||
$object = DateInterval::createFromDateString('2 days');
|
||||
|
||||
$filter = new DateIntervalFilter();
|
||||
|
||||
$copy = $filter->apply($object);
|
||||
|
||||
// The copy has to shift a date by the same amount as the original.
|
||||
$base = new DateTimeImmutable('2026-01-01 00:00:00');
|
||||
|
||||
$this->assertSame(
|
||||
$base->add($object)->format('Y-m-d H:i:s'),
|
||||
$base->add($copy)->format('Y-m-d H:i:s')
|
||||
);
|
||||
|
||||
$this->assertEquals($object, $copy);
|
||||
$this->assertNotSame($object, $copy);
|
||||
}
|
||||
|
||||
public function test_it_deep_copies_an_inverted_DateInterval_with_microseconds()
|
||||
{
|
||||
$object = new DateInterval('PT1S');
|
||||
$object->invert = 1;
|
||||
$object->f = 0.25;
|
||||
|
||||
$filter = new DateIntervalFilter();
|
||||
|
||||
$copy = $filter->apply($object);
|
||||
|
||||
$this->assertEquals($object, $copy);
|
||||
$this->assertNotSame($object, $copy);
|
||||
|
||||
$base = new DateTimeImmutable('2026-01-01 00:00:00');
|
||||
|
||||
$this->assertSame(
|
||||
$base->add($object)->format('Y-m-d H:i:s.u'),
|
||||
$base->add($copy)->format('Y-m-d H:i:s.u')
|
||||
);
|
||||
}
|
||||
}
|
||||
Reference in new issue
Block a user