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 embedded document parent before computing changeset #1542

Merged
merged 1 commit into from
Jan 2, 2017

Conversation

alcaeus
Copy link
Member

@alcaeus alcaeus commented Dec 25, 2016

Fixes #1525.

The problem is that the changeset wrote down the old embedded document, which was then used when trying to persist changes. I don't like adding another check for the embedded document parent, maybe @malarzm has a better idea?

@alcaeus alcaeus changed the title Fix embedded dociument parent before computing changeset Fix embedded document parent before computing changeset Dec 25, 2016
@alcaeus alcaeus added the Bug label Dec 25, 2016
@alcaeus alcaeus added this to the 1.1.3 milestone Dec 25, 2016
@malarzm
Copy link
Member

malarzm commented Dec 27, 2016

The only thing that comes to my mind is removing the check in persist part of UoW and seeing if the new one will be sufficient for both cases (I think there was a test checking if the embedded document is changed as soon as persist is called but I think we could break that as we were mostly interested in having embedded document cloned eventually during flush, not in specific moment in time)

@alcaeus
Copy link
Member Author

alcaeus commented Dec 29, 2016

@malarzm tried that, with the following result:

  1. Doctrine\ODM\MongoDB\Tests\Functional\EmbeddedTest::testReusedEmbeddedDocumentsAreClonedInFact
    Failed asserting that two variables don't reference the same object.

tests/Doctrine/ODM/MongoDB/Tests/Functional/EmbeddedTest.php:580

@malarzm
Copy link
Member

malarzm commented Dec 29, 2016

That exact assertion could be removed as it was checking if embedded documents were cloned as soon as persist :)

@alcaeus
Copy link
Member Author

alcaeus commented Jan 2, 2017

@malarzm I removed the failing assertion, everything else works as expected.

@malarzm
Copy link
Member

malarzm commented Jan 2, 2017

Cool!

@malarzm malarzm merged commit 1a520b5 into doctrine:1.1.x Jan 2, 2017
@alcaeus alcaeus deleted the fix-embedded-parent-again branch January 10, 2017 06:18
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.

2 participants