From d750fbf648256c7c631f51ffdbf67d7c18b0114e Mon Sep 17 00:00:00 2001 From: Paul Yang Date: Wed, 23 Jan 2019 12:44:20 -0800 Subject: [PATCH] Fix more issues for reference values (#5613) * Fix more issues for reference values * Revert change in gdb test * Add more tests --- php/ext/google/protobuf/storage.c | 16 +++-- php/ext/google/protobuf/type_check.c | 5 ++ php/tests/array_test.php | 44 +++++++----- php/tests/generated_class_test.php | 103 +++++++++++++++++++++++++++ php/tests/map_field_test.php | 37 ++++++++++ php/tests/proto/test.proto | 1 - 6 files changed, 181 insertions(+), 25 deletions(-) diff --git a/php/ext/google/protobuf/storage.c b/php/ext/google/protobuf/storage.c index 610d3eb75e..0c5b68c578 100644 --- a/php/ext/google/protobuf/storage.c +++ b/php/ext/google/protobuf/storage.c @@ -161,6 +161,11 @@ bool native_slot_set(upb_fieldtype_t type, const zend_class_entry* klass, bool native_slot_set_by_array(upb_fieldtype_t type, const zend_class_entry* klass, void* memory, zval* value TSRMLS_DC) { +#if PHP_MAJOR_VERSION >= 7 + if (Z_ISREF_P(value)) { + ZVAL_DEREF(value); + } +#endif switch (type) { case UPB_TYPE_STRING: case UPB_TYPE_BYTES: { @@ -186,12 +191,6 @@ bool native_slot_set_by_array(upb_fieldtype_t type, break; } case UPB_TYPE_MESSAGE: { -#if PHP_MAJOR_VERSION >= 7 - if (Z_ISREF_P(value)) { - ZVAL_DEREF(value); - } -#endif - if (Z_TYPE_P(value) != IS_OBJECT) { zend_error(E_USER_ERROR, "Given value is not message."); return false; @@ -219,6 +218,11 @@ bool native_slot_set_by_array(upb_fieldtype_t type, bool native_slot_set_by_map(upb_fieldtype_t type, const zend_class_entry* klass, void* memory, zval* value TSRMLS_DC) { +#if PHP_MAJOR_VERSION >= 7 + if (Z_ISREF_P(value)) { + ZVAL_DEREF(value); + } +#endif switch (type) { case UPB_TYPE_STRING: case UPB_TYPE_BYTES: { diff --git a/php/ext/google/protobuf/type_check.c b/php/ext/google/protobuf/type_check.c index 31dc44493d..d8a9245603 100644 --- a/php/ext/google/protobuf/type_check.c +++ b/php/ext/google/protobuf/type_check.c @@ -373,6 +373,11 @@ bool protobuf_convert_to_bool(zval* from, int8_t* to) { } bool protobuf_convert_to_string(zval* from) { +#if PHP_MAJOR_VERSION >= 7 + if (Z_ISREF_P(from)) { + ZVAL_DEREF(from); + } +#endif TSRMLS_FETCH(); switch (Z_TYPE_P(from)) { case IS_STRING: { diff --git a/php/tests/array_test.php b/php/tests/array_test.php index d47a107765..b251404083 100644 --- a/php/tests/array_test.php +++ b/php/tests/array_test.php @@ -533,31 +533,39 @@ class RepeatedFieldTest extends \PHPUnit\Framework\TestCase # Test reference in array ######################################################### - public function testArrayElementIsReference() + public function testArrayElementIsReferenceInSetters() { + // Bool elements + $values = [true]; + array_walk($values, function (&$value) {}); $m = new TestMessage(); - $subs = [1, 2]; - - foreach ($subs as &$sub) { - $sub = new Sub(['a' => $sub]); - } + $m->setRepeatedBool($values); - $m->setRepeatedMessage($subs); - } + // Int32 elements + $values = [1]; + array_walk($values, function (&$value) {}); + $m = new TestMessage(); + $m->setRepeatedInt32($values); - public function testArrayIsReference() - { - $keys = [['repeated_message' => [['a' => 1]]]]; + // Double elements + $values = [1.0]; + array_walk($values, function (&$value) {}); + $m = new TestMessage(); + $m->setRepeatedDouble($values); - foreach ($keys as &$key) { - foreach ($key['repeated_message'] as &$element) { - $element = new Sub($element); - } - $key = new TestMessage($key); - } + // String elements + $values = ['a']; + array_walk($values, function (&$value) {}); + $m = new TestMessage(); + $m->setRepeatedString($values); + // Message elements $m = new TestMessage(); - $m->setRepeatedDeep($keys); + $subs = [1, 2]; + foreach ($subs as &$sub) { + $sub = new Sub(['a' => $sub]); + } + $m->setRepeatedMessage($subs); } ######################################################### diff --git a/php/tests/generated_class_test.php b/php/tests/generated_class_test.php index 83deaba163..93b7b29bb4 100644 --- a/php/tests/generated_class_test.php +++ b/php/tests/generated_class_test.php @@ -1375,6 +1375,78 @@ class GeneratedClassTest extends TestBase $this->assertTrue(true); } + public function testReferenceInArrayConstructor() + { + $keys = [[ + 'optional_bool' => true, + 'repeated_bool' => [true], + 'map_bool_bool' => [true => true], + 'optional_double' => 1.0, + 'repeated_double' => [1.0], + 'map_int32_double' => [1 => 1.0], + 'optional_int32' => 1, + 'repeated_int32' => [1], + 'map_int32_int32' => [1 => 1], + 'optional_string' => 'a', + 'repeated_string' => ['a'], + 'map_string_string' => ['a' => 'a'], + 'optional_message' => ['a' => 1], + 'repeated_message' => [['a' => 1]], + 'map_int32_message' => [1 => ['a' => 1]], + ]]; + + foreach ($keys as &$key) { + foreach ($key as $id => &$value) { + if ($id === 'repeated_bool') { + foreach ($value as &$element) { + } + } + if ($id === 'map_bool_bool') { + foreach ($value as $mapKey => &$element) { + } + } + if ($id === 'repeated_double') { + foreach ($value as &$element) { + } + } + if ($id === 'map_int32_double') { + foreach ($value as $mapKey => &$element) { + } + } + if ($id === 'repeated_int32') { + foreach ($value as &$element) { + } + } + if ($id === 'map_int32_int32') { + foreach ($value as $mapKey => &$element) { + } + } + if ($id === 'repeated_string') { + foreach ($value as &$element) { + } + } + if ($id === 'map_string_string') { + foreach ($value as $mapKey => &$element) { + } + } + if ($id === 'optional_message') { + $value = new Sub($value); + } + if ($id === 'repeated_message') { + foreach ($value as &$element) { + $element = new Sub($element); + } + } + if ($id === 'map_int32_message') { + foreach ($value as $mapKey => &$element) { + $element = new Sub($element); + } + } + } + $key = new TestMessage($key); + } + } + ######################################################### # Test message equals. ######################################################### @@ -1387,4 +1459,35 @@ class GeneratedClassTest extends TestBase TestUtil::setTestMessage($n); $this->assertEquals($m, $n); } + + ######################################################### + # Test reference of value + ######################################################### + + public function testValueIsReference() + { + // Bool element + $values = [true]; + array_walk($values, function (&$value) {}); + $m = new TestMessage(); + $m->setOptionalBool($values[0]); + + // Int32 element + $values = [1]; + array_walk($values, function (&$value) {}); + $m = new TestMessage(); + $m->setOptionalInt32($values[0]); + + // Double element + $values = [1.0]; + array_walk($values, function (&$value) {}); + $m = new TestMessage(); + $m->setOptionalDouble($values[0]); + + // String element + $values = ['a']; + array_walk($values, function (&$value) {}); + $m = new TestMessage(); + $m->setOptionalString($values[0]); + } } diff --git a/php/tests/map_field_test.php b/php/tests/map_field_test.php index 0260879328..577be681bf 100644 --- a/php/tests/map_field_test.php +++ b/php/tests/map_field_test.php @@ -442,6 +442,43 @@ class MapFieldTest extends \PHPUnit\Framework\TestCase { $this->assertSame(3, $i); } + ######################################################### + # Test reference in map + ######################################################### + + public function testMapElementIsReference() + { + // Bool elements + $values = [true => true]; + array_walk($values, function (&$value) {}); + $m = new TestMessage(); + $m->setMapBoolBool($values); + + // Int32 elements + $values = [1 => 1]; + array_walk($values, function (&$value) {}); + $m = new TestMessage(); + $m->setMapInt32Int32($values); + + // Double elements + $values = [1 => 1.0]; + array_walk($values, function (&$value) {}); + $m = new TestMessage(); + $m->setMapInt32Double($values); + + // String elements + $values = ['a' => 'a']; + array_walk($values, function (&$value) {}); + $m = new TestMessage(); + $m->setMapStringString($values); + + // Message elements + $values = [1 => new Sub()]; + array_walk($values, function (&$value) {}); + $m = new TestMessage(); + $m->setMapInt32Message($values); + } + ######################################################### # Test memory leak ######################################################### diff --git a/php/tests/proto/test.proto b/php/tests/proto/test.proto index ca39ea46a4..e610c581b5 100644 --- a/php/tests/proto/test.proto +++ b/php/tests/proto/test.proto @@ -31,7 +31,6 @@ message TestMessage { Sub optional_message = 17; bar.TestInclude optional_included_message = 18; TestMessage recursive = 19; - repeated TestMessage repeated_deep = 20; // Repeated repeated int32 repeated_int32 = 31;