Improve ArrayObject::exchangeArray() behaviour with objects and CoW references (see http://turl.ca/exarr ).

3 files changed, 185 insertions, 65 deletions

diff --git a/ext/spl/spl_array.c b/ext/spl/spl_array.c
index 5bd1ce7bf9..653a6a785a 100755
--- a/ext/spl/spl_array.c
+++ b/ext/spl/spl_array.c
@@ -892,6 +892,51 @@ static void spl_array_it_rewind(zend_object_iterator *iter TSRMLS_DC) /* {{{ */
 }
 /* }}} */
 
+/* {{{ spl_array_set_array */
+static void spl_array_set_array(zval *object, spl_array_object *intern, zval **array, long ar_flags, int just_array TSRMLS_DC) {
+
+	if (Z_TYPE_PP(array) == IS_ARRAY) {
+		SEPARATE_ZVAL_IF_NOT_REF(array);
+	}
+
+	if (Z_TYPE_PP(array) == IS_OBJECT && (Z_OBJ_HT_PP(array) == &spl_handler_ArrayObject || Z_OBJ_HT_PP(array) == &spl_handler_ArrayIterator)) {
+		zval_ptr_dtor(&intern->array);
+		if (just_array)	{
+			spl_array_object *other = (spl_array_object*)zend_object_store_get_object(*array TSRMLS_CC);
+			ar_flags = other->ar_flags & ~SPL_ARRAY_INT_MASK;
+		}		
+		ar_flags |= SPL_ARRAY_USE_OTHER;
+		intern->array = *array;
+	} else {
+		if (Z_TYPE_PP(array) != IS_OBJECT && Z_TYPE_PP(array) != IS_ARRAY) {
+			php_set_error_handling(EH_NORMAL, NULL TSRMLS_CC);
+			zend_throw_exception(spl_ce_InvalidArgumentException, "Passed variable is not an array or object, using empty array instead", 0 TSRMLS_CC);
+			return;
+		}
+		zval_ptr_dtor(&intern->array);
+		intern->array = *array;
+	}
+	if (object == *array) {
+		intern->ar_flags |= SPL_ARRAY_IS_SELF;
+		intern->ar_flags &= ~SPL_ARRAY_USE_OTHER;
+	} else {
+		intern->ar_flags &= ~SPL_ARRAY_IS_SELF;
+	}
+	intern->ar_flags |= ar_flags;
+	Z_ADDREF_P(intern->array);
+	if (Z_TYPE_PP(array) == IS_OBJECT) {
+		zend_object_get_properties_t handler = Z_OBJ_HANDLER_PP(array, get_properties);
+		if ((handler != std_object_handlers.get_properties && handler != spl_array_get_properties)
+		|| !spl_array_get_hash_table(intern, 0 TSRMLS_CC)) {
+			php_set_error_handling(EH_NORMAL, NULL TSRMLS_CC);
+			zend_throw_exception_ex(spl_ce_InvalidArgumentException, 0 TSRMLS_CC, "Overloaded object of type %s is not compatible with %s", Z_OBJCE_PP(array)->name, intern->std.ce->name);
+		}
+	}
+
+	spl_array_rewind(intern TSRMLS_CC);
+}
+/* }}} */
+
 /* iterator handler table */
 zend_object_iterator_funcs spl_array_it_funcs = {
 	spl_array_it_dtor,
@@ -949,10 +994,6 @@ SPL_METHOD(Array, __construct)
 		return;
 	}
 
-	if (Z_TYPE_PP(array) == IS_ARRAY) {
-		SEPARATE_ZVAL_IF_NOT_REF(array);
-	}
-
 	if (ZEND_NUM_ARGS() > 2) {
 		if (zend_lookup_class(class_name, class_name_len, &pce_get_iterator TSRMLS_CC) == FAILURE) {
 			zend_throw_exception(spl_ce_InvalidArgumentException, "A class that implements Iterator must be specified", 0 TSRMLS_CC);
@@ -964,43 +1005,7 @@ SPL_METHOD(Array, __construct)
 
 	ar_flags &= ~SPL_ARRAY_INT_MASK;
 
-	if (Z_TYPE_PP(array) == IS_OBJECT && (Z_OBJ_HT_PP(array) == &spl_handler_ArrayObject || Z_OBJ_HT_PP(array) == &spl_handler_ArrayIterator)) {
-		zval_ptr_dtor(&intern->array);
-		if (ZEND_NUM_ARGS() == 1)
-		{
-			spl_array_object *other = (spl_array_object*)zend_object_store_get_object(*array TSRMLS_CC);
-			ar_flags = other->ar_flags & ~SPL_ARRAY_INT_MASK;
-		}		
-		ar_flags |= SPL_ARRAY_USE_OTHER;
-		intern->array = *array;
-	} else {
-		if (Z_TYPE_PP(array) != IS_OBJECT && Z_TYPE_PP(array) != IS_ARRAY) {
-			php_set_error_handling(EH_NORMAL, NULL TSRMLS_CC);
-			zend_throw_exception(spl_ce_InvalidArgumentException, "Passed variable is not an array or object, using empty array instead", 0 TSRMLS_CC);
-			return;
-		}
-		zval_ptr_dtor(&intern->array);
-		intern->array = *array;
-	}
-	if (object == *array) {
-		intern->ar_flags |= SPL_ARRAY_IS_SELF;
-		intern->ar_flags &= ~SPL_ARRAY_USE_OTHER;
-	} else {
-		intern->ar_flags &= ~SPL_ARRAY_IS_SELF;
-	}
-	intern->ar_flags |= ar_flags;
-	Z_ADDREF_P(intern->array);
-	if (Z_TYPE_PP(array) == IS_OBJECT) {
-		zend_object_get_properties_t handler = Z_OBJ_HANDLER_PP(array, get_properties);
-		if ((handler != std_object_handlers.get_properties && handler != spl_array_get_properties)
-		|| !spl_array_get_hash_table(intern, 0 TSRMLS_CC)) {
-			php_set_error_handling(EH_NORMAL, NULL TSRMLS_CC);
-			zend_throw_exception_ex(spl_ce_InvalidArgumentException, 0 TSRMLS_CC, "Overloaded object of type %s is not compatible with %s", Z_OBJCE_PP(array)->name, intern->std.ce->name);
-			return;
-		}
-	}
-
-	spl_array_rewind(intern TSRMLS_CC);
+	spl_array_set_array(object, intern, array, ar_flags, ZEND_NUM_ARGS() == 1 TSRMLS_CC);
 
 	php_set_error_handling(EH_NORMAL, NULL TSRMLS_CC);
 }
@@ -1081,31 +1086,9 @@ SPL_METHOD(Array, exchangeArray)
 	if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "Z", &array) == FAILURE) {
 		return;
 	}
-	if (Z_TYPE_PP(array) == IS_OBJECT && intern == (spl_array_object*)zend_object_store_get_object(object TSRMLS_CC)) {
-		zval_ptr_dtor(&intern->array);
-		array = &object;
-		intern->array = object;
-	} else if (Z_TYPE_PP(array) == IS_OBJECT && (Z_OBJ_HT_PP(array) == &spl_handler_ArrayObject || Z_OBJ_HT_PP(array) == &spl_handler_ArrayIterator)) {
-		spl_array_object *other  = (spl_array_object*)zend_object_store_get_object(*array TSRMLS_CC);
-		zval_ptr_dtor(&intern->array);
-		intern->array = other->array;
-	} else {
-		if (Z_TYPE_PP(array) != IS_OBJECT && !HASH_OF(*array)) {
-			zend_throw_exception(spl_ce_InvalidArgumentException, "Passed variable is not an array or object, using empty array instead", 0 TSRMLS_CC);
-			return;
-		}
-		zval_ptr_dtor(&intern->array);
-		intern->array = *array;
-		intern->ar_flags &= ~SPL_ARRAY_USE_OTHER;
-	}
-	if (object == *array) {
-		intern->ar_flags |= SPL_ARRAY_IS_SELF;
-	} else {
-		intern->ar_flags &= ~SPL_ARRAY_IS_SELF;
-	}
-	Z_ADDREF_P(intern->array);
 
-	spl_array_rewind(intern TSRMLS_CC);
+	spl_array_set_array(object, intern, array, 0L, 1 TSRMLS_CC);
+
 }
 /* }}} */
 
diff --git a/ext/spl/tests/arrayObject_exchangeArray_basic1.phpt b/ext/spl/tests/arrayObject_exchangeArray_basic1.phpt
new file mode 100644
index 0000000000..988f103698
--- /dev/null
+++ b/ext/spl/tests/arrayObject_exchangeArray_basic1.phpt
@@ -0,0 +1,40 @@
+--TEST--
+SPL: ArrayObject::exchangeArray() and copy-on-write references
+--FILE--
+<?php
+$ao = new ArrayObject();
+$swapIn = array();
+$cowRef = $swapIn; // create a copy-on-write ref to $swapIn
+$ao->exchangeArray($swapIn);
+
+$ao['a'] = 'adding element to $ao';
+$swapIn['b'] = 'adding element to $swapIn';
+$ao['c'] = 'adding another element to $ao';
+
+echo "\n--> swapIn:  ";
+var_dump($swapIn);
+
+echo "\n--> cowRef:  ";
+var_dump($cowRef);
+
+echo "\n--> ao:  ";
+var_dump($ao);
+?>
+--EXPECTF--
+--> swapIn:  array(1) {
+  ["b"]=>
+  string(25) "adding element to $swapIn"
+}
+
+--> cowRef:  array(0) {
+}
+
+--> ao:  object(ArrayObject)#%d (1) {
+  ["storage":"ArrayObject":private]=>
+  array(2) {
+    ["a"]=>
+    string(21) "adding element to $ao"
+    ["c"]=>
+    string(29) "adding another element to $ao"
+  }
+}
diff --git a/ext/spl/tests/arrayObject_exchangeArray_basic2.phpt b/ext/spl/tests/arrayObject_exchangeArray_basic2.phpt
new file mode 100644
index 0000000000..c7f1b3a7af
--- /dev/null
+++ b/ext/spl/tests/arrayObject_exchangeArray_basic2.phpt
@@ -0,0 +1,97 @@
+--TEST--
+SPL: ArrayObject::exchangeArray() with various object arguments
+--FILE--
+<?php
+echo "--> exchangeArray(array):\n";
+$ao = new ArrayObject();
+$ao->exchangeArray(array('key'=>'original'));
+var_dump($ao['key']);
+var_dump($ao);
+
+echo "\n--> exchangeArray(normal object):\n";
+$obj = new stdClass;
+$obj->key = 'normal object prop';
+$ao->exchangeArray($obj);
+var_dump($ao['key']);
+var_dump($ao);
+
+echo "\n--> exchangeArray(ArrayObject):\n";
+$obj = new ArrayObject(array('key'=>'ArrayObject element'));
+$ao->exchangeArray($obj);
+var_dump($ao['key']);
+var_dump($ao);
+
+echo "\n--> exchangeArray(ArrayIterator):\n";
+$obj = new ArrayIterator(array('key'=>'ArrayIterator element'));
+$ao->exchangeArray($obj);
+var_dump($ao['key']);
+var_dump($ao);
+
+echo "\n--> exchangeArray(nested ArrayObject):\n";
+$obj = new ArrayObject(new ArrayObject(array('key'=>'nested ArrayObject element')));
+$ao->exchangeArray($obj);
+var_dump($ao['key']);
+var_dump($ao);
+?>
+--EXPECTF--
+--> exchangeArray(array):
+string(8) "original"
+object(ArrayObject)#%d (1) {
+  ["storage":"ArrayObject":private]=>
+  array(1) {
+    ["key"]=>
+    string(8) "original"
+  }
+}
+
+--> exchangeArray(normal object):
+string(18) "normal object prop"
+object(ArrayObject)#%d (1) {
+  ["storage":"ArrayObject":private]=>
+  object(stdClass)#%d (1) {
+    ["key"]=>
+    string(18) "normal object prop"
+  }
+}
+
+--> exchangeArray(ArrayObject):
+string(19) "ArrayObject element"
+object(ArrayObject)#%d (1) {
+  ["storage":"ArrayObject":private]=>
+  object(ArrayObject)#%d (1) {
+    ["storage":"ArrayObject":private]=>
+    array(1) {
+      ["key"]=>
+      string(19) "ArrayObject element"
+    }
+  }
+}
+
+--> exchangeArray(ArrayIterator):
+string(21) "ArrayIterator element"
+object(ArrayObject)#%d (1) {
+  ["storage":"ArrayObject":private]=>
+  object(ArrayIterator)#%d (1) {
+    ["storage":"ArrayIterator":private]=>
+    array(1) {
+      ["key"]=>
+      string(21) "ArrayIterator element"
+    }
+  }
+}
+
+--> exchangeArray(nested ArrayObject):
+string(26) "nested ArrayObject element"
+object(ArrayObject)#%d (1) {
+  ["storage":"ArrayObject":private]=>
+  object(ArrayObject)#%d (1) {
+    ["storage":"ArrayObject":private]=>
+    object(ArrayObject)#%d (1) {
+      ["storage":"ArrayObject":private]=>
+      array(1) {
+        ["key"]=>
+        string(26) "nested ArrayObject element"
+      }
+    }
+  }
+}
\ No newline at end of file