myclabs-DeepCopy/PR.md

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.

$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

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

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

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