From 9bb798c18b33bd31259722d95a2e98b11ec6006d Mon Sep 17 00:00:00 2001 From: "Alexander A. Klimov" Date: Wed, 12 Oct 2016 11:06:04 +0200 Subject: [PATCH 1/5] IniWriter: don't persist a section key if the value is null refs #11743 --- library/Icinga/File/Ini/IniWriter.php | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/library/Icinga/File/Ini/IniWriter.php b/library/Icinga/File/Ini/IniWriter.php index 83410599f..f00040e39 100644 --- a/library/Icinga/File/Ini/IniWriter.php +++ b/library/Icinga/File/Ini/IniWriter.php @@ -149,6 +149,10 @@ class IniWriter $domSection = $doc->getSection($section); } foreach ($directives as $key => $value) { + if ($value === null) { + continue; + } + if ($value instanceof ConfigObject) { throw new ProgrammingError('Cannot diff recursive configs'); } From 014e7c136a2aaf60201726185c4d2f7655f9b0ad Mon Sep 17 00:00:00 2001 From: "Alexander A. Klimov" Date: Wed, 12 Oct 2016 11:07:21 +0200 Subject: [PATCH 2/5] Revert "UserGroupBackendForm: Do not persist null values, really" This reverts commit 975edbe548ecacabee22fb871e21dd2b8312dc19. refs #11743 --- .../forms/Config/UserGroup/UserGroupBackendForm.php | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/application/forms/Config/UserGroup/UserGroupBackendForm.php b/application/forms/Config/UserGroup/UserGroupBackendForm.php index 84c1cb4f8..42e76e347 100644 --- a/application/forms/Config/UserGroup/UserGroupBackendForm.php +++ b/application/forms/Config/UserGroup/UserGroupBackendForm.php @@ -127,14 +127,7 @@ class UserGroupBackendForm extends ConfigForm unset($data['name']); } - $backendConfig->merge($data); - foreach ($backendConfig->toArray() as $k => $v) { - if ($v === null) { - unset($backendConfig->$k); - } - } - - $this->config->setSection($name, $backendConfig); + $this->config->setSection($name, $backendConfig->merge($data)); return $this; } From 58b2e6c00f78a86199ed453c8804722665b543b3 Mon Sep 17 00:00:00 2001 From: "Alexander A. Klimov" Date: Wed, 12 Oct 2016 13:41:42 +0200 Subject: [PATCH 3/5] Add unit tests for the fixed IniWriter implementation refs #11743 --- .../library/Icinga/File/Ini/IniWriterTest.php | 49 +++++++++++++++++++ 1 file changed, 49 insertions(+) diff --git a/test/php/library/Icinga/File/Ini/IniWriterTest.php b/test/php/library/Icinga/File/Ini/IniWriterTest.php index 3148779ef..e4bfd2716 100644 --- a/test/php/library/Icinga/File/Ini/IniWriterTest.php +++ b/test/php/library/Icinga/File/Ini/IniWriterTest.php @@ -400,4 +400,53 @@ EOD; file_put_contents($this->tempFile, $config); return $this->tempFile; } + + public function testWhetherNullValuesGetPersisted() + { + $config = Config::fromArray(array()); + $section = $config->getSection('garbage'); + $section->foobar = null; + $config->setSection('garbage', $section); + + $iniWriter = new IniWriter($config, '/dev/null'); + $this->assertEquals( + 0, + preg_match('/foobar/', $iniWriter->render()), + 'IniWriter persists section keys with null values' + ); + } + + public function testWhetherEmptyValuesGetPersisted() + { + $config = Config::fromArray(array()); + $section = $config->getSection('garbage'); + $section->foobar = ''; + $config->setSection('garbage', $section); + + $iniWriter = new IniWriter($config, '/dev/null'); + $this->assertEquals( + 1, + preg_match('/foobar/', $iniWriter->render()), + 'IniWriter doesn\'t persist section keys with empty values' + ); + } + + public function testExplicitRemove() + { + $filename = tempnam(sys_get_temp_dir(), 'iw2'); + $config = Config::fromArray(array('garbage' => array('foobar' => 'lolcat'))); + $iniWriter = new IniWriter($config, $filename); + $iniWriter->write(); + + $section = $config->getSection('garbage'); + $section->foobar = null; + $iniWriter = new IniWriter($config, $filename); + $this->assertEquals( + 0, + preg_match('/foobar/', $iniWriter->render()), + 'IniWriter doesn\'t remove section keys with null values' + ); + + unlink($filename); + } } From 99866bfdbea99667ff7cf5bf27dfbbbb7efd5d05 Mon Sep 17 00:00:00 2001 From: "Alexander A. Klimov" Date: Mon, 17 Oct 2016 13:51:58 +0200 Subject: [PATCH 4/5] IniWriterTest: make recently added tests more expressive refs #11743 --- .../library/Icinga/File/Ini/IniWriterTest.php | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/test/php/library/Icinga/File/Ini/IniWriterTest.php b/test/php/library/Icinga/File/Ini/IniWriterTest.php index e4bfd2716..9e38aa9bd 100644 --- a/test/php/library/Icinga/File/Ini/IniWriterTest.php +++ b/test/php/library/Icinga/File/Ini/IniWriterTest.php @@ -409,9 +409,9 @@ EOD; $config->setSection('garbage', $section); $iniWriter = new IniWriter($config, '/dev/null'); - $this->assertEquals( - 0, - preg_match('/foobar/', $iniWriter->render()), + $this->assertNotContains( + 'foobar', + $iniWriter->render(), 'IniWriter persists section keys with null values' ); } @@ -424,9 +424,9 @@ EOD; $config->setSection('garbage', $section); $iniWriter = new IniWriter($config, '/dev/null'); - $this->assertEquals( - 1, - preg_match('/foobar/', $iniWriter->render()), + $this->assertContains( + 'foobar', + $iniWriter->render(), 'IniWriter doesn\'t persist section keys with empty values' ); } @@ -441,9 +441,9 @@ EOD; $section = $config->getSection('garbage'); $section->foobar = null; $iniWriter = new IniWriter($config, $filename); - $this->assertEquals( - 0, - preg_match('/foobar/', $iniWriter->render()), + $this->assertNotContains( + 'foobar', + $iniWriter->render(), 'IniWriter doesn\'t remove section keys with null values' ); From e2f6c81bfa41fe78cd9c3f8db3aeb09cfb2cc0d1 Mon Sep 17 00:00:00 2001 From: "Alexander A. Klimov" Date: Mon, 17 Oct 2016 15:00:43 +0200 Subject: [PATCH 5/5] Remove obsolete workarounds refs #11743 --- application/forms/Config/UserBackendConfigForm.php | 6 ------ application/forms/Navigation/NavigationConfigForm.php | 5 ----- .../application/forms/Config/BackendConfigForm.php | 6 ------ .../application/forms/Config/TransportConfigForm.php | 6 ------ 4 files changed, 23 deletions(-) diff --git a/application/forms/Config/UserBackendConfigForm.php b/application/forms/Config/UserBackendConfigForm.php index 421313820..f7d4d1572 100644 --- a/application/forms/Config/UserBackendConfigForm.php +++ b/application/forms/Config/UserBackendConfigForm.php @@ -200,12 +200,6 @@ class UserBackendConfigForm extends ConfigForm } $backendConfig->merge($data); - foreach ($backendConfig->toArray() as $k => $v) { - if ($v === null) { - unset($backendConfig->$k); - } - } - $this->config->setSection($name, $backendConfig); return $this; } diff --git a/application/forms/Navigation/NavigationConfigForm.php b/application/forms/Navigation/NavigationConfigForm.php index 158875c87..9bfc3a02a 100644 --- a/application/forms/Navigation/NavigationConfigForm.php +++ b/application/forms/Navigation/NavigationConfigForm.php @@ -426,11 +426,6 @@ class NavigationConfigForm extends ConfigForm } $itemConfig->merge($data); - foreach ($itemConfig->toArray() as $k => $v) { - if ($v === null) { - unset($itemConfig->$k); - } - } if ($shared) { // Share all descendant children diff --git a/modules/monitoring/application/forms/Config/BackendConfigForm.php b/modules/monitoring/application/forms/Config/BackendConfigForm.php index 78cb049de..e7fdba67c 100644 --- a/modules/monitoring/application/forms/Config/BackendConfigForm.php +++ b/modules/monitoring/application/forms/Config/BackendConfigForm.php @@ -147,12 +147,6 @@ class BackendConfigForm extends ConfigForm } $backendConfig->merge($data); - foreach ($backendConfig->toArray() as $k => $v) { - if ($v === null) { - unset($backendConfig->$k); - } - } - $this->config->setSection($name, $backendConfig); return $this; } diff --git a/modules/monitoring/application/forms/Config/TransportConfigForm.php b/modules/monitoring/application/forms/Config/TransportConfigForm.php index 0e5ad4f49..79ac1741b 100644 --- a/modules/monitoring/application/forms/Config/TransportConfigForm.php +++ b/modules/monitoring/application/forms/Config/TransportConfigForm.php @@ -167,12 +167,6 @@ class TransportConfigForm extends ConfigForm } $transportConfig->merge($data); - foreach ($transportConfig->toArray() as $k => $v) { - if ($v === null) { - unset($transportConfig->$k); - } - } - $this->config->setSection($name, $transportConfig); return $this; }