Skip to content

Remove usage of array_merge#51

Merged
mnapoli merged 1 commit intomyclabs:masterfrom
theofidry:bugfix/reflection-helper
Sep 22, 2016
Merged

Remove usage of array_merge#51
mnapoli merged 1 commit intomyclabs:masterfrom
theofidry:bugfix/reflection-helper

Conversation

@theofidry
Copy link
Collaborator

Follow up of #45.

$propsArr = array_merge($parentPropsArr, $propsArr);
foreach ($propsArr as $key => $property) {
$parentPropsArr[$key] = $property;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The behaviour is different: please add a test where the parent class and the child class has a private variable with the same name, and the test verifies that the ReflectionProperty is the one of the child instead the one of the parent

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, the test only assert that $a3 is in the properties list returned by the helper.
What you have to test, and then to ensure, is that the ReflectionProperty returned refers to the $a3 of ReflectionHelperTestChild instead of ReflectionHelperTestParent one.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

I would suggest to put the test in the scope they are addressing, so into tests/DeepCopyTest/Reflection/ReflectionHelperTest.php. @mnapoli what do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's fine as it is, it's implemented as a functional test so it tests a behavior, not ReflectionHelperTest directly.

@theofidry theofidry force-pushed the bugfix/reflection-helper branch from 466027a to 3f763a4 Compare September 22, 2016 13:02
@mnapoli mnapoli merged commit cd26048 into myclabs:master Sep 22, 2016
@theofidry theofidry deleted the bugfix/reflection-helper branch September 22, 2016 21:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants