[GRIDFIELD] GridField: Duplicate write()-call with HasManyList

[th3hamm0r]

Affected Version

We've first found it in 3.6 (I think it affects all 3.x version) and in 4.x it is still present.

Description

If you have a model that contains a "has_many"-relation and you use a GridField to manage it, saving one of these related objects actually triggers two write() calls on that object. We found this issue, because if the object is Versioned a "save" operation in the CMS actually creates two versions!

This happens since the GridFieldDetailForm_ItemRequest first calls write() on the edited record and afterwards the add() operation of the HasManyList again calls write() on that object. It looks like for some reason the checks in write() which should protect from multiple db writes do not work in this case.

In SilverStripe 3.6 we've quick-fixed it by wrapping https://github.com/silverstripe/silverstripe-framework/blob/3.6/forms/gridfield/GridFieldDetailForm.php#L557 with:

// This avoids two writes for DataObjects in a HasManyList, since add() also calls write() for those lists.
if (!($list instanceof HasManyList)) {
    $this->record->write();
}

In SilverStripe 4.x the code is here and could be fixed similarly.

But this is more like a quick-fix, because there may be still some other code that bypasses the checks in DataObject's write() and triggers additional db writes.

[tractorcow]

I don't think this is an impact/high. The worst thing is a slight slow down / unnecessary work.

[tractorcow]

@th3hamm0r what if you swapped the code around. This should not write (since the last write has no changes)

old:

$form->saveInto($this->record);
$this->record->write();
$extraData = $this->getExtraSavedData($this->record, $list);
$list->add($this->record, $extraData);

new:

$form->saveInto($this->record);
$extraData = $this->getExtraSavedData($this->record, $list);
$list->add($this->record, $extraData);
$this->record->write(); // no-op if already saved due to change detection

[tractorcow]

Oh, in 4.1.x ManyManyList::add() works with unsaved records. I guess it doesn't work in 3.x / 4.0.

[tractorcow]

4.1: ManyManyList::add()

// Ensure record is saved
if (!$item->isInDB()) {
    $item->write();
}
$itemID = $item->ID;

4.0 and below:

if (empty($itemID)) {
    throw new InvalidArgumentException("ManyManyList::add() doesn't accept unsaved records");
}

[th3hamm0r]

@tractorcow Yes, your suggestion above works too (tested it with 4.0.0).

[tractorcow]

yeah but it will break 4.0 with manymanylist. In 4.1 it'll safely lazy-save as needed.