From acf19b72e2a6fb52527a788b1ed87f552e57f314 Mon Sep 17 00:00:00 2001 From: Damian Mooyman Date: Mon, 8 Jun 2015 15:44:27 +1200 Subject: [PATCH] BUG Fix false values for many_many_ExtraFields not being saved Fixes #4067 --- forms/gridfield/GridFieldDetailForm.php | 37 +++++++--- model/ManyManyList.php | 74 +++++++++---------- .../gridfield/GridFieldDetailFormTest.php | 42 ++++++++++- 3 files changed, 103 insertions(+), 50 deletions(-) diff --git a/forms/gridfield/GridFieldDetailForm.php b/forms/gridfield/GridFieldDetailForm.php index 9baf1cea7..5eeabcfcb 100644 --- a/forms/gridfield/GridFieldDetailForm.php +++ b/forms/gridfield/GridFieldDetailForm.php @@ -495,21 +495,39 @@ class GridFieldDetailForm_ItemRequest extends RequestHandler { return $backlink; } - - + + /** + * Get the list of extra data from the $record as saved into it by + * {@see Form::saveInto()} + * + * Handles detection of falsey values explicitly saved into the + * DataObject by formfields + * + * @param DataObject $record + * @param SS_List $list + * @return array List of data to write to the relation + */ + protected function getExtraSavedData($record, $list) { + // Skip extra data if not ManyManyList + if(!($list instanceof ManyManyList)) { + return null; + } + + $data = array(); + foreach($list->getExtraFields() as $field => $dbSpec) { + $savedField = "ManyMany[{$field}]"; + if($record->hasField($savedField)) { + $data[$field] = $record->getField($savedField); + } + } + return $data; + } public function doSave($data, $form) { $new_record = $this->record->ID == 0; $controller = $this->getToplevelController(); $list = $this->gridField->getList(); - if($list instanceof ManyManyList) { - // Data is escaped in ManyManyList->add() - $extraData = (isset($data['ManyMany'])) ? $data['ManyMany'] : null; - } else { - $extraData = null; - } - if(!$this->record->canEdit()) { return $controller->httpError(403); } @@ -527,6 +545,7 @@ class GridFieldDetailForm_ItemRequest extends RequestHandler { try { $form->saveInto($this->record); $this->record->write(); + $extraData = $this->getExtraSavedData($this->record, $list); $list->add($this->record, $extraData); } catch(ValidationException $e) { $form->sessionMessage($e->getResult()->message(), 'bad', false); diff --git a/model/ManyManyList.php b/model/ManyManyList.php index 7c4f7a4a7..96eb7332e 100644 --- a/model/ManyManyList.php +++ b/model/ManyManyList.php @@ -230,37 +230,29 @@ class ManyManyList extends RelationList { $hasExisting = false; } - $manipulation = array(); + // Blank manipulation + $manipulation = array( + $this->joinTable => array( + 'command' => $hasExisting ? 'update' : 'insert', + 'fields' => array() + ) + ); if($hasExisting) { - $manipulation[$this->joinTable]['command'] = 'update'; $manipulation[$this->joinTable]['where'] = array( "\"{$this->joinTable}\".\"{$this->foreignKey}\"" => $foreignID, "\"{$this->joinTable}\".\"{$this->localKey}\"" => $itemID ); - } else { - $manipulation[$this->joinTable]['command'] = 'insert'; } - if($extraFields) { - foreach($extraFields as $fieldName => $fieldValue) { - if(is_null($fieldValue)) { - $manipulation[$this->joinTable]['fields'][$fieldName] = null; - } elseif($fieldValue instanceof DBField) { - // rely on writeToManipulation to manage the changes - // required for this field. - $working = array('fields' => array()); - - // create a new instance of the field so we can - // modify the field name to the correct version. - $field = DBField::create_field(get_class($fieldValue), $fieldValue); - $field->setName($fieldName); - $field->writeToManipulation($working); - - foreach($working['fields'] as $extraName => $extraValue) { - $manipulation[$this->joinTable]['fields'][$extraName] = $extraValue; - } - } else { - $manipulation[$this->joinTable]['fields'][$fieldName] = $fieldValue; + if($extraFields && $this->extraFields) { + // Write extra field to manipluation in the same way + // that DataObject::prepareManipulationTable writes fields + foreach($this->extraFields as $fieldName => $fieldSpec) { + // Skip fields without an assignment + if(array_key_exists($fieldName, $extraFields)) { + $fieldObject = Object::create_from_string($fieldSpec, $fieldName); + $fieldObject->setValue($extraFields[$fieldName]); + $fieldObject->writeToManipulation($manipulation[$this->joinTable]); } } } @@ -357,24 +349,32 @@ class ManyManyList extends RelationList { */ public function getExtraData($componentName, $itemID) { $result = array(); + + // Skip if no extrafields or unsaved record + if(empty($this->extraFields) || empty($itemID)) { + return $result; + } if(!is_numeric($itemID)) { user_error('ComponentSet::getExtraData() passed a non-numeric child ID', E_USER_ERROR); } - if($this->extraFields) { - $cleanExtraFields = array(); - foreach ($this->extraFields as $fieldName => $dbFieldSpec) { - $cleanExtraFields[] = "\"{$fieldName}\""; - } - $query = new SQLQuery($cleanExtraFields, "\"{$this->joinTable}\""); - if($filter = $this->foreignIDWriteFilter($this->getForeignID())) { - $query->setWhere($filter); - } else { - user_error("Can't call ManyManyList::getExtraData() until a foreign ID is set", E_USER_WARNING); - } - $query->addWhere("\"{$this->localKey}\" = {$itemID}"); - $queryResult = $query->execute()->current(); + $cleanExtraFields = array(); + foreach ($this->extraFields as $fieldName => $dbFieldSpec) { + $cleanExtraFields[] = "\"{$fieldName}\""; + } + $query = new SQLQuery($cleanExtraFields, "\"{$this->joinTable}\""); + $filter = $this->foreignIDWriteFilter($this->getForeignID()); + if($filter) { + $query->setWhere($filter); + } else { + user_error("Can't call ManyManyList::getExtraData() until a foreign ID is set", E_USER_WARNING); + } + $query->addWhere(array( + "\"{$this->localKey}\"" => $itemID + )); + $queryResult = $query->execute()->current(); + if ($queryResult) { foreach ($queryResult as $fieldName => $value) { $result[$fieldName] = $value; } diff --git a/tests/forms/gridfield/GridFieldDetailFormTest.php b/tests/forms/gridfield/GridFieldDetailFormTest.php index caea5ac0e..a3bac5dfc 100644 --- a/tests/forms/gridfield/GridFieldDetailFormTest.php +++ b/tests/forms/gridfield/GridFieldDetailFormTest.php @@ -174,11 +174,15 @@ class GridFieldDetailFormTest extends FunctionalTest { $manyManyField = $parser->getByXpath('//*[@id="Form_ItemEditForm"]//input[@name="ManyMany[IsPublished]"]'); $this->assertTrue((bool)$manyManyField); + // Test save of IsPublished field $response = $this->post( $editformurl, array( 'Name' => 'Updated Category', - 'ManyMany' => array('IsPublished' => 1), + 'ManyMany' => array( + 'IsPublished' => 1, + 'PublishedBy' => 'Richard' + ), 'action_doSave' => 1 ) ); @@ -187,7 +191,33 @@ class GridFieldDetailFormTest extends FunctionalTest { $person = GridFieldDetailFormTest_Person::get()->sort('FirstName')->First(); $category = $person->Categories()->filter(array('Name' => 'Updated Category'))->First(); $this->assertEquals( - array('IsPublished' => 1), + array( + 'IsPublished' => 1, + 'PublishedBy' => 'Richard' + ), + $person->Categories()->getExtraData('', $category->ID) + ); + + // Test update of value with falsey value + $response = $this->post( + $editformurl, + array( + 'Name' => 'Updated Category', + 'ManyMany' => array( + 'PublishedBy' => '' + ), + 'action_doSave' => 1 + ) + ); + $this->assertFalse($response->isError()); + + $person = GridFieldDetailFormTest_Person::get()->sort('FirstName')->First(); + $category = $person->Categories()->filter(array('Name' => 'Updated Category'))->First(); + $this->assertEquals( + array( + 'IsPublished' => 0, + 'PublishedBy' => '' + ), $person->Categories()->getExtraData('', $category->ID) ); } @@ -316,7 +346,8 @@ class GridFieldDetailFormTest_Person extends DataObject implements TestOnly { private static $many_many_extraFields = array( 'Categories' => array( - 'IsPublished' => 'Boolean' + 'IsPublished' => 'Boolean', + 'PublishedBy' => 'Varchar' ) ); @@ -464,7 +495,10 @@ class GridFieldDetailFormTest_CategoryController extends Controller implements T // GridField lists categories for a specific person $person = GridFieldDetailFormTest_Person::get()->sort('FirstName')->First(); $detailFields = singleton('GridFieldDetailFormTest_Category')->getCMSFields(); - $detailFields->addFieldToTab('Root.Main', new CheckboxField('ManyMany[IsPublished]')); + $detailFields->addFieldsToTab('Root.Main', array( + new CheckboxField('ManyMany[IsPublished]'), + new TextField('ManyMany[PublishedBy]')) + ); $field = new GridField('testfield', 'testfield', $person->Categories()); $field->getConfig()->addComponent($gridFieldForm = new GridFieldDetailForm($this, 'Form')); $gridFieldForm->setFields($detailFields);