BUG Fix writeBaseRecord with unique indexes

[tractorcow]

Fixes #6819

I've had this bug in mind for years. This will fix the below bug once and for all:

Couldn't run query:

INSERT INTO "App_MemberRole"
 (\"Created\")
 VALUES
 (?)

Duplicate entry '0-0' for key 'OneMemberPerProperty'

This often came up when writing ChangesetItem due to the primary key being a composite on a couple of columns.

[tractorcow]

Interestingly this bug has been around as long as SilverStripe has.

https://github.com/silverstripe/silverstripe-framework/blob/2.1/core/model/DataObject.php#L432-L436

[tractorcow]

Oops, prepareManipulationTable assumed ID was set. A small tweak fixed that. :)

[kinglozzer]

LGTM! 👍

[dhensby]

Nice one @tractorcow! 🎉

[patricknelson]

I'm guessing in the example above App_MemberRole is actually a descendant of another DataObject correct? If so, then the reason why this fix works (i.e. checking $this->isInDB() === true first) is because you're actually writing a new record in a child object's table, so the child object (who's ID must be synchronized with the parent object's table) doesn't actually exist in the DB, which is why you're assigning the ID. As the code is written, it looks backwards, but it works because the ->ID is indeed assigned because the base record has in fact already been written and (naturally) an artifact of that is the the ID will not be zero and thus no duplicate errors.

It's more of a point on succinct semantics and reducing tech debt/legacy. Maybe a comment can be introduced here, like:

        // Ensure we have an ID first and then synchronize ID's across child object records.
        if ($this->isInDB()) {
            $manipulation[$table]['id'] = $this->record['ID'];
        }

Or similar. I've proposed an explanation of an API level disambiguation on that nuance in #8411 (comment)

[tractorcow]

I would write something like // Provide ID if generated for this record, otherwise leave blank to trigger a new auto-increment value.

[robbieaverill]

@tractorcow the kitchen sink found some bugs that this introduced, I've made a tiny patch at #8839 to fix that

[robbieaverill]

@tractorcow ignore me, actually some dodgy logic in the auditor module