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

Change persistence ordering for OneToOne to allow making JoinColumns non-nullable #8703

Closed
wants to merge 2 commits into from

Conversation

rvanlaak
Copy link
Contributor

@rvanlaak rvanlaak commented May 20, 2021

closes #7006
closes #6499
closes #5538
closes #4230

The issue as discussed earlier in #7006, #6499, #5538 and #4230 makes it harder to encapsulate behavior inside domain objects, when following some basic principles of DDD aggregate roots. In other words, the aggregate should not leak it's child nor have a dependency on a em/repo for the child to get persisted.

To be clear on the workaround, setting the @JoinColumn to nullable solves this, as Doctrine does take care of setting the id on the object at a later moment.

In response to discussing a fix for #6499 we borrowed the test as @Frikkle proposed in #6533 and slightly adjusted it to demonstrate the issue on persisting cascading relationships.

Expected

The exception we should get on running the test, that would confirm that the persistence ordering is wrong:

Exception : [Doctrine\ORM\ORMException] The identifier id is missing for a query of Doctrine\Tests\ORM\Functional\Ticket\DDC6499A

Actual

There is pending discussion in #6533 on a proposed fix.

As the tests in this PR pass, that proves that the commit ordering works correctly.

@rvanlaak rvanlaak force-pushed the bugfix/ddc6499 branch 3 times, most recently from 5cba2a7 to cca8b89 Compare May 26, 2021 13:43
…bug on non-nullable cascading relationship
@rvanlaak
Copy link
Contributor Author

rvanlaak commented Jul 1, 2021

@greg0ire I've fixed failing cs tests of commit 251e1a6 in 9a3bc3c , can you reapprove running the workflows?

@greg0ire
Copy link
Member

greg0ire commented Jul 1, 2021

I'm confused: how can a PR that changes only the tests directory fix any bug?

@rvanlaak
Copy link
Contributor Author

rvanlaak commented Jul 1, 2021

The point would be that the tests in this PR should prove that the long-standing issues related to persistence ordering have been fixed by other changes/commits in the meanwhile, but that is something we should verify with a code review.

I've also locally cherry-picked these tests on the 2.6 branch, and tests also pass over there.

@mpdude
Copy link
Contributor

mpdude commented Feb 20, 2023

@rvanlaak do I understand correctly that this PR provides test cases to show other issues being fixed?

If yes, it should not be too difficult to get these tests merged, and tests are always a good thing.

Could you rebase this to 2.14.x?

@rvanlaak
Copy link
Contributor Author

@mpdude yes it should. I see you also asked the other ticket creators (the ones mentioned on this PR description) to verify the situation, so let's also wait for their answer.

@mpdude
Copy link
Contributor

mpdude commented Feb 27, 2023

I'd love to get your 👀 on #10547

@rvanlaak
Copy link
Contributor Author

@mpdude did check the changes and specifically the test cases on your PR, and they look extremely promising in solving all these commit order issues 👏

@rvanlaak
Copy link
Contributor Author

Feel free to also mark this PR as closed with your PR #10547

mpdude added a commit to mpdude/doctrine2 that referenced this pull request Jun 2, 2023
@Frikkle was the first to propose these tests in doctrine#6533.
@rvanlaak followed up in doctrine#8703, making some adjustments.
mpdude added a commit to mpdude/doctrine2 that referenced this pull request Jun 2, 2023
@Frikkle was the first to propose these tests in doctrine#6533.
@rvanlaak followed up in doctrine#8703, making some adjustments.

Co-authored-by: Gabe van der Weijde <[email protected]>
Co-authored-by: Richard van Laak <[email protected]>
Co-authored-by: Grégoire Paris <[email protected]>
@greg0ire greg0ire closed this Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants