Matchers & Filters: use the right ReflectionProperty

[Slamdunk]

Currently DeepCopy does not allow to copy private properties on the parent class when Matchers and Filters are used, as I show in this new test: https://github.com/myclabs/DeepCopy/pull/35/files#diff-0fe594d8935dca0233edc25909b8267dR59

This is because the ReflectionHelper gathers all the properties from parent classes but the Matchers and the Filters try to instantiate their own ReflectionProperty, which raise an ReflectionException because private properties on parent class are not part of sub classes: https://3v4l.org/MuZPG

There are two solutions:

1. Use in every matcher and in every filter a new helper to find the property through the parent classes, which is backward compatible, but you have to test that everyone uses this "helper"
2. Change the Matcher and Filter interface to match the ReflectionProperty gathered be the ReflectionHelper, which is a BC break if someone is using a custom implementation of this two interfaces, but is way more solid

For this PR I chose the latter.

The current test on this subject, testPrivatePropertyOfParentObjectCopy, passes because no matchers and no filters are used.

[mnapoli]

Hi, thank you for this very great analysis and PR!

I would definitely prefer option 2 as well, but unfortunately we have to keep BC :( PHPUnit is using this library, so I definitely don't want to break BC if it can be avoided. It seems like option 1 is the only choice, do you have time to implement it?

[Slamdunk]

Well, considering that PHPUnit bounded the DeepCopy version
https://github.com/sebastianbergmann/phpunit/blob/5.1.6/composer.json#L41
we can follow the http://semver.org/ and tag a new 2.0.0 version.

I can PR a new CHANGELOG.md in the way PHPUnit does: http://keepachangelog.com/

You have to feel free to change API you if follow widespread rules and document it

[mnapoli]

I don't want to have to maintain 2 versions of this library that has already a maintenance cost way to high compared to what it does. Releasing a new major version just for this small fix is not worth it IMO. A v2 would deserve some refactoring to simplify a lot the API.

What we can do in the meantime is create a trait or base class that will add a helper to get the correct ReflectionProperty instance?

[Slamdunk]

Independently from this PR, you should decide and declare what and how you intend to mantain.
If you don't want to mantain 2 versions you either choose not to have anytime in the future a v2, or declare that due to the purpose and cost, this package will always have only one mantained version, the latest one.

Beside that, I don't understand the statement "A v2 would deserve [...]", a version change is what you decide it has to be.
If you think that a v2 will be a major release and it deserves a big refactoring, users will be less likely to adopt it, like what happened to ZF1 and ZF2.
If you choose to have more major releases with small changes well documented, it will be a lot easier for everyone to understand, get and use the changes.

What I suggest this project should do is:

1. Declare that only one version will be mantained, the latest one (due to lack of resources)
2. Declare that this project will follow http://keepachangelog.com/ directives to let users know for API changes
3. Declare that this project will follow http://semver.org/ for versioning, so no BC breaks will be introducted for packagies using this dependancy declared on specific major versions

Even with low resources and time available, changes shouldn't be feared because of others usages.
I can help on this topic pushing appropriate PR if you ask.

What do you and other maintainers think?

[mnapoli]

This packages already has a changelog and follows semver.

My point is that for users, upgrading a major version takes time. Even if there are not many BC breaks. Creating multiple new major versions is not a good thing, what happens with Guzzle is a good example of that.

If you choose to have more major releases with small changes well documented, it will be a lot easier for everyone to understand, get and use the changes.

Keep in mind this is a small tool, not a framework. Users of this package don't look out for new features or versions, if they used the package it means it did the job for them. I don't think users would appreciate having to read changelogs and go over their old code just to be on a correctly maintained major version. It's much easier for them upgrading minors.

So again, I am not against a new major version. But it has to be worth it. This PR can be solved easily without breaking BC, so it's not worth creating a new major version.

"A v2 would deserve [...]", a version change is what you decide it has to be.

Yes, that's what I meant. I want v2 to be much simpler (big internal refactoring). I expect 80% of users not to be affected by this refactoring though (just like PHPUnit probably won't be). As it is now this packages has too many hacks and is too complex (internally and for users too).

[Slamdunk]

I observe and respect your point.

Ping me again when you are ready for API changes, best regards.

[mnapoli]

I'm sorry maybe I didn't express correctly what I was trying to say: I do think what you suggested as solution 1 is a good solution (coupled to a trait that would be an easy change I guess). And I do appreciate the time you are putting into this discussion.

Are you opposed a solution that favors keeping BC?

[Slamdunk]

I am sorry if you intended my last comment as a rage quit, as it wasn't.

The solution 1 would be really easy: just add a new method like ReflectionHelper::getProperty($object, $propertyName) that gathers throw parents the correct property, and let Filters and Matchers use it.

But I do really believe that a simple and small project like this must/needs to act with a rigid policy of small deployments, even for API changes, to ease the workload of maintainers and the lightweight of the project itself.

When I use a dependancy, I accept the fact that I will have to watch it: small complexity and small changes are easier to follow and will lead to less issues, even though they may seem a burden for the watching time cost.

[Slamdunk]

@mnapoli may you add this PR to 2.0 milestone so we have a reminder for the future please?

Even if you are not going to tag a v2 (for now), better to have it hanging forever than hidden.

[ivoba]

Hello, i just stumbled into this issue as well with a class that extends a Doctrine ArrayCollection.

Im not sure on what you agreed here, but I would welcome a soon BC keeping fix :)

[Slamdunk]

Hi @theofidry may I ask you why #88 was favored instead of this PR?

With #88 you gather the property in DeepCopy, then pass the property name, and then re-gather the property again.
With this PR use just use the already gathered property.

[theofidry]

@Slamdunk apologies this was not intended. Seems like it has been closed when switching the default branch

[Slamdunk]

When you are ready with the 2.0 branch, ping me and I'll rebase this PR

[Slamdunk]

Hi, may I ask why this PR was closed?
As far as I can see the issue is still in place.

[theofidry]

Another switch of branch. I've deleted master in favour of 1.x and 2.x and since this PR was targeting master it closed the PR. Could you open it again against 2.x?

[Slamdunk]

Sure: please reopen the PR and mark it as awaiting- author-updates, so I know the ball is up to me and I have to do something

[theofidry]

I cannot re-open the PR, the button is disabled: "The master branch has been deleted". (I would happily do so and re-target the branch otherwise)