Fixed filters & matchers to allow copy private properties of ancestor objects

[vtsykun]

Fix issue #62

Q	A
Branch?	master
Bug fix?	yes
New feature?	no
BC Breaks?	no
Deprecations?	no
Fix issue ?	#62
License	MIT
Doc	-

[fsmeier]

+1
this is exact what i need and atm i added nearly every filter by myself to add such private-property-access-function. This should be in the lastest version asap, otherwise you can not work with doctrine-proxies for example

[theofidry]

Unless I didn't understand what's going on properly, you're not supposed to access to the private properties of the parent entity

[vtsykun]

I agree with you, that I'm not supposed to access to the private properties of the parent entity. But this possibility for copying private properties of ancestor objects was added here. Currently the ReflectionHelper:getProperties() return all the properties from parent classes (include private from parent classes), but at the same time the Filters & Matchers is not support it, so in this PR I fixed BC break that was introduced after merge #23, keeping backward compatibility.

[fsmeier]

@theofidry if you want to provide sth like a strict mode without parent-privates we can add an option. if the option is set to strict(or other way around), ReflectionHelper:getProperties() should not return privates from parent anymore and the current proposed solution should also check this option then. So you can have a strict mode but allow the possibility of accessing them.

the current version break and do not work properly like @vtsykun mentioned.

[theofidry]

@trebi could you provide a use case for this parent private property? I would like to have one to make my opinion on this

[fsmeier]

@theofidry i am not @trebi but I can give you one use-case:
If you work with entities handled by doctrine it is nearly impossible to get all the time the pure entity back. So the ProxyEntity is a Child of the actual entity. But the 'real' properties from the actual entity are most of the time private.
If we would implement what is proposed in issue#67 we could forward the 'power' to the matcher i.e.: Match just if the property is from the child-class or other way around.

[trebi]

@theofidry On one hand, accessing parent object clearly is violation of OOP encapsulation. But not supporting it during deep copy could result in violation of Liskov substitution principle, which is even worse (if your code is deigned to work with class Y and you subtitute it with class Z that inherits from Y, which is perfectly valid, it will suddenly break).

IMHO, it also contraditcs the intuition to use DeepCopy library in the first place, as it will not create completely isolated copy of the graph of objects, as shown on this image.

My use-case was that I had graph of various objects linked together and I wanted to create a copy of this graph (while preserving inner references), try out some changes and then decise whether or not to "commit" these changes (replace the original graph of objects with the new one). Before I implemented #23, it was not creating the isolated copy of object graph and therefore it was useless for me.

// this is how it worked before #23 was accepted
class X {}

class Y {
    private $x;
    public function __construct(X $x) { $this->x = $x; }
    public function getX() { return $this->x; }
}

class Z extends Y {}

$deepCopy = new DeepCopy;
$x = new X;

$y = new Y($x);
$yCopy = $deepCopy->copy($y);
var_dump($y->getX());
var_dump($yCopy->getX()); // X is copied as expected 

$z = new Z($x);
$zCopy = $deepCopy->copy($z);
var_dump($z->getX());
var_dump($zCopy->getX()); // X is not copied - violates Liskov substitution principle

[theofidry]

Thanks @trebi, a use case was all I needed :)

[fsmeier]

And what arethe next steps here?

[mnapoli]

@theofidry is it good to you for merging? That looks good to me (except the extra interface but that's easily fixed) but given how widely the project is used I'd really appreciate extra eyes ;)

[mnapoli]

This interface was added but I don't think it's related to this pull request, could you remove it please?

[vtsykun]

Done.

[theofidry]

All good for me 👍

[fsmeier]

Any update here?

[mjpvandenberg]

I'd love to see this pull request merged.

[theofidry]

Sorry, just need to merge this and tag it, might do over the weekend

[vtsykun]

ping @theofidry

[theofidry]

Thank you guys, sorry for the delay