Merge branch 'bugfix/preserving-ini-writer-order-4610-4615'

resolves #4610
resolves #4615

application/controllers/ConfigController.php

@@ -61,7 +61,6 @@ class ConfigController extends BaseConfigController
                     'url'       => Url::fromPath('/config')
                 )
             ),
-
             'authentication' => new Tab(
                 array(
                     'name'      => 'auth',
@@ -208,9 +207,6 @@ class ConfigController extends BaseConfigController
     /**
      * Write changes to an authentication file
      *
-     * This uses the Zend_Config_Writer_Ini implementation for now, as the Preserving ini writer can't
-     * handle ordering
-     *
      * @param   array $config The configuration changes
      *
      * @return  bool True when persisting succeeded, otherwise false
@@ -218,13 +214,7 @@ class ConfigController extends BaseConfigController
      * @see     writeConfigFile()
      */
     private function writeAuthenticationFile($config) {
-        $writer = new Zend_Config_Writer_Ini(
+        return $this->writeConfigFile($config, 'authentication');
-            array(
-                'config' => new Zend_Config($config),
-                'filename' => IcingaConfig::app('authentication')->getConfigFile()
-            )
-        );
-        return $this->writeConfigFile($config, 'authentication', $writer);
     }
 
     /**

library/Icinga/Config/IniEditor.php

@@ -47,14 +47,47 @@ class IniEditor
      */
     private $nestSeparator = '.';
 
+    /**
+     * The indentation level of the comments
+     *
+     * @var string
+     */
+    private $commentIndentation;
+
+    /**
+     * The indentation level of the values
+     *
+     * @var string
+     */
+    private $valueIndentation;
+
+    /**
+     * The number of new lines between sections
+     *
+     * @var number
+     */
+    private $sectionSeparators;
+
     /**
      * Create a new IniEditor
      *
-     * @param string $content  The content of the ini as string
+     * @param string $content   The content of the ini as string
+     * @param array  $options   Optional formatting options used when changing the ini file
+     *                          * valueIndentation:     The indentation level of the values
+     *                          * commentIndentation:   The indentation level of the comments
+     *                          * sectionSeparators:    The amount of newlines between sections
      */
-    public function __construct($content)
+    public function __construct(
-    {
+        $content,
-        $this->text = explode("\n", $content);
+        array $options = array()
+    ) {
+        $this->text = explode(PHP_EOL, $content);
+        $this->valueIndentation = array_key_exists('valueIndentation', $options)
+            ? $options['valueIndentation'] : 19;
+        $this->commentIndentation = array_key_exists('commentIndentation', $options)
+            ? $options['commentIndentation'] : 43;
+        $this->sectionSeparators = array_key_exists('sectionSeparators', $options)
+            ? $options['sectionSeparators'] : 2;
     }
 
     /**
@@ -103,6 +136,9 @@ class IniEditor
             if (isset($section)) {
                 $this->updateLine($line, $this->formatKeyValuePair($key, $value));
             } else {
+                /*
+                 * Move into new section to avoid ambiguous configurations
+                 */
                 $section = $key[0];
                 unset($key[0]);
                 $this->deleteLine($line);
@@ -180,6 +216,92 @@ class IniEditor
         }
     }
 
+    /**
+     * Refresh the section order of the ini file
+     *
+     * @param array $order  An array containing the section names in the new order
+     *                      Example: array(0 => 'FirstSection', 1 => 'SecondSection')
+     */
+    public function refreshSectionOrder(array $order)
+    {
+        $sections = $this->createSectionMap($this->text);
+        /*
+         * Move section-less properties to the start of the ordered text
+         */
+        $orderedText = array();
+        foreach ($sections['[section-less]'] as $line) {
+            array_push($orderedText, $line);
+        }
+        /*
+         * Reorder the sections
+         */
+        $len = count($order);
+        for ($i = 0; $i < $len; $i++) {
+            if (array_key_exists($i, $order)) {
+                /*
+                 * Append the lines of the section to the end of the
+                 * ordered text
+                 */
+                foreach ($sections[$order[$i]] as $line) {
+                    array_push($orderedText, $line);
+                }
+            }
+        }
+        $this->text = $orderedText;
+    }
+
+    /**
+     * Create a map of sections to lines of a given ini file
+     *
+     * @param array $text           The text split up in lines
+     *
+     * @return array $sectionMap    A map containing all sections as arrays of lines. The array of section-less
+     *                              lines will be available using they key '[section-less]' which is no valid
+     *                              section declaration because it contains brackets.
+     */
+    private function createSectionMap($text)
+    {
+        $sections = array('[section-less]' => array());
+        $section = '[section-less]';
+        $len = count($text);
+        for ($i = 0; $i < $len; $i++) {
+            if ($this->isSectionDeclaration($text[$i])) {
+                $newSection = $this->getSectionFromDeclaration($this->text[$i]);
+                $sections[$newSection] = array();
+
+                /*
+                 * Remove comments 'glued' to the new section from the old
+                 * section array and put them into the new section.
+                 */
+                $j = $i - 1;
+                $comments = array();
+                while ($j > 0 && $this->isComment($this->text[$j])) {
+                    array_push($comments, array_pop($sections[$section]));
+                    $j--;
+                }
+                $comments = array_reverse($comments);
+                foreach ($comments as $comment) {
+                    array_push($sections[$newSection], $comment);
+                }
+
+                $section = $newSection;
+            }
+            array_push($sections[$section], $this->text[$i]);
+        }
+        return $sections;
+    }
+
+    /**
+     * Extract the section name from a section declaration
+     *
+     * @param String $declaration    The section declaration
+     */
+    private function getSectionFromDeclaration($declaration)
+    {
+        $tmp = preg_split('/(\[|\]|:)/', $declaration);
+        return trim($tmp[1]);
+    }
+
     /**
      * Remove a section declaration
      *
@@ -215,7 +337,7 @@ class IniEditor
     public function getText()
     {
         $this->cleanUpWhitespaces();
-        return implode("\n", $this->text);
+        return implode(PHP_EOL, $this->text);
     }
 
     /**
@@ -223,7 +345,6 @@ class IniEditor
      */
     private function cleanUpWhitespaces()
     {
-
         $i = count($this->text) - 1;
         for (; $i > 0; $i--) {
             $line = $this->text[$i];
@@ -233,7 +354,7 @@ class IniEditor
                 /*
                  * Ignore comments that are glued to the section declaration
                  */
-                while ($i > 0 && preg_match('/^\s*;/', $line) === 1) {
+                while ($i > 0 && $this->isComment($line)) {
                     $i--;
                     $line = $this->text[$i];
                 }
@@ -246,10 +367,10 @@ class IniEditor
                     $line = $this->text[$i];
                 }
                 /*
-                 * Add a single whitespace
+                 * Refresh section separators
                  */
-                if ($i !== 0) {
+                if ($i !== 0 && $this->sectionSeparators > 0) {
-                    $this->insertAtLine($i + 1, '');
+                    $this->insertAtLine($i + 1, str_repeat(PHP_EOL, $this->sectionSeparators - 1));
                 }
             }
         }
@@ -275,10 +396,12 @@ class IniEditor
     private function updateLine($lineNr, $content)
     {
         $comment = $this->getComment($this->text[$lineNr]);
+        $comment = trim($comment);
         if (strlen($comment) > 0) {
-            $comment = ' ; ' . trim($comment);
+            $comment = ' ; ' . $comment;
+            $content = str_pad($content, $this->commentIndentation) . $comment;
         }
-        $this->text[$lineNr] = str_pad($content, 43) . $comment;
+        $this->text[$lineNr] = $content;
     }
 
     /**
@@ -320,7 +443,7 @@ class IniEditor
      */
     private function formatKeyValuePair(array $key, $value)
     {
-        return str_pad($this->formatKey($key), 19) . ' = ' . $this->formatValue($value);
+        return str_pad($this->formatKey($key), $this->valueIndentation) . ' = ' . $this->formatValue($value);
     }
 
     /**
@@ -355,7 +478,7 @@ class IniEditor
                  * ignore all comments 'glued' to the next section, to allow section
                  * comments in front of sections
                  */
-                while ($i > 0 && preg_match('/^\s*;/', $this->text[$i - 1]) === 1) {
+                while ($i > 0 && $this->isComment($this->text[$i - 1])) {
                     $i--;
                 }
                 return $i;
@@ -370,6 +493,14 @@ class IniEditor
         return $i;
     }
 
+    /**
+     * Check if the given line contains only a comment
+     */
+    private function isComment($line)
+    {
+        return preg_match('/^\s*;/', $line) === 1;
+    }
+
     /**
      * Check if the line contains the property declaration for a key
      *

library/Icinga/Config/PreservingIniWriter.php

@@ -39,6 +39,31 @@ use \Icinga\Config\IniEditor;
  */
 class PreservingIniWriter extends Zend_Config_Writer_FileAbstract
 {
+    /**
+     * Stores the options
+     *
+     * @var array
+     */
+    private $options;
+
+    /**
+     * Create a new PreservingIniWriter
+     *
+     * @param array $options    Contains the options that should be used for the ConfigWriter
+     *                          in an associative array. Supports all options of Zend_Config_Writer and additional
+     *                          options for setting the formatting for the internal IniEditor:
+     *                          * valueIndentation:     The indentation level of the values
+     *                          * commentIndentation:   The indentation level of the comments
+     *                          * sectionSeparators:    The amount of newlines between sections
+     *
+     * @link http://framework.zend.com/apidoc/1.12/files/Config.Writer.html#\Zend_Config_Writer
+     */
+    public function __construct(array $options = null)
+    {
+        $this->options = $options;
+        parent::__construct($options);
+    }
+
     /**
      * Render the Zend_Config into a config file string
      *
@@ -48,8 +73,9 @@ class PreservingIniWriter extends Zend_Config_Writer_FileAbstract
     {
         $oldconfig = new Zend_Config_Ini($this->_filename);
         $newconfig = $this->_config;
-        $editor = new IniEditor(file_get_contents($this->_filename));
+        $editor = new IniEditor(file_get_contents($this->_filename), $this->options);
         $this->diffConfigs($oldconfig, $newconfig, $editor);
+        $this->updateSectionOrder($newconfig, $editor);
         return $editor->getText();
     }
 
@@ -71,6 +97,23 @@ class PreservingIniWriter extends Zend_Config_Writer_FileAbstract
         $this->diffPropertyDeletions($oldconfig, $newconfig, $editor, $parents);
     }
 
+    /**
+     * Update the order of the sections in the ini file to match
+     * the order of the new config
+     */
+    private function updateSectionOrder(
+        Zend_Config $newconfig,
+        IniEditor $editor
+    ) {
+        $order = array();
+        foreach ($newconfig as $key => $value) {
+            if ($value instanceof Zend_Config) {
+                array_push($order, $key);
+            }
+        }
+        $editor->refreshSectionOrder($order);
+    }
+
     /**
      * Search for created and updated properties and use the editor to create or update these entries
      *

test/php/library/Icinga/Config/PreservingIniWriterTest.php

@@ -36,6 +36,7 @@ require_once('../../library/Icinga/Config/IniEditor.php');
 require_once('../../library/Icinga/Config/PreservingIniWriter.php');
 
 use Icinga\Config\PreservingIniWriter;
+use Zend_Config;
 
 class PreservingIniWriterTest extends \PHPUnit_Framework_TestCase {
 
@@ -86,10 +87,10 @@ Prop5="true"
 PropOne="overwritten"
 ;10      
 ';
-        $this->writeToTmp('orig',$ini);
+        $this->writeToTmp('orig', $ini);
 
         $emptyIni = " ";
-        $this->writeToTmp('empty',$emptyIni);
+        $this->writeToTmp('empty', $emptyIni);
 
         $editedIni =
 ';1
@@ -114,21 +115,21 @@ prop2="2"
 [nested : different]
 prop2="5"
 ';
-        $this->writeToTmp('edited',$editedIni);
+        $this->writeToTmp('edited', $editedIni);
     }
 
     /**
      * Write a string to a temporary file
      *
-     * @param $name The name of the temporary file
+     * @param string    $name     The name of the temporary file
-     * @param $content The content
+     * @param string    $content  The content
      */
-    private function writeToTmp($name,$content)
+    private function writeToTmp($name, $content)
     {
         $this->tmpfiles[$name] =
-            tempnam(dirname(__FILE__) . '/temp',$name);
+            tempnam(dirname(__FILE__) . '/temp', $name);
-        $file = fopen($this->tmpfiles[$name],'w');
+        $file = fopen($this->tmpfiles[$name], 'w');
-        fwrite($file,$content);
+        fwrite($file, $content);
         fflush($file);
         fclose($file);
     }
@@ -151,7 +152,7 @@ prop2="5"
     {
         $this->changeConfigAndWriteToFile('orig');
         $config = new \Zend_Config_Ini(
-            $this->tmpfiles['orig'],null,array('allowModifications' => true)
+            $this->tmpfiles['orig'], null, array('allowModifications' => true)
         );
         $this->checkConfigProperties($config);
         $this->checkConfigComments($this->tmpfiles['orig']);
@@ -164,7 +165,7 @@ prop2="5"
     {
         $this->changeConfigAndWriteToFile('empty');
         $config = new \Zend_Config_Ini(
-            $this->tmpfiles['empty'],null,array('allowModifications' => true)
+            $this->tmpfiles['empty'], null, array('allowModifications' => true)
         );
         $this->checkConfigProperties($config);
     }
@@ -176,17 +177,69 @@ prop2="5"
     {
         $original = $this->changeConfigAndWriteToFile('edited');
         $config = new \Zend_Config_Ini(
-            $this->tmpfiles['edited'],null,array('allowModifications' => true)
+            $this->tmpfiles['edited'], null, array('allowModifications' => true)
         );
         $this->checkConfigProperties($config);
         $this->checkConfigComments($this->tmpfiles['edited']);
     }
 
     /**
-     * Change the test config and write the changes to the temporary
+     * Test if the order of sections is correctly changed in the config.
-     * file $tmpFile
+     */
+    public function testSectionOrderChange()
+    {
+        $original = '
+;1
+
+[section2]
+;3
+
+;4
+[section3]
+;5
+
+;2
+[section1]
+property = "something" ; comment
+
+        ';
+        $this->writeToTmp('section-order',$original);
+        $config = new Zend_Config(
+            array(
+                'section1' => array(
+                    'property' => 'something'
+                ),
+                'section2' => array(),
+                'section3' => array()
+            )
+        );
+        $writer = new PreservingIniWriter(
+            array('config' => $config, 'filename' => $this->tmpfiles['section-order'])
+        );
+        $writer->write();
+        $changed = new \Zend_Config_Ini(
+            $this->tmpfiles['section-order'],
+            null,
+            array('allowModifications' => true)
+        );
+        $this->assertEquals($config->section1->property, $changed->section1->property);
+
+        /*
+         * IniWriter should move the sections, so that comments
+         * are now in the right order
+         */
+        $this->checkConfigComments(
+            $this->tmpfiles['section-order'],
+            5,
+            'Sections re-ordered correctly'
+        );
+    }
+
+    /**
+     * Change the test config, write the changes to the temporary
+     * file $tmpFile and save the path to the file in the array tmpfiles
      *
-     * @param $tmpFile
+     * @param string $tmpFile   The name that should be given to the temporary file
      */
     private function changeConfigAndWriteToFile($tmpFile)
     {
@@ -202,9 +255,11 @@ prop2="5"
     /**
      * Check if all comments are present
      *
-     * @param $file
+     * @param String    $file       The file to check
+     * @param Number    $count      The amount of comments that should be present
+     * @param String    $assertion  The assertion message that will be displayed on errors
      */
-    private function checkConfigComments($file)
+    private function checkConfigComments($file,$count = 10,$assertion = 'Comment unchanged')
     {
         $i = 0;
         foreach (explode("\n",file_get_contents($file)) as $line) {
@@ -212,86 +267,86 @@ prop2="5"
                 $i++;
                 $this->assertEquals(
                     $i,intval(substr($line,1)),
-                    'Comment unchanged'
+                    $assertion
                 );
             }
         }
-        $this->assertEquals(10,$i,'All comments exist');
+        $this->assertEquals($count, $i, 'All comments exist');
     }
 
     /**
      * Test if all configuration properties are set correctly
      *
-     * @param $config
+     * @param mixed $config     The configuration to check
      */
     private function checkConfigProperties($config)
     {
-        $this->assertEquals('val',$config->Trailing2,
+        $this->assertEquals('val', $config->Trailing2,
             'Section-less property updated.');
 
         $this->assertNull($config->trailing1,
             'Section-less property deleted.');
 
-        $this->assertEquals('value',$config->new,
+        $this->assertEquals('value', $config->new,
             'Section-less property created.');
 
-        $this->assertEquals('0',$config->arr->{0},
+        $this->assertEquals('0', $config->arr->{0},
             'Value persisted in array');
 
-        $this->assertEquals('update',$config->arr->{2},
+        $this->assertEquals('update', $config->arr->{2},
             'Value changed in array');
 
-        $this->assertEquals('arrvalue',$config->arr->{4},
+        $this->assertEquals('arrvalue', $config->arr->{4},
             'Value added to array');
 
-        $this->assertEquals('',$config->parent->propOne,
+        $this->assertEquals('', $config->parent->propOne,
             'Section property deleted.');
 
-        $this->assertEquals("2",$config->parent->propTwo,
+        $this->assertEquals("2", $config->parent->propTwo,
             'Section property numerical unchanged.');
 
-        $this->assertEquals('update',$config->parent->propThree,
+        $this->assertEquals('update', $config->parent->propThree,
             'Section property updated.');
 
-        $this->assertEquals("true",$config->parent->propFour,
+        $this->assertEquals("true", $config->parent->propFour,
             'Section property boolean unchanged.');
 
-        $this->assertEquals("1",$config->parent->new,
+        $this->assertEquals("1", $config->parent->new,
             'Section property numerical created.');
 
         $this->assertNull($config->parent->list->{0},
             'Section array deleted');
 
-        $this->assertEquals('new',$config->parent->list->{1},
+        $this->assertEquals('new', $config->parent->list->{1},
             'Section array changed.');
 
-        $this->assertEquals('changed',$config->parent->many->many->nests,
+        $this->assertEquals('changed', $config->parent->many->many->nests,
             'Change strongly nested value.');
 
-        $this->assertEquals('new',$config->parent->many->many->new,
+        $this->assertEquals('new', $config->parent->many->many->new,
             'Ccreate strongy nested value.');
 
-        $this->assertEquals('overwritten',$config->child->PropOne,
+        $this->assertEquals('overwritten', $config->child->PropOne,
             'Overridden inherited property unchanged.');
 
-        $this->assertEquals('somethingNew',$config->child->propTwo,
+        $this->assertEquals('somethingNew', $config->child->propTwo,
             'Inherited property changed.');
 
-        $this->assertEquals('test',$config->child->create,
+        $this->assertEquals('test', $config->child->create,
             'Non-inherited property created.');
 
-        $this->assertInstanceOf('Zend_Config',$config->newsection,
+        $this->assertInstanceOf('Zend_Config', $config->newsection,
             'New section created.');
 
         $extends = $config->getExtends();
-        $this->assertEquals('child',$extends['newsection'],
+        $this->assertEquals('child', $extends['newsection'],
             'New inheritance created.');
     }
 
     /**
-     * Change the content of a Zend_Config
+     * Change the content of a Zend_Config for testing purposes
      *
-     * @param Zend_Config $config
+     * @param Zend_Config $config   The configuration that should be changed
      */
     private function alterConfig(\Zend_Config $config)
     {
@@ -320,7 +375,7 @@ prop2="5"
         $config->newsection = array();
         $config->newsection->p1 = "prop";
         $config->newsection->P2 = "prop";
-        $config->setExtend('newsection','child');
+        $config->setExtend('newsection', 'child');
     }
 
     /**