[DDC-2524] Wrong commit order with cascade remove and double association #707

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
6 participants
@mnapoli
Contributor

mnapoli commented Jun 24, 2013

This PR contains the failing test case for DDC-2524.

+
+use Doctrine\Common\Collections\ArrayCollection;
+
+require_once __DIR__ . '/../../../TestInit.php';

This comment has been minimized.

Show comment Hide comment
@stof

stof Jun 24, 2013

Member

you don't need this line as it is alreayd the phpunit bootstrap file

@stof

stof Jun 24, 2013

Member

you don't need this line as it is alreayd the phpunit bootstrap file

This comment has been minimized.

Show comment Hide comment
@mnapoli

mnapoli Jun 24, 2013

Contributor

Removed thanks

@mnapoli

mnapoli Jun 24, 2013

Contributor

Removed thanks

@guilhermeblanco

This comment has been minimized.

Show comment Hide comment
@guilhermeblanco

guilhermeblanco Jul 31, 2013

Owner

Code coverage probing that this functionality works was committed as of b070676

Owner

guilhermeblanco commented Jul 31, 2013

Code coverage probing that this functionality works was committed as of b070676

@mnapoli

This comment has been minimized.

Show comment Hide comment
@mnapoli

mnapoli Aug 9, 2013

Contributor

This PR was manually merged into master, but then reverted because it has failing tests (which is the purpose of this PR).

As explained in DDC-2524, it is actually a bug fixable in Doctrine, not a limitation of the DB, so could this PR and the Jira ticket be reopened please?

Contributor

mnapoli commented Aug 9, 2013

This PR was manually merged into master, but then reverted because it has failing tests (which is the purpose of this PR).

As explained in DDC-2524, it is actually a bug fixable in Doctrine, not a limitation of the DB, so could this PR and the Jira ticket be reopened please?

@mnapoli

This comment has been minimized.

Show comment Hide comment
@mnapoli

mnapoli Nov 4, 2013

Contributor

ping

Could this PR be reopened? The issue has been reopened, this PR is still useful, it contains a testcase reproducing the bug.

Contributor

mnapoli commented Nov 4, 2013

ping

Could this PR be reopened? The issue has been reopened, this PR is still useful, it contains a testcase reproducing the bug.

@Ocramius Ocramius reopened this Nov 4, 2013

@beberlei

This comment has been minimized.

Show comment Hide comment
@beberlei

beberlei Jan 3, 2014

Owner

@mnapoli Is this fixed with the DDC-2775 PR?

Owner

beberlei commented Jan 3, 2014

@mnapoli Is this fixed with the DDC-2775 PR?

@mnapoli

This comment has been minimized.

Show comment Hide comment
@mnapoli

mnapoli Jan 3, 2014

Contributor

@beberlei Nope, unfortunately.

I just merged this PR with master locally, and it's not fixed (you could also try to re-launch the travis job to confirm this). I re-read both tickets and they are not the same thing. DDC-2775 was an actual bug with cascade order because of lazy collections, DDC-2524 is more about "the cascade order could be improved for this situation to work, because it can be made to work". So it's less of a bug, but more of a improvement on how doctrine chooses the commit order.

Contributor

mnapoli commented Jan 3, 2014

@beberlei Nope, unfortunately.

I just merged this PR with master locally, and it's not fixed (you could also try to re-launch the travis job to confirm this). I re-read both tickets and they are not the same thing. DDC-2775 was an actual bug with cascade order because of lazy collections, DDC-2524 is more about "the cascade order could be improved for this situation to work, because it can be made to work". So it's less of a bug, but more of a improvement on how doctrine chooses the commit order.

+
+ /**
+ * @OneToOne(targetEntity="Doctrine\Tests\ORM\Functional\Ticket\CascadeRemoveOrderEntityG")
+ * @JoinColmun(nullable=true)

This comment has been minimized.

Show comment Hide comment
@netiul

netiul Apr 29, 2014

Contributor

Typo @joincolumn

@netiul

netiul Apr 29, 2014

Contributor

Typo @joincolumn

This comment has been minimized.

Show comment Hide comment
@mnapoli

mnapoli Apr 29, 2014

Contributor

Thanks! Fixed

@mnapoli

mnapoli Apr 29, 2014

Contributor

Thanks! Fixed

@mnapoli

This comment has been minimized.

Show comment Hide comment
@mnapoli

mnapoli Apr 29, 2014

Contributor

I have rebased the pull request and squashed the commits to make it cleaner. That also shows that it's still failing on master.

Contributor

mnapoli commented Apr 29, 2014

I have rebased the pull request and squashed the commits to make it cleaner. That also shows that it's still failing on master.

@netiul

This comment has been minimized.

Show comment Hide comment
@netiul

netiul Apr 29, 2014

Contributor

How do you work around this @mnapoli ?

Contributor

netiul commented Apr 29, 2014

How do you work around this @mnapoli ?

@mnapoli

This comment has been minimized.

Show comment Hide comment
@mnapoli

mnapoli Apr 29, 2014

Contributor

@netiul Very badly, sometimes I create a whole function dedicated to deleting the objects: it does the "remove cascade" manually and flushes several times in between (kind of try until it works), sometimes I set the associations as nullable (even though they shouldn't) and have a onDelete: SET NULL or something like that (I don't remember exactly I'm not working on that code right now).

Contributor

mnapoli commented Apr 29, 2014

@netiul Very badly, sometimes I create a whole function dedicated to deleting the objects: it does the "remove cascade" manually and flushes several times in between (kind of try until it works), sometimes I set the associations as nullable (even though they shouldn't) and have a onDelete: SET NULL or something like that (I don't remember exactly I'm not working on that code right now).

@netiul

This comment has been minimized.

Show comment Hide comment
@netiul

netiul Apr 29, 2014

Contributor

@mnapoli That's unfortunate. I was hoping you had a nice work around, since my solution is like yours :)

Contributor

netiul commented Apr 29, 2014

@mnapoli That's unfortunate. I was hoping you had a nice work around, since my solution is like yours :)

@netiul

This comment has been minimized.

Show comment Hide comment
@netiul

netiul Apr 29, 2014

Contributor

The best workaround I can come up with is hooking into the PreRemove LifecycleEvent, inside that event remove associations on both sides and call flush again inside the Event.

Assuming the remove action is cascaded, the following should work.

/** @ORM\HasLifecycleCallbacks */
class Entity 
{

    /**
     * @ORM\PreRemove
     */
    public function preRemove(LifecycleEventArgs $lifecycleEvent)
    {
        //set one-to-one association to null
        //for each item in collection of one-to-many association, remove assoc with $this

        $em = $lifecycleEvent->getEntityManager();
        $em->flush($this);
    }
}
Contributor

netiul commented Apr 29, 2014

The best workaround I can come up with is hooking into the PreRemove LifecycleEvent, inside that event remove associations on both sides and call flush again inside the Event.

Assuming the remove action is cascaded, the following should work.

/** @ORM\HasLifecycleCallbacks */
class Entity 
{

    /**
     * @ORM\PreRemove
     */
    public function preRemove(LifecycleEventArgs $lifecycleEvent)
    {
        //set one-to-one association to null
        //for each item in collection of one-to-many association, remove assoc with $this

        $em = $lifecycleEvent->getEntityManager();
        $em->flush($this);
    }
}

@Ocramius Ocramius changed the title from [#DDC-2524] Wrong commit order with cascade remove and double association to [DDC-2524] [Test] Wrong commit order with cascade remove and double association Jan 13, 2015

@Ocramius Ocramius changed the title from [DDC-2524] [Test] Wrong commit order with cascade remove and double association to [DDC-2524] Wrong commit order with cascade remove and double association Jan 24, 2015

@guilhermeblanco

This comment has been minimized.

Show comment Hide comment
@guilhermeblanco

guilhermeblanco Nov 25, 2015

Owner

@mnapoli I'm taking over this PR through a new one, managed by Doctrine itself #1570 including not only your test case, but also another one reported in the ticket (which looks like it was already fixed by my orphanRemoval support on CollectionPersister committed earlier this year.

Owner

guilhermeblanco commented Nov 25, 2015

@mnapoli I'm taking over this PR through a new one, managed by Doctrine itself #1570 including not only your test case, but also another one reported in the ticket (which looks like it was already fixed by my orphanRemoval support on CollectionPersister committed earlier this year.

@mnapoli

This comment has been minimized.

Show comment Hide comment
@mnapoli

mnapoli Nov 25, 2015

Contributor

👍 thank you!

Contributor

mnapoli commented Nov 25, 2015

👍 thank you!

@guilhermeblanco

This comment has been minimized.

Show comment Hide comment
@guilhermeblanco

guilhermeblanco Nov 27, 2015

Owner

@mnapoli on your second test testMany, you actually have a mapping issue.
You have to define the onDelete="SET NULL" because there's no way to control inverse awareness.

Once you change that, you fall back to same error as testSingle, which I addressed on #1570

Owner

guilhermeblanco commented Nov 27, 2015

@mnapoli on your second test testMany, you actually have a mapping issue.
You have to define the onDelete="SET NULL" because there's no way to control inverse awareness.

Once you change that, you fall back to same error as testSingle, which I addressed on #1570

@mnapoli

This comment has been minimized.

Show comment Hide comment
@mnapoli

mnapoli Nov 27, 2015

Contributor

Good to know. The original code is long gone, but thanks for following up.

Contributor

mnapoli commented Nov 27, 2015

Good to know. The original code is long gone, but thanks for following up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment