Skip to content

postDelete hook does not fire on soft deleted objects #48

Closed
jakerella opened this Issue Aug 18, 2011 · 7 comments

2 participants

@jakerella

Doesn't seem like this should be the case (but please let me know if this is expected behavior): When using the "soft_delete" behavior a deleted object will fire the preDelete hook, but not the postDelete hook. The problem lies in the fact that the "if" block for soft delete issues a "return" prematurely. Additionally, any cascading is killed due to this return.

I'll try to get a patch working, but wanted to make sure this was not expected behavior first

Current code (generated BaseMyClass):

public function delete(PropelPDO $con = null)
{
...
    $ret = $this->preDelete($con);
    // soft_delete behavior
    if (!empty($ret) && MyClassQuery::isSoftDeleteEnabled()) {
        $this->keepUpdateDateUnchanged();
        $this->setDeletedAt(time());
        $this->save($con);
        $con->commit();
        MyClassPeer::removeInstanceFromPool($this);
        return;   // <-- this return forces a skip of both the postDelete hook and cascading
    }

    if ($ret) {
        MyClassQuery::create()
            ->filterByPrimaryKey($this->getPrimaryKey())
            ->delete($con);
        $this->postDelete($con);
        // versionable behavior
        // emulate delete cascade
        MyClassVersionQuery::create()
            ->filterByMyClass($this)
            ->delete($con);
        $con->commit();
        $this->setDeleted(true);
    } else {
        $con->commit();
    }
...
}
@jakerella

Okay, adding the following code to line 96 of SoftDeleteBehavior will at least fix the postDelete hook. I suppose I can manually do the cascading on the objects that need it, but not sure how to fix that otherwise.

  \$this->postDelete(\$con);
@willdurand
Propel member

Hi, could you provide a unit test to test the postDelete hook ?

@jakerella

Hmm... I think the test would be part of SoftDeleteBehaviorTest (which already exists). Note that I have NOT run this test yet as I don't have my system set up to test the core Propel system. Anyone else want to do that for me? :)

public function testPostDelete()
{
    $t = new PostdeletehookedTable4();
    $t->setTitle('not post-deleted');
    $t->save();
    $t->delete();
    $this->assertNotNull($t->getDeletedAt(), 'deleted_column is not null after a soft delete');
    $this->assertEquals('post-deleted', $t->getTitle(), 'postDelete hook did not set new title as expected');
}

class PostdeletehookedTable4 extends Table4
{
    public function postDelete(PropelPDO $con = null)
    {
        parent::postDelete($con);
        $this->setTitle('post-deleted');
        $this->save();
    }
}
@jakerella

Off-topic question: how do I add labels to this issue? Or is that reserved functionality?

@willdurand
Propel member

I'll try this test this evening.

You cannot set the label yourself ;)

@willdurand
Propel member

Well, the provided unit test is ok. I attached a PR to this issue.
To know if to not call the post hook is a feature or not, that's the real question..

@willdurand
Propel member

PR #50 fixes this issue, thanks.

@willdurand willdurand closed this Aug 24, 2011
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.