Copy property after applying doctrine filter #133
Conversation
|
Hi, thanks for the PR! Could you please add a test for it as well to ensure this is fixing the issue and preventing any regression in the future? I'll try to have a look at it this week/weekend |
|
@theofidry hi! Thank you for quick reply! Just added test for applying DoctrineProxyFilter only. Hope it helps to understand my case somehow. |
|
@theofidry , looking forward to your reply. |
src/DeepCopy/DeepCopy.php
Outdated
| @@ -195,6 +196,10 @@ function ($object) { | |||
| } | |||
| ); | |||
|
|
|||
| if ($filter instanceof DoctrineProxyFilter) { | |||
There was a problem hiding this comment.
FYI I added the ChainableFilter interface (#101) to avoid using directly the DoctrineProxyFilter filter.
With that in mind, the two conditions seems redundant (and now, the second condition is now uncovered in tests, see https://coveralls.io/builds/22507096/source?filename=src%2FDeepCopy%2FDeepCopy.php#L206)
I guess that the proper fix would be to remove the $filterWasApplied variable as I added it to avoid a possible BC break back then (but I guess there was still this bug ^^).
There was a problem hiding this comment.
$filterWasApplied is used to prevent recursive copy of property, if filter was applied. In other words you use filter or recursive copy. Removing $filterWasApplied breaks some fundamentals of DeepCopy.
There was a problem hiding this comment.
$filterWasApplied is only use when you use the DoctrineProxyFilter (=ChainableFilter) without any other filter (your use case).
I added this variable to keep the same behaviour (possible BC break) as before while fixing #98.
The issue before was that when you use the DoctrineProxyFilter with an other filter (SetNullFilter on the ID for example), the second filter was never used (so no ID with null value).
So, I added a condition to allow ChainableFilter (=DoctrineProxyFilter) to apply an other filter after (with the continue).
You can reach the return in the if ($filterWasApplied) { condition only if there is no other filter to apply. There is already a return statement when you apply a filter.
There was a problem hiding this comment.
Got it now. You say, instead of adding "continue" on DoctrineProxyFilter, we need to remove $filterWasApplied variable. So that when chainable filter applied, either recursive copy or other filter applies after. Right?
There was a problem hiding this comment.
Yep 👍 (sorry if it wasn't clear the first time, english is not my native language :D)
fixtures/f010/A.php
Outdated
| { | ||
| $this->foo = $foo; | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing EOF(end of file).
fixtures/f010/B.php
Outdated
| { | ||
| $this->foo = null; | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing EOF(end of file).
|
Back from holidays. Thanks for the PR! Would you mind rebasing it? |
|
@theofidry done! |
theofidry
changed the title
When DoctrineProxyFilter applied and there is no other filters for property, deep copy doesn't create recursive copy of property.