ZF-5771: Wrong pointer position after unsetting data.

Description

require_once 'Zend/Config.php';
$config = new Zend_Config(array(
    'first'  => array(1),
    'second' => array(2),
    'third'  => array(3)
), true);

foreach ($config as $key => $value)
{
    echo $key . PHP_EOL;
    if ($key == 'first') {
        unset($config->$key); // uses magic Zend_Config::__unset() method
    }
}
first
second
third
first
third

Comments

This is the most simple test code I could come up with to show the problem.

I have no idea how to fix it!

Corrected file that shows reduced use-case

Solution to the reduced test case - no optimisation. Created with help from Freeaqingme and Jon Whitcraft on #zftalk.dev.

Obvious optimisation is to store the deleted keys in and _deleted array to save iterating over _data.

This patch solves the problem as currently described in the issue, and all Zend/Config* unit tests run fine with this patch.

However, I still think the patch needs more testing before applying it to the trunk.


--- library/Zend/Config.php (revision 14089)
+++ library/Zend/Config.php (working copy)
@@ -56,6 +56,13 @@
      */
     protected $_data;
 
+    /**
+     * Flag to use when unsetting during config iteration
+     *
+     * @link http://framework.zend.com/issues/browse/ZF-5771
+     * @var boolean
+     */
+    protected $_iterationUnsetFlag;
 
     /**
      * Contains which config file sections were loaded. This is null
@@ -223,6 +230,7 @@
         if ($this->_allowModifications) {
             unset($this->_data[$name]);
             $this->_count = count($this->_data);
+            $this->_iterationUnsetFlag = true;
         } else {
             /** @see Zend_Config_Exception */
             require_once 'Zend/Config/Exception.php';
@@ -267,6 +275,10 @@
      */
     public function next()
     {
+        if ($this->_iterationUnsetFlag) {
+            $this->_iterationUnsetFlag = false;
+            return;
+        }
         next($this->_data);
         $this->_index++;
     }

Robin,

Your patch fixes the described problem but introduces another one:


unset($config->first);
foreach($config as $key=>$item) {
    echo $key . PHP_EOL;
}

The first next() doesn't get called, so the output becomes: second second third

rather than the expected second third

I think I´ve found a solution.

Index: Config.php
===================================================================
--- Config.php  (revision 14089)
+++ Config.php  (working copy)
@@ -84,6 +84,13 @@
     protected $_loadFileErrorStr = null;

     /**
+     * Contains the index keys of the configuration data array.
+     *
+     * @var array
+     */
+    protected $_indexes = array();
+
+    /**
      * Zend_Config provides a property based interface to
      * an array. The data are read-only unless $allowModifications
      * is set to true on construction.
@@ -100,6 +107,7 @@
         $this->_allowModifications = (boolean) $allowModifications;
         $this->_loadedSection = null;
         $this->_index = 0;
+        $this->_indexes = array_keys($array);
         $this->_data = array();
         foreach ($array as $key => $value) {
             if (is_array($value)) {
@@ -155,6 +163,9 @@
             } else {
                 $this->_data[$name] = $value;
             }
+            if (!in_array($name, $this->_indexes)) {
+                $this->_indexes[] = $name;
+            }
             $this->_count = count($this->_data);
         } else {
             /** @see Zend_Config_Exception */
@@ -222,7 +233,10 @@
     {
         if ($this->_allowModifications) {
             unset($this->_data[$name]);
+            unset($this->_indexes[array_search($name, $this->_indexes)]);
+            $this->_indexes = array_values($this->_indexes);
             $this->_count = count($this->_data);
+            --$this->_index;
         } else {
             /** @see Zend_Config_Exception */
             require_once 'Zend/Config/Exception.php';
@@ -248,7 +262,10 @@
      */
     public function current()
     {
-        return current($this->_data);
+        if (!array_key_exists($key = $this->key(), $this->_data)) {
+            return false;
+        }
+        return $this->_data[$key];
     }

     /**
@@ -258,7 +275,7 @@
      */
     public function key()
     {
-        return key($this->_data);
+        return $this->_indexes[$this->_index];
     }

     /**
@@ -267,8 +284,7 @@
      */
     public function next()
     {
-        next($this->_data);
-        $this->_index++;
+        ++$this->_index;
     }

     /**
@@ -277,7 +293,6 @@
      */
     public function rewind()
     {
-        reset($this->_data);
         $this->_index = 0;
     }
foreach ($config as $key => $value)
{
    echo $key . PHP_EOL;
    if ($key == 'first') {
        unset($config->$key);
    }
}

first
second
third
unset($config->first);
foreach($config as $key=>$item) {
    echo $key . PHP_EOL;
}

second
third
[jan@jason Zend]# phpunit ConfigTest.php
PHPUnit 3.3.13 by Sebastian Bergmann.

.......................

Time: 0 seconds

OK (23 tests, 58 assertions)

Same basic solution, but more comprehensive exercising of the code.

Updated the reduced testcase file to include the more robust set of exercises. File is reduced_testcase2.php.

I attached a file 'robinsk_testcase_and_patch.tar.gz', which includes a modified version of Zend_Config with 6 runnable tests (not unit tests). The patch is also in the archive.

Summary of patch: Set $this->_iterationUnsetFlag = true in the __unset() method. If the flag is true, next() will not advance the internal pointer, but just flip the flag to false. Other iterator methods will also set the flag to false, so the internal state of the pointer/flag is not corrupted. This is way more efficient than maintaining deleted keys or using string operations.

To run tests: * Download robinsk_testcase_and_patch.tar.gz * $ tar -zxvf robinsk_testcase_and_patch.tar.gz * $ php test1.php * $ php test2.php * ... * $ php test6.php

Each test outputs what it tests, and what the expected and actual results are.

Fixed on trunk - r14321.