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.

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