deleting objects

[havvg]

see #636, #603

[havvg]

@marcj What do you say? You added most of the tests regarding #603.

I wrote down any combination I could come up with, and as far as I can see, the only configuration determining whether to delete or save the related object(s) is required="true|false". This configures whether it's actually valid to save the related one with the null value or not. If it's not valid, the related object has to be deleted as well and this should be done by the ORM instead of CASCADE by the database, e.g. $user->delete() does not actually delete but archive / soft delete it.

The altered test case in @bacf915 fails without the change of @145c0a2.

[marcj]

Hm, what happens to foreign keys where the local columns are not required? I believe they won't be deleted now with onDelete="CASCADE", right?

[havvg]

I just wrote a test and it passes. While writing it, I realized it's plain wrong, because the related object gets deleted - but not by Propel itself, but the database directly because of the CASCADE. Update incoming :)

[havvg]

I have no idea, how I could test this behavior, because of the Query::create static invocation.

[marcj]

You can use SQLite to check that, since we've not implemented there foreign keys yet.

[havvg]

Well, then I can't check the foreign keys, can I? (see @f317ce2)
The test case would fail as soon as the FK get implemented, which only delegates the issue to future us :)

[marcj]

Well, the actual mapping from the schema.xml files are still there, even though the SQLite platform just ignores it in the migration process. Means, $refFK should still contain the reference to a foreign key statement defined in the xml or did I miss something?

[havvg]

I will give it a try.

[havvg]

The PropelQuickBuilder is using sqlite by default, I added this testcase:

    public function testDeleteCascade()
    {
        \MoreRelationTest\PagePeer::doDeleteAll();
        \MoreRelationTest\ContentPeer::doDeleteAll();

        $page = new \MoreRelationTest\Page();
        $page->setTitle('Some important Page');

        $content = new \MoreRelationTest\Content();
        $content->setTitle('Content');

        $page->addContent($content);
        $page->save();

        $this->assertEquals(1, \MoreRelationTest\ContentQuery::create()->count());

        $page->delete();

        $this->assertEquals(0, \MoreRelationTest\ContentQuery::create()->count());
    }

However, it passes with and without the changes of @f317ce2. Am I missing something here?

[marcj]

Have you flagged the columns of the foreign key as required?

[havvg]

I thought they should be not required but cascade? Null is valid, but they should be deleted.

[havvg]

I tried it in our project, the mentioned changes are indeed required and result in the expected generated code, but still no valid test case to verify this. :(

[marcj]

Yes, they should not be required to test that, that's why I ask. :)

[havvg]

That's the case :) I'm using this schema: https://github.com/propelorm/Propel/blob/master/test/testsuite/generator/builder/om/GeneratedObjectMoreRelationTest.php#L31-L66

[marcj]

Found it, lol. Totally forgot that Peer does it magic too!

[marcj]

Ok, a test like that shows the issue with having no ForeignKey::CASCADE === $refFK->getOnDelete() check. It's because ->delete() triggers the peer hooks whereas ->save() does not.

    public function testOnDelete(){
        \MoreRelationTest\PagePeer::doDeleteAll();
        \MoreRelationTest\ContentPeer::doDeleteAll();

        $page = new \MoreRelationTest\Page();
        $page->setTitle('Some important Page');

        $content = new \MoreRelationTest\Content();
        $content->setTitle('Content');

        $page->addContent($content);
        $page->save();

        $this->assertEquals(1, \MoreRelationTest\ContentQuery::create()->count());

        $page->removeContent($content);
        $page->save();

        $this->assertEquals(0, \MoreRelationTest\ContentQuery::create()->count());
    }

[havvg]

But you are not deleting the page.

[havvg]

Ah wait, I see what you are doing there, thank you!

[havvg]

Added, rebased, squashed.

[marcj]

The only thing now is that https://github.com/propelorm/Propel/pull/656/files#L0R4317 makes the only change in the test result of GeneratedObjectMoreRelationTest.php. The other lines changed in generator/lib/builder/om/PHP5ObjectBuilder.php generate no difference. Can you explain why do you surround the addScheduledForDeletionAttribute calls with those conditions?

[havvg]

Because the attribute is not used, but only generated. Therefore I added the same condition where required, see https://github.com/havvg/Propel/blob/3bf0689decf37195148e948ca86151b9bc470e3e/generator/lib/builder/om/PHP5ObjectBuilder.php#L4790

[marcj]

ok, perfect! nice job 👯
I'd say: ready to merge

[havvg]

Cool, thanks for the review!

Ping @willdurand

[staabm]

great job guys! 👍

[willdurand]

thx guys!