DataObject::delete() does not delete live Versioned record

[emteknetnz]

Spun off from https://github.com/silverstripe/silverstripe-framework/pull/11131#issuecomment-1937946746

Calling DataObject::delete() on versioned object will only delete the draft record, the live record will remain on the live table

This is counter what the to the docblock on the method says and what the official docs say, the there's probably a bunch of code out there calling delete() and expecting it to correctly archive stuff.

Note that there is existing code that assumes that only a record from a single stage will be deleted when calling delete() for example Versioned::deleteFromStage() has this:

    public function deleteFromStage($stage)
    {
       // ...
        static::withVersionedMode(function () use ($stage, $owner) {
            Versioned::set_stage($stage);
            $clone = clone $owner;
            $clone->delete();
        });

So changing delete() to also delete from the live stage could have some undesirable side-effects if we don't account for that by changing the logic in Versioned.

Options

1. We could just update the relevant docs to say that delete() will only delete from a single stage, and you should call doArchive() to delete from both stages
   • We'd need to prominently call out this docs change and apologise for it being wrong and ask people to update their code
2. We could revisit the way Versioned handles this scenario, and see if there's a clean way to allow calling delete() to correctly archive records without causing bad or unexpected behaviour.
   • This would require refactoring the parts of Versioned that currently call delete(), in order to avoid recursive or other unexpected behaviour.

PRs

• https://github.com/silverstripe/silverstripe-framework/pull/11131

[GuySartorelli]

Adding type bug and edited description to show there's two options here - update docs to indicate that the old expected behaviour was wrong, or fix what I think is a bug so that behaviour aligns with the docs.

[kinglozzer]

We could just update the relevant docs to say that delete() will only delete from a single stage, and you should call doArchive() to delete from both stages

This is my preference - write() and delete() act on the current stage only, if you want anything else you have to use separate versioned APIs. That makes the most sense to me, and I think trying to adjust things for option 2 would be risky and unnecessarily time-consuming.