Zend Framework

Wrong pointer position after unsetting data.

Details

  • Type: Bug Bug
  • Status: Resolved Resolved
  • Priority: Critical Critical
  • Resolution: Fixed
  • Affects Version/s: 1.7.4
  • Fix Version/s: 1.8.0
  • Component/s: Zend_Config
  • Labels:
    None

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
    }
}
expected result
first
second
third
actual result
first
third
  1. reduced_testcase.php
    14/Feb/09 6:39 AM
    2 kB
    Rob Allen
  2. reduced_testcase2.php
    14/Feb/09 2:54 PM
    3 kB
    Rob Allen
  3. robinsk_testcase_and_patch.tar.gz
    14/Feb/09 3:55 PM
    4 kB
    Robin Skoglund
  4. unset_during_iteration_solution_1.php
    14/Feb/09 6:45 AM
    3 kB
    Rob Allen
  5. unset_during_iteration_solution_2.php
    14/Feb/09 2:47 PM
    3 kB
    Rob Allen

Activity

Hide
Rob Allen added a comment -

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

I have no idea how to fix it!

Show
Rob Allen added a comment - This is the most simple test code I could come up with to show the problem. I have no idea how to fix it!
Hide
Rob Allen added a comment -

Corrected file that shows reduced use-case

Show
Rob Allen added a comment - Corrected file that shows reduced use-case
Hide
Rob Allen added a comment -

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.

Show
Rob Allen added a comment - 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.
Hide
Robin Skoglund added a comment - - edited

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++;
     }
Show
Robin Skoglund added a comment - - edited 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++;
     }
Hide
Rob Allen added a comment -

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

Show
Rob Allen added a comment - 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
Hide
Jan Pieper added a comment -

I think I´ve found a solution.

Diff
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;
     }
My example
foreach ($config as $key => $value)
{
    echo $key . PHP_EOL;
    if ($key == 'first') {
        unset($config->$key);
    }
}

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

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

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

Time: 0 seconds

OK (23 tests, 58 assertions)
Show
Jan Pieper added a comment - I think I´ve found a solution.
Diff
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;
     }
My example
foreach ($config as $key => $value)
{
    echo $key . PHP_EOL;
    if ($key == 'first') {
        unset($config->$key);
    }
}

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

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

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

Time: 0 seconds

OK (23 tests, 58 assertions)
Hide
Rob Allen added a comment -

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

Show
Rob Allen added a comment - Same basic solution, but more comprehensive exercising of the code.
Hide
Rob Allen added a comment -

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

Show
Rob Allen added a comment - Updated the reduced testcase file to include the more robust set of exercises. File is reduced_testcase2.php.
Hide
Robin Skoglund added a comment -

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.

Show
Robin Skoglund added a comment - 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.
Hide
Rob Allen added a comment -

Fixed on trunk - r14321.

Show
Rob Allen added a comment - Fixed on trunk - r14321.

People

Vote (0)
Watch (1)

Dates

  • Created:
    Updated:
    Resolved: