diff --git a/PR.md b/PR.md new file mode 100644 index 0000000..29511a7 --- /dev/null +++ b/PR.md @@ -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. diff --git a/src/DeepCopy/TypeFilter/Date/DateIntervalFilter.php b/src/DeepCopy/TypeFilter/Date/DateIntervalFilter.php index 62aaea5..af166c5 100644 --- a/src/DeepCopy/TypeFilter/Date/DateIntervalFilter.php +++ b/src/DeepCopy/TypeFilter/Date/DateIntervalFilter.php @@ -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; } diff --git a/tests/DeepCopyTest/TypeFilter/Date/DateIntervalFilterTest.php b/tests/DeepCopyTest/TypeFilter/Date/DateIntervalFilterTest.php index 9552f56..6f95607 100644 --- a/tests/DeepCopyTest/TypeFilter/Date/DateIntervalFilterTest.php +++ b/tests/DeepCopyTest/TypeFilter/Date/DateIntervalFilterTest.php @@ -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') + ); + } }