[DDC-2922] Error when new entities are reachable through multiple paths but not all of them are marked cascade-persist

[DHager]

Issue first-reported in DDC-2922.

• Uninteresting test to issue doesn't surface when all entities are new
• Reproduction test showing error
• Rough fix for error
• Analyze why simple-test doesn't repro it

As explained in the test-case, it seems to hinge on the "valid path" stemming a non-new object, so it's important that CmsEmail is flushed before the other things happen. If everything occurs together, the problem doesn't surface.

Note that I've made a small but essential cascade-persist change to CmsEmail in order to show the problem, but this change to the fixtures may or may not be desirable in the long-run.

[doctrinebot]

Hello,

thank you for creating this pull request. I have automatically opened an issue
on our Jira Bug Tracker for you. See the issue link:

http://www.doctrine-project.org/jira/browse/DDC-3921

We use Jira to track the state of pull requests and the versions they got
included in.

[kimhemsoe]

What happens if do not persist address ?

[DHager]

What happens if do not persist address ?

@kimhemsoe Then the Email/User stuff is changed successfully. The issue is that Doctrine "fails too fast" when it finds a non-cascade-persist link leading to a new object. In this case there is another (valid, cascading) link from Email->User that should, logically speaking, be enough to let everything continue.

In short, Doctrine sometimes errors if <100% of the references to an indirect new-object are cascade-persist. Ideally, it should only error if 0% of the references are cascade-persist.

[DHager]

I've checked in a proof-of-concept fix to go with the test-case.

[DHager]

@Ocramius : Does this issue/fix make sense? Would you prefer some other way to fix it?

[DHager]

@guilhermeblanco Any thoughts?

Alternatively, should Doctrine require that if an entity is cascaded in one relation, it must be also be configured as cascaded in all other possible relations?

[DHager]

@Ocramius This is a bug, right?

[Ocramius]

@DHager I need to take some time to read the tests and understand them

[DHager]

@Ocramius If I recall correctly...

In both cases the CmsUser is the only entity being discovered indirectly through associations, rather than being recorded directly with persist().

[DHager]

@Ocramius Without this fix, I think it forces people to exhaustively put cascading on everything/nothing, since otherwise a chance arrangement of changes may cause an error.

[DHager]

Bump.

[DHager]

Two-month bump: A coworker independently ran into the same issue.

[DHager]

Aw, dang, I missed celebrating the 1-year birthday of this PR.

[guilhermeblanco]

@DHager so if I understood correctly, the issue is not about tracking new entities indirectly in cascade persist, but rather tracking association changes that cascade persist but done indirectly from a non-cascade persist.

If this is correct, then there's a completely different approach to solve this problem... =\

[guilhermeblanco]

Expanding on "different approach". If it is valid, then the problem is that we're not fully tracking association changes, and that would be a bigger issue.
The solution would be to monitor changes at association level, instead of only field level.

[DHager]

@guilhermeblanco IMO this is purely issue of how the ORM explores the graph, and it's being far too hasty to reject a new node when it finds a non-cascading edge, even if other cascading-edges exist but haven't been seen yet.

To use an analogy:

1. Bob and Alice have a child, Carl.
2. Bob's family is very aristocratic and old-fashioned, so a lawyer is called in to determine whether Carl can eventually inherit some of the family fortune.
3. The lawyer takes one look at Alice (the mother) and says: "Since Alice is not a member of the clan, Carl cannot inherit."
4. The lawyer has failed because he gave up too soon, and didn't consider that Carl can inherit through his father.

Either the algorithm needs to check all the associations before disqualifying an entity (which is my fix in the PR) or else the documentation needs to be explicit that cascade-persist is something that must be set for all associations pointing to an entity, not just some of them.

[DHager]

Nostalgia-bump.

[Ocramius]

Nostalgia-bump.

@DHager assuming that I still have any life force at the end of this day, I'll work on it. So far, the approach looks simple/solid: I rebased the branch locally and am trying to simplify the tests.

[Ocramius]

@DHager I cleaned up and simplified the tests, reproduced the issue in isolation in a unit test and made sure the exception produces a message containing all involved entities. This is 🚢 -ed and ready for 2.6 👍

Thanks for all the patience (too much of it :-) )