Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix deep copying of ArrayObject in PHP 7.4 #145

Merged
merged 1 commit into from
Jan 17, 2020

Conversation

dontub
Copy link
Contributor

@dontub dontub commented Dec 17, 2019

In PHP 7.4 the storage of an ArrayObject isn't returned as
ReflectionProperty.

In PHP 7.4 the storage of an ArrayObject isn't returned as
ReflectionProperty.
@dontub dontub force-pushed the fix-copying-array-object-php74 branch from dd12c91 to 8dcad4e Compare December 17, 2019 11:18
@@ -385,6 +387,18 @@ public function test_it_uses_the_first_filter_matching_for_copying_object_proper
$this->assertNull($copy->getAProp()->cloned);
}

public function test_it_can_deep_copy_an_array_object()
Copy link
Member

Choose a reason for hiding this comment

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

Just to be sure: this test fails on PHP 7.4 if the new ArrayObjectFilter is not registered?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, without ArrayObjectFilter $foo and $copy['foo'] would be same.

@mnapoli mnapoli added the bug label Dec 17, 2019
@mnapoli
Copy link
Member

mnapoli commented Dec 17, 2019

LGTM, just waiting on the confirmation.

@@ -59,6 +61,7 @@ public function __construct($useCloneMethod = false)
{
$this->useCloneMethod = $useCloneMethod;

$this->addTypeFilter(new ArrayObjectFilter($this), new TypeMatcher(ArrayObject::class));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it make sense to register it only on PHP 7.4+?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't see any reason for not using it in PHP <7.4, do you?

@dontub
Copy link
Contributor Author

dontub commented Jan 16, 2020

@mnapoli @theofidry Can this be merged, or is there anything left to do?

@theofidry theofidry merged commit b2c2878 into myclabs:1.x Jan 17, 2020
@theofidry
Copy link
Collaborator

Thank you @dontub!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants