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

Use array_diff_key to get diff for new or deleted documents #2388

Merged
merged 5 commits into from
Nov 12, 2021

Conversation

pepeh
Copy link
Contributor

@pepeh pepeh commented Nov 5, 2021

Q A
Type bug
BC Break no
Fixed issues fixes #2387

Summary

Avoid of array_udiff usage for getDeletedDocuments and getInsertedDocuments functions

@IonBazan IonBazan added the Bug label Nov 5, 2021
@IonBazan IonBazan linked an issue Nov 5, 2021 that may be closed by this pull request
@IonBazan IonBazan added this to the 2.2.3 milestone Nov 5, 2021
@IonBazan IonBazan changed the title Use array_diff_key to get diff for new or deleted documents (#2387) Use array_diff_key to get diff for new or deleted documents Nov 5, 2021
@IonBazan
Copy link
Member

IonBazan commented Nov 5, 2021

@pepeh thanks for reporting and solving the problem. Can you add some tests to verify the new behaviour? Please also run vendor/bin/phpcbf to fix code style.

@pepeh
Copy link
Contributor Author

pepeh commented Nov 5, 2021

@IonBazan Thank you. I have fixed code style.

We already have test cases
testGetDeletedDocuments
testGetInsertedDocuments
and i'm not sure what can I add more.

Copy link
Member

@malarzm malarzm left a comment

Choose a reason for hiding this comment

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

IIRC the is_object check could come from some ooold times when PersistentCollection was meant to be used with other collection thingies. As it's only used for holding objects I think it's safe to drop it (but I'm still in vacation mode so I might be wrong 👼)

All in all, the change looks good to me 👍

@IonBazan IonBazan merged commit 5259b02 into doctrine:2.2.x Nov 12, 2021
@IonBazan
Copy link
Member

Thank you @pepeh! ❤️

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.

Doctrine deletes embedded documents
5 participants