delete without ID deletes all

[sunnysideup]

Hi,

Sorry, just a quick note here about a finding we made.

We had some code that went something like this:

        if($this->recordBeingEdited->canDelete()) {
            if($this->recordBeingEdited instanceof SiteTree) {
                $this->recordBeingEdited->deleteFromStage('Live');
                $this->recordBeingEdited->deleteFromStage('Stage');

            }
            else {
                $this->recordBeingEdited->delete();
            }
        }

We found that if the recordBeingEdited was a SiteTree object (e.g. a ProductPage) but had no ID (e.g. new record?) then it would actually delete the entire sitetree ;-)

We are not 100% sure about this, but it definitely gave very strange results. It seems to me that this is a bit drastic. Better to have an error saying: cant delete object without ID or something like that.

Pull Requests

• [x] https://github.com/silverstripe/silverstripe-cms/pull/1994
• [x] https://github.com/silverstripe/silverstripe-cms/pull/1993

[kinglozzer]

I think we may indeed have a bug here. DataObject::delete() does throw an exception if you try to delete an object without an ID, but it throws that after calling onBeforeDelete(). SiteTree::onBeforeDelete() will, by default, run through and delete all child pages.

My guess at this point is that Hierarchy will try to find all pages with the parent ID of the page being deleted (except it doesn’t have an ID, so presumably that comes out as 0). So it finds all top level pages as they have ParentID => 0 and deletes them, which in turn deletes all child pages too.

IMO we shouldn’t be triggering onBeforeDelete() before we check whether the object has an ID. I’m not sure whether that counts as an API/BC-breaking change?

[kinglozzer]

Just tested locally, can confirm the above theory is correct:

$page = new Page();
$page->delete();

Will result in all pages being deleted from the current stage. Oops. Nice find @sunnysideup!

[dhensby]

Well, here's some tests to prove it https://github.com/dhensby/silverstripe-framework/commits/pulls/3.1/check-delte-non-existant-obj

[dhensby]

One problem appears to be that Hierarchy performs the delete cascade onBeforeDelete not onAfterDelete, which is wrong

[tractorcow]

Wow that is a nice find... I agree that we should move the recursive delete to onAfterDelete(), but other extensions might still behave incorrectly. Maybe we should do that as well as move the exception to just before the first onBeforeDelete.

[dhensby]

Maybe we should do that as well as move the exception to just before the first onBeforeDelete.

Maybe - but the onBefore* hooks should run even if the actual action isn't going to complete - so I would try to avoid moving it

[tractorcow]

Sure, I guess until someone can come up with a valid reason why it should move, it might be best not to change anything.

[jonom]

I think the result of this is that we should leave the exception where it is, but move the recursive delete to the onAfterDelete() method. Is anyone still working on this? Seems like a pretty important bug to fix. @dhensby you've already written tests so that's probably 95% of the work done :)

[sunnysideup]

sorry, I am not working on this ;-)

[dhensby]

I might be able to work on it, but I'm low on time... @tractorcow ?

[tractorcow]

I've raised the impact of this to critical; Will try to address some time this week.

[tractorcow]

If it's critical, we should probably fix this as far back as 3.1.19, although we can use a different solution for 3.1-3.3 if we want to change the behaviour in 3.4.

[unclecheese]

As @dhensby has indicated in his tests, there are two issues that are creating this bug, independent of each other. One is that the cascading delete happens too early in SiteTree, and the second is that AllChildren() returns a non-empty array for non-existent objects. Only one of these two issues needs to be patched to mitigate the bug. If anyone can see a really good reason to keep cascading deletes in onBeforeDelete, we should leave it alone, and fix this issue simply by patching the AllChildren() issue, but otherwise, I prefer to fix both.

[unclecheese]

Thinking about this a bit more, I don't think the AllChildren() issue should be fixed. If the default value for ParentID is effectively 0, then it's reasonable for the user to expect it to return a valid list of children, even before it's been written.

[dhensby]

I'm really not sure that you should get a result for AllChildren for a non-existing object.

I'd say it's unexpected for SiteTree::create()->AllChildren() to return anything. If you want to literally get all the pages (where ParentID = 0) then you should be running that query yourself and not relying on some dark magic of using a non-existing SiteTree object

[flamerohr]

I agree that AllChildren probably shouldn't return anything, if you really wanted all top-level ParentID = 0 items, then you should be explicitly calling for this one with a query yourself.

Creating a new object or singleton to then use AllChildren for top-level makes little semantic sense...

[unclecheese]

Wouldn't this be true? And if so, wouldn't we want it to stay true?

$page = new Page();
echo $page->ParentID // 0;
$page->ParentID = 5;
$page->AllChildren()->count() > 0  // true

If left untouched, ParentID is 0, so why should writing the record make a difference for only root level pages?

Ideology aside, this would be an API breakage, and I think that's a bigger reason why we shouldn't do it.

[flamerohr]

I think you might be looking at this the wrong way...?

$page = new Page();
echo $page->ID // 0;
$page->AllChildren()->count() > 0  // wait, what?

We're querying for Page which belong to the new Page, which hasn't been added to the database yet...? and it returns a list of top-level pages as its children now

[unclecheese]

Ah, right, ID, not ParentID. I suppose the use case for explicitly assigning an ID are limited. :)

Still an API breakage, though.

[flamerohr]

yeah, I agree that it is, so maybe we could look at change for this when there's a planned version bump somewhere along the lines

[tractorcow]

I had the same concerns; See my comment over at https://github.com/silverstripe/silverstripe-cms/pull/1988

[unclecheese]

Per discussion with @tractorcow on Slack, we are currently relying on singleton($obj)->Children() to create a parent of the root, "god page" of sorts in 3.x. So we can't fix this at the Hierarchy level. For consistency, and so as not to break the Hierarchy API in 4.x, we'll apply this fix at the SiteTree level only, adding a isInDB() check.

[unclecheese]

Have redone the PRs. See updated OP with new links.