From bf20611fd43a609dd1d049c6b3ef820e93d4976d Mon Sep 17 00:00:00 2001 From: Noah Hilverling Date: Mon, 13 Feb 2017 16:52:52 +0100 Subject: [PATCH 1/5] ConfigForm: Fix that empty values are not handled correctly refs #2751 --- application/forms/ConfigForm.php | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/application/forms/ConfigForm.php b/application/forms/ConfigForm.php index 1c79600b5..8ee00884d 100644 --- a/application/forms/ConfigForm.php +++ b/application/forms/ConfigForm.php @@ -58,7 +58,7 @@ class ConfigForm extends Form { $sections = array(); foreach ($this->getValues() as $sectionAndPropertyName => $value) { - if (empty($value)) { + if ($value === '' || (is_array($value) && empty($value))) { $value = null; // Causes the config writer to unset it } @@ -137,8 +137,6 @@ class ConfigForm extends Form */ public static function transformEmptyValuesToNull(array $values) { - return array_map(function ($v) { - return empty($v) ? null : $v; - }, $values); + return array_map(function ($v) { return ($v === '' || (is_array($v) && empty($v))) ? null : $v; }, $values); } } From bd6e65374de00291107a1b4bf0f60dd4ee74bc00 Mon Sep 17 00:00:00 2001 From: Noah Hilverling Date: Tue, 14 Feb 2017 09:48:58 +0100 Subject: [PATCH 2/5] Add test for method transformEmptyValuesToNull refs #2751 --- .../php/library/Icinga/Web/ConfigFormTest.php | 52 +++++++++++++++++++ 1 file changed, 52 insertions(+) create mode 100644 test/php/library/Icinga/Web/ConfigFormTest.php diff --git a/test/php/library/Icinga/Web/ConfigFormTest.php b/test/php/library/Icinga/Web/ConfigFormTest.php new file mode 100644 index 000000000..071dbd23f --- /dev/null +++ b/test/php/library/Icinga/Web/ConfigFormTest.php @@ -0,0 +1,52 @@ + '', + 'value2' => 'this is a test', + 'value3' => array(), + 'value4' => array('Test1', 'Test2'), + 'value5' => 0, + 'value6' => 1 + ); + $values = ConfigForm::transformEmptyValuesToNull($values); + + $this->assertNull( + $values['value1'], + 'ConfigForm::transformEmptyValuesToNull does not handle empty strings correctly' + ); + $this->assertEquals( + 'this is a test', + $values['value2'], + 'ConfigForm::transformEmptyValuesToNull does not handle strings correctly' + ); + $this->assertNull( + $values['value3'], + 'ConfigForm::transformEmptyValuesToNull does not handle empty arrays correctly' + ); + $this->assertEquals( + 'Test1', + $values['value4'][0], + 'ConfigForm::transformEmptyValuesToNull does not handle arrays correctly' + ); + $this->assertEquals( + 0, + $values['value5'], + 'ConfigForm::transformEmptyValuesToNull does not handle zeros correctly' + ); + $this->assertEquals( + 1, + $values['value6'], + 'ConfigForm::transformEmptyValuesToNull does not handle numbers correctly' + ); + } +} From 9791e6ffb8fe6b1380f5cb6347aaaef70d461402 Mon Sep 17 00:00:00 2001 From: Noah Hilverling Date: Thu, 18 May 2017 15:39:24 +0200 Subject: [PATCH 3/5] ConfigForm: Do not ignore false while transforming to null refs #2751 --- application/forms/ConfigForm.php | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/application/forms/ConfigForm.php b/application/forms/ConfigForm.php index 8ee00884d..63492525f 100644 --- a/application/forms/ConfigForm.php +++ b/application/forms/ConfigForm.php @@ -58,7 +58,7 @@ class ConfigForm extends Form { $sections = array(); foreach ($this->getValues() as $sectionAndPropertyName => $value) { - if ($value === '' || (is_array($value) && empty($value))) { + if ($value === '' || $value === false || $value === []) { $value = null; // Causes the config writer to unset it } @@ -137,6 +137,8 @@ class ConfigForm extends Form */ public static function transformEmptyValuesToNull(array $values) { - return array_map(function ($v) { return ($v === '' || (is_array($v) && empty($v))) ? null : $v; }, $values); + return array_map(function ($v) { + return ($v === '' || $v === false || $v === []) ? null : $v; + }, $values); } } From 9ed56858499172aec5be2c4da7e69a5073f66caf Mon Sep 17 00:00:00 2001 From: Eric Lippmann Date: Thu, 9 Nov 2017 09:07:15 +0100 Subject: [PATCH 4/5] ConfigForm: Use transformEmptyValuesToNull() also in onSuccess() refs #2751 --- application/forms/ConfigForm.php | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/application/forms/ConfigForm.php b/application/forms/ConfigForm.php index 63492525f..f3a61ee79 100644 --- a/application/forms/ConfigForm.php +++ b/application/forms/ConfigForm.php @@ -57,11 +57,7 @@ class ConfigForm extends Form public function onSuccess() { $sections = array(); - foreach ($this->getValues() as $sectionAndPropertyName => $value) { - if ($value === '' || $value === false || $value === []) { - $value = null; // Causes the config writer to unset it - } - + foreach (static::transformEmptyValuesToNull($this->getValues()) as $sectionAndPropertyName => $value) { list($section, $property) = explode('_', $sectionAndPropertyName, 2); $sections[$section][$property] = $value; } @@ -137,8 +133,12 @@ class ConfigForm extends Form */ public static function transformEmptyValuesToNull(array $values) { - return array_map(function ($v) { - return ($v === '' || $v === false || $v === []) ? null : $v; - }, $values); + array_walk($values, function (&$v) { + if ($v === '' || $v === false || $v === array()) { + $v = null; + } + }); + + return $values; } } From f0ad3d5188cf06ecb811f9a9df02107716414aaf Mon Sep 17 00:00:00 2001 From: Eric Lippmann Date: Thu, 9 Nov 2017 09:10:57 +0100 Subject: [PATCH 5/5] Add more test cases to the transformEmptyValuesToNull() test refs #2751 --- .../php/library/Icinga/Web/ConfigFormTest.php | 87 ++++++++++++++----- 1 file changed, 63 insertions(+), 24 deletions(-) diff --git a/test/php/library/Icinga/Web/ConfigFormTest.php b/test/php/library/Icinga/Web/ConfigFormTest.php index 071dbd23f..dab3582ed 100644 --- a/test/php/library/Icinga/Web/ConfigFormTest.php +++ b/test/php/library/Icinga/Web/ConfigFormTest.php @@ -11,42 +11,81 @@ class ConfigFormTest extends BaseTestCase public function testWhetherTransformEmptyValuesToNullHandlesValuesCorrectly() { $values = array( - 'value1' => '', - 'value2' => 'this is a test', - 'value3' => array(), - 'value4' => array('Test1', 'Test2'), - 'value5' => 0, - 'value6' => 1 - ); + 'empty_string' => '', + 'example_string' => 'this is a test', + 'empty_array' => array(), + 'example_array' => array('test1', 'test2'), + 'zero_as_int' => 0, + 'one_as_int' => 1, + 'zero_as_string' => '0', + 'one_as_string' => '1', + 'bool_true' => true, + 'bool_false' => false, + 'null' => null + ); + $values = ConfigForm::transformEmptyValuesToNull($values); $this->assertNull( - $values['value1'], - 'ConfigForm::transformEmptyValuesToNull does not handle empty strings correctly' + $values['empty_string'], + 'ConfigForm::transformEmptyValuesToNull() does not handle empty strings correctly' ); - $this->assertEquals( + + $this->assertSame( 'this is a test', - $values['value2'], - 'ConfigForm::transformEmptyValuesToNull does not handle strings correctly' + $values['example_string'], + 'ConfigForm::transformEmptyValuesToNull() does not handle strings correctly' ); + $this->assertNull( - $values['value3'], - 'ConfigForm::transformEmptyValuesToNull does not handle empty arrays correctly' + $values['empty_array'], + 'ConfigForm::transformEmptyValuesToNull() does not handle empty arrays correctly' ); - $this->assertEquals( - 'Test1', - $values['value4'][0], - 'ConfigForm::transformEmptyValuesToNull does not handle arrays correctly' + + $this->assertSame( + 'test1', + $values['example_array'][0], + 'ConfigForm::transformEmptyValuesToNull() does not handle arrays correctly' ); - $this->assertEquals( + + $this->assertSame( 0, - $values['value5'], - 'ConfigForm::transformEmptyValuesToNull does not handle zeros correctly' + $values['zero_as_int'], + 'ConfigForm::transformEmptyValuesToNull() does not handle zeros correctly' ); - $this->assertEquals( + + $this->assertSame( 1, - $values['value6'], - 'ConfigForm::transformEmptyValuesToNull does not handle numbers correctly' + $values['one_as_int'], + 'ConfigForm::transformEmptyValuesToNull() does not handle numbers correctly' + ); + + $this->assertSame( + '0', + $values['zero_as_string'], + 'ConfigForm::transformEmptyValuesToNull() does not handle zeros correctly' + ); + + $this->assertSame( + '1', + $values['one_as_string'], + 'ConfigForm::transformEmptyValuesToNull() does not handle numbers correctly' + ); + + $this->assertSame( + true, + $values['bool_true'], + 'ConfigForm::transformEmptyValuesToNull() does not handle bool true correctly' + ); + + $this->assertNull( + $values['bool_false'], + 'ConfigForm::transformEmptyValuesToNull() does not handle bool false correctly' + ); + + $this->assertNull( + $values['null'], + 'ConfigForm::transformEmptyValuesToNull() does not handle null correctly' ); } }