PHP: Exclude repeated and map fields from normalization in constructor (#5723)

* Exclude repeated and map fields from normalization

* Remove erroneous comments

* Remove unnecessary check for map type

* Add support for repeated/map fields, add tests

* Fix wrapper message in repeated/map fields in array constructor

* Address PR comments

* Removed unused code

* Update docs

php/ext/google/protobuf/message.c

@@ -294,6 +294,57 @@ void build_class_from_descriptor(
 // PHP Methods
 // -----------------------------------------------------------------------------
 
+static bool is_wrapper_msg(const upb_msgdef* m) {
+  upb_wellknowntype_t type = upb_msgdef_wellknowntype(m);
+  return type >= UPB_WELLKNOWN_DOUBLEVALUE &&
+         type <= UPB_WELLKNOWN_BOOLVALUE;
+}
+
+static void append_wrapper_message(
+    zend_class_entry* subklass, RepeatedField* intern, zval* value TSRMLS_DC) {
+  MessageHeader* submsg;
+  const upb_fielddef* field;
+#if PHP_MAJOR_VERSION < 7
+  zval* val = NULL;
+  MAKE_STD_ZVAL(val);
+  ZVAL_OBJ(val, subklass->create_object(subklass TSRMLS_CC));
+  repeated_field_push_native(intern, &val);
+  submsg = UNBOX(MessageHeader, val);
+#else
+  zend_object* obj = subklass->create_object(subklass TSRMLS_CC);
+  repeated_field_push_native(intern, &obj);
+  submsg = (MessageHeader*)((char*)obj - XtOffsetOf(MessageHeader, std));
+#endif
+  custom_data_init(subklass, submsg PHP_PROTO_TSRMLS_CC);
+
+  field = upb_msgdef_itof(submsg->descriptor->msgdef, 1);
+  layout_set(submsg->descriptor->layout, submsg, field, value TSRMLS_CC);
+}
+
+static void set_wrapper_message_as_map_value(
+    zend_class_entry* subklass, zval* map, zval* key,  zval* value TSRMLS_DC) {
+  MessageHeader* submsg;
+  const upb_fielddef* field;
+#if PHP_MAJOR_VERSION < 7
+  zval* val = NULL;
+  MAKE_STD_ZVAL(val);
+  ZVAL_OBJ(val, subklass->create_object(subklass TSRMLS_CC));
+  map_field_handlers->write_dimension(
+      map, key, val TSRMLS_CC);
+  submsg = UNBOX(MessageHeader, val);
+#else
+  zval val;
+  zend_object* obj = subklass->create_object(subklass TSRMLS_CC);
+  ZVAL_OBJ(&val, obj);
+  map_field_handlers->write_dimension(map, key, &val TSRMLS_CC);
+  submsg = (MessageHeader*)((char*)obj - XtOffsetOf(MessageHeader, std));
+#endif
+  custom_data_init(subklass, submsg PHP_PROTO_TSRMLS_CC);
+
+  field = upb_msgdef_itof(submsg->descriptor->msgdef, 1);
+  layout_set(submsg->descriptor->layout, submsg, field, value TSRMLS_CC);
+}
+
 void Message_construct(zval* msg, zval* array_wrapper) {
   TSRMLS_FETCH();
   zend_class_entry* ce = Z_OBJCE_P(msg);
@@ -336,14 +387,38 @@ void Message_construct(zval* msg, zval* array_wrapper) {
       HashPosition subpointer;
       zval subkey;
       void* memory;
+      bool is_wrapper = false;
+      zend_class_entry* subklass = NULL;
+      const upb_msgdef* mapentry = upb_fielddef_msgsubdef(field);
+      const upb_fielddef *value_field = upb_msgdef_itof(mapentry, 2);
+
+      if (upb_fielddef_issubmsg(value_field)) {
+        const upb_msgdef* submsgdef = upb_fielddef_msgsubdef(value_field);
+        upb_wellknowntype_t type = upb_msgdef_wellknowntype(submsgdef);
+        is_wrapper = is_wrapper_msg(submsgdef);
+
+        if (is_wrapper) {
+          PHP_PROTO_HASHTABLE_VALUE subdesc_php = get_def_obj(submsgdef);
+          Descriptor* subdesc = UNBOX_HASHTABLE_VALUE(Descriptor, subdesc_php);
+          subklass = subdesc->klass;
+        }
+      }
+
       for (zend_hash_internal_pointer_reset_ex(subtable, &subpointer);
            php_proto_zend_hash_get_current_data_ex(subtable, (void**)&memory,
                                                    &subpointer) == SUCCESS;
            zend_hash_move_forward_ex(subtable, &subpointer)) {
         zend_hash_get_current_key_zval_ex(subtable, &subkey, &subpointer);
-        map_field_handlers->write_dimension(
-            submap, &subkey,
-            CACHED_PTR_TO_ZVAL_PTR((CACHED_VALUE*)memory) TSRMLS_CC);
+        if (is_wrapper &&
+            Z_TYPE_P(CACHED_PTR_TO_ZVAL_PTR((CACHED_VALUE*)memory)) != IS_OBJECT) {
+          set_wrapper_message_as_map_value(
+              subklass, submap, &subkey,
+              CACHED_PTR_TO_ZVAL_PTR((CACHED_VALUE*)memory) TSRMLS_CC);
+        } else {
+          map_field_handlers->write_dimension(
+              submap, &subkey,
+              CACHED_PTR_TO_ZVAL_PTR((CACHED_VALUE*)memory) TSRMLS_CC);
+        }
         zval_dtor(&subkey);
       }
     } else if (upb_fielddef_isseq(field)) {
@@ -354,13 +429,36 @@ void Message_construct(zval* msg, zval* array_wrapper) {
           CACHED_PTR_TO_ZVAL_PTR((CACHED_VALUE*)value));
       HashPosition subpointer;
       void* memory;
+      bool is_wrapper = false;
+      zend_class_entry* subklass = NULL;
+
+      if (upb_fielddef_issubmsg(field)) {
+        const upb_msgdef* submsgdef = upb_fielddef_msgsubdef(field);
+        upb_wellknowntype_t type = upb_msgdef_wellknowntype(submsgdef);
+        is_wrapper = is_wrapper_msg(submsgdef);
+
+        if (is_wrapper) {
+          PHP_PROTO_HASHTABLE_VALUE subdesc_php = get_def_obj(submsgdef);
+          Descriptor* subdesc = UNBOX_HASHTABLE_VALUE(Descriptor, subdesc_php);
+          subklass = subdesc->klass;
+        }
+      }
+
       for (zend_hash_internal_pointer_reset_ex(subtable, &subpointer);
            php_proto_zend_hash_get_current_data_ex(subtable, (void**)&memory,
                                                    &subpointer) == SUCCESS;
            zend_hash_move_forward_ex(subtable, &subpointer)) {
-        repeated_field_handlers->write_dimension(
-            subarray, NULL,
-            CACHED_PTR_TO_ZVAL_PTR((CACHED_VALUE*)memory) TSRMLS_CC);
+        if (is_wrapper &&
+            Z_TYPE_P(CACHED_PTR_TO_ZVAL_PTR((CACHED_VALUE*)memory)) != IS_OBJECT) {
+          RepeatedField* intern = UNBOX(RepeatedField, subarray);
+          append_wrapper_message(
+              subklass, intern,
+              CACHED_PTR_TO_ZVAL_PTR((CACHED_VALUE*)memory) TSRMLS_CC);
+        } else {
+          repeated_field_handlers->write_dimension(
+              subarray, NULL,
+              CACHED_PTR_TO_ZVAL_PTR((CACHED_VALUE*)memory) TSRMLS_CC);
+        }
       }
     } else if (upb_fielddef_issubmsg(field)) {
       const upb_msgdef* submsgdef = upb_fielddef_msgsubdef(field);

php/src/Google/Protobuf/Internal/Message.php

@@ -976,9 +976,12 @@ class Message
      * ]);
      * ```
      *
+     * This method will trigger an error if it is passed data that cannot
+     * be converted to the correct type. For example, a StringValue field
+     * must receive data that is either a string or a StringValue object.
+     *
      * @param array $array An array containing message properties and values.
      * @return null.
-     * @throws \Exception Invalid data.
      */
     protected function mergeFromArray(array $array)
     {
@@ -990,22 +993,61 @@ class Message
                     'Invalid message property: ' . $key);
             }
             $setter = $field->getSetter();
-            if ($field->isWrapperType()) {
-                self::normalizeToMessageType($value, $field->getMessageType()->getClass());
+            if ($field->isMap()) {
+                $valueField = $field->getMessageType()->getFieldByName('value');
+                if (!is_null($valueField) && $valueField->isWrapperType()) {
+                    self::normalizeArrayElementsToMessageType($value, $valueField->getMessageType()->getClass());
+                }
+            } elseif ($field->isWrapperType()) {
+                $class = $field->getMessageType()->getClass();
+                if ($field->isRepeated()) {
+                    self::normalizeArrayElementsToMessageType($value, $class);
+                } else {
+                    self::normalizeToMessageType($value, $class);
+                }
             }
             $this->$setter($value);
         }
     }
 
+    /**
+     * Tries to normalize the elements in $value into a provided protobuf
+     * wrapper type $class. If $value is any type other than array, we do
+     * not do any conversion, and instead rely on the existing protobuf
+     * type checking. If $value is an array, we process each element and
+     * try to convert it to an instance of $class.
+     *
+     * @param mixed $value The array of values to normalize.
+     * @param string $class The expected wrapper class name
+     */
+    private static function normalizeArrayElementsToMessageType(&$value, $class)
+    {
+        if (!is_array($value)) {
+            // In the case that $value is not an array, we do not want to
+            // attempt any conversion. Note that this includes the cases
+            // when $value is a RepeatedField of MapField. In those cases,
+            // we do not need to convert the elements, as they should
+            // already be the correct types.
+            return;
+        } else {
+            // Normalize each element in the array.
+            foreach ($value as $key => &$elementValue) {
+              self::normalizeToMessageType($elementValue, $class);
+            }
+        }
+    }
+
     /**
      * Tries to normalize $value into a provided protobuf wrapper type $class.
      * If $value is any type other than an object, we attempt to construct an
      * instance of $class and assign $value to it using the setValue method
      * shared by all wrapper types.
      *
+     * This method will raise an error if it receives a type that cannot be
+     * assigned to the wrapper type via setValue.
+     *
      * @param mixed $value The value to normalize.
      * @param string $class The expected wrapper class name
-     * @throws \Exception If $value cannot be converted to a wrapper type
      */
     private static function normalizeToMessageType(&$value, $class)
     {
@@ -1022,10 +1064,9 @@ class Message
                 $value = $msg;
                 return;
             } catch (\Exception $exception) {
-                throw new \Exception(
+                trigger_error(
                     "Error normalizing value to type '$class': " . $exception->getMessage(),
-                    $exception->getCode(),
-                    $exception
+                    E_USER_ERROR
                 );
             }
         }

php/tests/proto/test_wrapper_type_setters.proto

@@ -19,4 +19,8 @@ message TestWrapperSetters {
     google.protobuf.DoubleValue double_value_oneof = 10;
     google.protobuf.StringValue string_value_oneof = 11;
   }
+
+  repeated google.protobuf.StringValue repeated_string_value = 12;
+
+  map<string, google.protobuf.StringValue> map_string_value = 13;
 }

php/tests/wrapper_type_setters_test.php

@@ -225,4 +225,88 @@ class WrapperTypeSettersTest extends TestBase
             [TestWrapperSetters::class, BytesValue::class, 'bytes_value', 'getBytesValue', "nine"],
         ];
     }
+
+    /**
+     * @dataProvider constructorWithRepeatedWrapperTypeDataProvider
+     */
+    public function testConstructorWithRepeatedWrapperType($wrapperField, $getter, $value)
+    {
+        $actualInstance = new TestWrapperSetters([$wrapperField => $value]);
+        foreach ($actualInstance->$getter() as $key => $actualWrapperValue) {
+            $actualInnerValue = $actualWrapperValue->getValue();
+            $expectedElement = $value[$key];
+            if (is_object($expectedElement) && is_a($expectedElement, '\Google\Protobuf\StringValue')) {
+                $expectedInnerValue = $expectedElement->getValue();
+            } else {
+                $expectedInnerValue = $expectedElement;
+            }
+            $this->assertEquals($expectedInnerValue, $actualInnerValue);
+        }
+    }
+
+    public function constructorWithRepeatedWrapperTypeDataProvider()
+    {
+        $sv7 = new StringValue(['value' => 'seven']);
+        $sv8 = new StringValue(['value' => 'eight']);
+
+        $testWrapperSetters = new TestWrapperSetters();
+        $testWrapperSetters->setRepeatedStringValue([$sv7, $sv8]);
+        $repeatedField = $testWrapperSetters->getRepeatedStringValue();
+
+        return [
+            ['repeated_string_value', 'getRepeatedStringValue', []],
+            ['repeated_string_value', 'getRepeatedStringValue', [$sv7]],
+            ['repeated_string_value', 'getRepeatedStringValue', [$sv7, $sv8]],
+            ['repeated_string_value', 'getRepeatedStringValue', ['seven']],
+            ['repeated_string_value', 'getRepeatedStringValue', [7]],
+            ['repeated_string_value', 'getRepeatedStringValue', [7.7]],
+            ['repeated_string_value', 'getRepeatedStringValue', ['seven', 'eight']],
+            ['repeated_string_value', 'getRepeatedStringValue', [$sv7, 'eight']],
+            ['repeated_string_value', 'getRepeatedStringValue', ['seven', $sv8]],
+            ['repeated_string_value', 'getRepeatedStringValue', $repeatedField],
+        ];
+    }
+
+    /**
+     * @dataProvider constructorWithMapWrapperTypeDataProvider
+     */
+    public function testConstructorWithMapWrapperType($wrapperField, $getter, $value)
+    {
+        $actualInstance = new TestWrapperSetters([$wrapperField => $value]);
+        foreach ($actualInstance->$getter() as $key => $actualWrapperValue) {
+            $actualInnerValue = $actualWrapperValue->getValue();
+            $expectedElement = $value[$key];
+            if (is_object($expectedElement) && is_a($expectedElement, '\Google\Protobuf\StringValue')) {
+                $expectedInnerValue = $expectedElement->getValue();
+            } elseif (is_object($expectedElement) && is_a($expectedElement, '\Google\Protobuf\Internal\MapEntry')) {
+                $expectedInnerValue = $expectedElement->getValue()->getValue();
+            } else {
+                $expectedInnerValue = $expectedElement;
+            }
+            $this->assertEquals($expectedInnerValue, $actualInnerValue);
+        }
+    }
+
+    public function constructorWithMapWrapperTypeDataProvider()
+    {
+        $sv7 = new StringValue(['value' => 'seven']);
+        $sv8 = new StringValue(['value' => 'eight']);
+
+        $testWrapperSetters = new TestWrapperSetters();
+        $testWrapperSetters->setMapStringValue(['key' => $sv7, 'key2' => $sv8]);
+        $mapField = $testWrapperSetters->getMapStringValue();
+
+        return [
+            ['map_string_value', 'getMapStringValue', []],
+            ['map_string_value', 'getMapStringValue', ['key' => $sv7]],
+            ['map_string_value', 'getMapStringValue', ['key' => $sv7, 'key2' => $sv8]],
+            ['map_string_value', 'getMapStringValue', ['key' => 'seven']],
+            ['map_string_value', 'getMapStringValue', ['key' => 7]],
+            ['map_string_value', 'getMapStringValue', ['key' => 7.7]],
+            ['map_string_value', 'getMapStringValue', ['key' => 'seven', 'key2' => 'eight']],
+            ['map_string_value', 'getMapStringValue', ['key' => $sv7, 'key2' => 'eight']],
+            ['map_string_value', 'getMapStringValue', ['key' => 'seven', 'key2' => $sv8]],
+            ['map_string_value', 'getMapStringValue', $mapField],
+        ];
+    }
 }