Issues

ZF-5771: Wrong pointer position after unsetting data.

Issue Type: Bug Created: 2009-02-13T02:59:39.000+0000 Last Updated: 2009-03-14T14:39:27.000+0000 Status: Resolved Fix version(s): - 1.8.0 (30/Apr/09)

Reporter: Jan Pieper (jpieper) Assignee: Rob Allen (rob) Tags: - Zend_Config

Related issues: Attachments: - reduced_testcase.php

Description

<pre class="highlight">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
    }
}


<pre class="highlight">first
second
third


<pre class="highlight">first
third

Comments

Posted by Rob Allen (rob) on 2009-02-14T05:16:41.000+0000

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

I have no idea how to fix it!

Posted by Rob Allen (rob) on 2009-02-14T06:39:48.000+0000

Corrected file that shows reduced use-case

Posted by Rob Allen (rob) on 2009-02-14T06:45:03.000+0000

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.

Posted by Robin Skoglund (robinsk) on 2009-02-14T09:07:02.000+0000

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.

<pre class="highlight">
--- 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 <a href="http://framework.zend.com/issues/browse/ZF-5771">http://framework.zend.com/issues/browse/ZF-5771</a>
+     * @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++;
     }

Posted by Rob Allen (rob) on 2009-02-14T11:12:58.000+0000

Robin,

Your patch fixes the described problem but introduces another one:

<pre class="highlight">
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

Posted by Jan Pieper (jpieper) on 2009-02-14T14:17:37.000+0000

I think I´ve found a solution.

<pre class="highlight">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;
     }



<pre class="highlight">foreach ($config as $key => $value)
{
    echo $key . PHP_EOL;
    if ($key == 'first') {
        unset($config->$key);
    }
}

first
second
third


<pre class="highlight">unset($config->first);
foreach($config as $key=>$item) {
    echo $key . PHP_EOL;
}

second
third


<pre class="highlight">[jan@jason Zend]# phpunit ConfigTest.php
PHPUnit 3.3.13 by Sebastian Bergmann.

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

Time: 0 seconds

OK (23 tests, 58 assertions)

Posted by Rob Allen (rob) on 2009-02-14T14:47:06.000+0000

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

Posted by Rob Allen (rob) on 2009-02-14T14:54:20.000+0000

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

Posted by Robin Skoglund (robinsk) on 2009-02-14T15:55:03.000+0000

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.

Posted by Rob Allen (rob) on 2009-03-14T14:39:21.000+0000

Fixed on trunk - r14321.

Have you found an issue?

See the Overview section for more details.

Copyright

© 2006-2016 by Zend, a Rogue Wave Company. Made with by awesome contributors.

This website is built using zend-expressive and it runs on PHP 7.

Contacts