From 0dd97a38f6ef9c46232f1f2d2a4857e7e94d4e27 Mon Sep 17 00:00:00 2001 From: Hamish Friedlander Date: Fri, 16 Nov 2012 11:47:32 +1300 Subject: [PATCH 1/2] API: Form#loadDataFrom 2nd arg now sets how existing field data is merged with new data --- forms/Form.php | 108 +++++++++++++++++++++++++++------------ tests/forms/FormTest.php | 32 +++++++++++- 2 files changed, 104 insertions(+), 36 deletions(-) diff --git a/forms/Form.php b/forms/Form.php index c93d1e453..d1cf61b03 100644 --- a/forms/Form.php +++ b/forms/Form.php @@ -1080,6 +1080,10 @@ class Form extends RequestHandler { return true; } + const MERGE_DEFAULT = 0; + const MERGE_CLEAR_MISSING = 1; + const MERGE_IGNORE_FALSEISH = 2; + /** * Load data from the given DataObject or array. * It will call $object->MyField to get the value of MyField. @@ -1098,20 +1102,43 @@ class Form extends RequestHandler { * @uses FormField->setValue() * * @param array|DataObject $data - * @param boolean $clearMissingFields By default, fields which don't match - * a property or array-key of the passed {@link $data} argument are "left alone", - * meaning they retain any previous values (if present). If this flag is set to true, - * those fields are overwritten with null regardless if they have a match in {@link $data}. - * @param $fieldList An optional list of fields to process. This can be useful when you have a + * @param int $mergeStrategy + * For every field, {@link $data} is interogated whether it contains a relevant property/key, and + * what that property/key's value is. + * + * By default, if {@link $data} does contain a property/key, the fields value is always replaced by {@link $data}'s + * value, even if that value is null/false/etc. Fields which don't match any property/key in {@link $data} are + * "left alone", meaning they retain any previous value. + * + * You can pass a bitmask here to change this behaviour. + * + * Passing CLEAR_MISSING means that any fields that don't match any property/key in + * {@link $data} are cleared. + * + * Passing IGNORE_FALSEISH means that any false-ish value in {@link $data} won't replace + * a field's value. + * + * For backwards compatibility reasons, this parameter can also be set to === true, which is the same as passing + * CLEAR_MISSING + * + * @param $fieldList An optional list of fields to process. This can be useful when you have a * form that has some fields that save to one object, and some that save to another. * @return Form */ - public function loadDataFrom($data, $clearMissingFields = false, $fieldList = null) { + public function loadDataFrom($data, $mergeStrategy = 0, $fieldList = null) { if(!is_object($data) && !is_array($data)) { user_error("Form::loadDataFrom() not passed an array or an object", E_USER_WARNING); return $this; } + // Handle the backwards compatible case of passing "true" as the second argument + if ($mergeStrategy === true) { + $mergeStrategy = self::MERGE_CLEAR_MISSING; + } + else if ($mergeStrategy === false) { + $mergeStrategy = 0; + } + // if an object is passed, save it for historical reference through {@link getRecord()} if(is_object($data)) $this->record = $data; @@ -1125,37 +1152,50 @@ class Form extends RequestHandler { // First check looks for (fieldname)_unchanged, an indicator that we shouldn't overwrite the field value if(is_array($data) && isset($data[$name . '_unchanged'])) continue; - - // get value in different formats - $hasObjectValue = false; - if( - is_object($data) - && ( - isset($data->$name) - || $data->hasMethod($name) - || ($data->hasMethod('hasField') && $data->hasField($name)) - ) - ) { - // We don't actually call the method because it might be slow. - // In a later release, relation methods will just return references to the query that should be - // executed, and so we will be able to safely pass the return value of the relation method to the - // first argument of setValue - $val = $data->__get($name); - $hasObjectValue = true; - } else if(strpos($name,'[') && is_array($data) && !isset($data[$name])) { - // if field is in array-notation, we need to resolve the array-structure PHP creates from query-strings - preg_match('/' . addcslashes($name,'[]') . '=([^&]*)/', urldecode(http_build_query($data)), $matches); - $val = isset($matches[1]) ? $matches[1] : null; - } elseif(is_array($data) && array_key_exists($name, $data)) { - // else we assume its a simple keyed array - $val = $data[$name]; - } else { - $val = null; + + // Does this property exist on $data? + $exists = false; + // The value from $data for this field + $val = null; + + if(is_object($data)) { + $exists = ( + isset($data->$name) || + $data->hasMethod($name) || + ($data->hasMethod('hasField') && $data->hasField($name)) + ); + + if ($exists) { + $val = $data->__get($name); + } + } + else if(is_array($data)){ + if(array_key_exists($name, $data)) { + $exists = true; + $val = $data[$name]; + } + // If field is in array-notation we need to access nested data + else if(strpos($name,'[')) { + // First encode data using PHP's method of converting nested arrays to form data + $flatData = urldecode(http_build_query($data)); + // Then pull the value out from that flattened string + preg_match('/' . addcslashes($name,'[]') . '=([^&]*)/', $flatData, $matches); + + if (isset($matches[1])) { + $exists = true; + $val = $matches[1]; + } + } } // save to the field if either a value is given, or loading of blank/undefined values is forced - if(isset($val) || $hasObjectValue || $clearMissingFields) { - // pass original data as well so composite fields can act on the additional information + if($exists){ + if ($val != false || ($mergeStrategy & self::MERGE_IGNORE_FALSEISH) != self::MERGE_IGNORE_FALSEISH){ + // pass original data as well so composite fields can act on the additional information + $field->setValue($val, $data); + } + } + else if(($mergeStrategy & self::MERGE_CLEAR_MISSING) == self::MERGE_CLEAR_MISSING){ $field->setValue($val, $data); } } diff --git a/tests/forms/FormTest.php b/tests/forms/FormTest.php index fc356d40b..dbba1aa03 100644 --- a/tests/forms/FormTest.php +++ b/tests/forms/FormTest.php @@ -153,7 +153,7 @@ class FormTest extends FunctionalTest { $captainWithDetails = $this->objFromFixture('FormTest_Player', 'captainNoDetails'); $team2 = $this->objFromFixture('FormTest_Team', 'team2'); $form->loadDataFrom($captainWithDetails); - $form->loadDataFrom($team2, true); + $form->loadDataFrom($team2, Form::MERGE_CLEAR_MISSING); $this->assertEquals( $form->getData(), array( @@ -166,7 +166,35 @@ class FormTest extends FunctionalTest { 'LoadDataFrom() overwrites fields not found in the object with $clearMissingFields=true' ); } - + + public function testLoadDataFromIgnoreFalseish() { + $form = new Form( + new Controller(), + 'Form', + new FieldList( + new TextField('Biography', 'Biography', 'Custom Default') + ), + new FieldList() + ); + + $captainNoDetails = $this->objFromFixture('FormTest_Player', 'captainNoDetails'); + $captainWithDetails = $this->objFromFixture('FormTest_Player', 'captainWithDetails'); + + $form->loadDataFrom($captainNoDetails, Form::MERGE_IGNORE_FALSEISH); + $this->assertEquals( + $form->getData(), + array('Biography' => 'Custom Default'), + 'LoadDataFrom() doesn\'t overwrite fields when MERGE_IGNORE_FALSEISH set and values are false-ish' + ); + + $form->loadDataFrom($captainWithDetails, Form::MERGE_IGNORE_FALSEISH); + $this->assertEquals( + $form->getData(), + array('Biography' => 'Bio 1'), + 'LoadDataFrom() does overwrite fields when MERGE_IGNORE_FALSEISH set and values arent false-ish' + ); + } + public function testFormMethodOverride() { $form = $this->getStubForm(); $form->setFormMethod('GET'); From 7315be4531975998f683e41b48781d3ed7d9c74f Mon Sep 17 00:00:00 2001 From: Hamish Friedlander Date: Fri, 16 Nov 2012 11:48:31 +1300 Subject: [PATCH 2/2] FIX default values from DataObject not showing in GridField details form --- forms/gridfield/GridFieldDetailForm.php | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/forms/gridfield/GridFieldDetailForm.php b/forms/gridfield/GridFieldDetailForm.php index 4aeef7931..d6000e10d 100644 --- a/forms/gridfield/GridFieldDetailForm.php +++ b/forms/gridfield/GridFieldDetailForm.php @@ -325,9 +325,8 @@ class GridFieldDetailForm_ItemRequest extends RequestHandler { $actions, $this->component->getValidator() ); - if($this->record->ID !== 0) { - $form->loadDataFrom($this->record); - } + + $form->loadDataFrom($this->record, $this->record->ID == 0 ? Form::MERGE_IGNORE_FALSEISH : Form::MERGE_DEFAULT); // TODO Coupling with CMS $toplevelController = $this->getToplevelController();