Repeated/Map field setter should accept a regular PHP array (#2817)

Accept regular PHP array for repeated/map setter. Existing map/repeated
field will be swapped by a clean map/repeated field. Then, elements in
the array will be added to the map/repeated field. All elements will be
type checked before adding.

See #2686 for detail.

php/ext/google/protobuf/array.c

@@ -225,8 +225,17 @@ void repeated_field_push_native(RepeatedField *intern, void *value TSRMLS_DC) {
   zend_hash_next_index_insert(ht, (void **)value, size, NULL);
 }
 
+void repeated_field_create_with_field(zend_class_entry *ce,
+                                      const upb_fielddef *field,
+                                      zval **repeated_field TSRMLS_DC) {
+  upb_fieldtype_t type = upb_fielddef_type(field);
+  const zend_class_entry *msg_ce = field_type_class(field TSRMLS_CC);
+  repeated_field_create_with_type(ce, type, msg_ce, repeated_field);
+}
+
 void repeated_field_create_with_type(zend_class_entry *ce,
-                                     const upb_fielddef *field,
+                                     upb_fieldtype_t type,
+                                     const zend_class_entry* msg_ce,
                                      zval **repeated_field TSRMLS_DC) {
   MAKE_STD_ZVAL(*repeated_field);
   Z_TYPE_PP(repeated_field) = IS_OBJECT;
@@ -235,13 +244,8 @@ void repeated_field_create_with_type(zend_class_entry *ce,
 
   RepeatedField *intern =
       zend_object_store_get_object(*repeated_field TSRMLS_CC);
-  intern->type = upb_fielddef_type(field);
-  if (intern->type == UPB_TYPE_MESSAGE) {
-    const upb_msgdef *msg = upb_fielddef_msgsubdef(field);
-    zval *desc_php = get_def_obj(msg);
-    Descriptor *desc = zend_object_store_get_object(desc_php TSRMLS_CC);
-    intern->msg_ce = desc->klass;
-  }
+  intern->type = type;
+  intern->msg_ce = msg_ce;
   MAKE_STD_ZVAL(intern->array);
   repeated_field_array_init(intern->array, intern->type, 0 ZEND_FILE_LINE_CC);
 

php/ext/google/protobuf/map.c

@@ -146,6 +146,11 @@ static zend_function_entry map_field_methods[] = {
   ZEND_FE_END
 };
 
+// Forward declare static functions.
+
+static bool map_field_write_dimension(zval *object, zval *key,
+                                      zval *value TSRMLS_DC);
+
 // -----------------------------------------------------------------------------
 // MapField creation/desctruction
 // -----------------------------------------------------------------------------
@@ -183,6 +188,7 @@ void map_field_init(TSRMLS_D) {
   map_field_handlers = PEMALLOC(zend_object_handlers);
   memcpy(map_field_handlers, zend_get_std_object_handlers(),
          sizeof(zend_object_handlers));
+  map_field_handlers->write_dimension = map_field_write_dimension;
   map_field_handlers->get_gc = map_field_get_gc;
 }
 
@@ -235,7 +241,18 @@ void map_field_free(void *object TSRMLS_DC) {
   efree(object);
 }
 
-void map_field_create_with_type(zend_class_entry *ce, const upb_fielddef *field,
+void map_field_create_with_field(zend_class_entry *ce, const upb_fielddef *field,
+                                zval **map_field TSRMLS_DC) {
+  const upb_fielddef *key_field = map_field_key(field);
+  const upb_fielddef *value_field = map_field_value(field);
+  map_field_create_with_type(
+      ce, upb_fielddef_type(key_field), upb_fielddef_type(value_field),
+      field_type_class(value_field TSRMLS_CC), map_field);
+}
+
+void map_field_create_with_type(zend_class_entry *ce, upb_fieldtype_t key_type,
+                                upb_fieldtype_t value_type,
+                                const zend_class_entry *msg_ce,
                                 zval **map_field TSRMLS_DC) {
   MAKE_STD_ZVAL(*map_field);
   Z_TYPE_PP(map_field) = IS_OBJECT;
@@ -245,11 +262,9 @@ void map_field_create_with_type(zend_class_entry *ce, const upb_fielddef *field,
   Map* intern =
       (Map*)zend_object_store_get_object(*map_field TSRMLS_CC);
 
-  const upb_fielddef *key_field = map_field_key(field);
-  const upb_fielddef *value_field = map_field_value(field);
-  intern->key_type = upb_fielddef_type(key_field);
-  intern->value_type = upb_fielddef_type(value_field);
-  intern->msg_ce = field_type_class(value_field TSRMLS_CC);
+  intern->key_type = key_type;
+  intern->value_type = value_type;
+  intern->msg_ce = msg_ce;
 }
 
 static void map_field_free_element(void *object) {

php/ext/google/protobuf/protobuf.h

@@ -296,6 +296,7 @@ PHP_METHOD(Util, checkBool);
 PHP_METHOD(Util, checkString);
 PHP_METHOD(Util, checkBytes);
 PHP_METHOD(Util, checkMessage);
+PHP_METHOD(Util, checkMapField);
 PHP_METHOD(Util, checkRepeatedField);
 
 // -----------------------------------------------------------------------------
@@ -349,7 +350,11 @@ const upb_fielddef* map_entry_key(const upb_msgdef* msgdef);
 const upb_fielddef* map_entry_value(const upb_msgdef* msgdef);
 
 zend_object_value map_field_create(zend_class_entry *ce TSRMLS_DC);
-void map_field_create_with_type(zend_class_entry *ce, const upb_fielddef *field,
+void map_field_create_with_field(zend_class_entry *ce, const upb_fielddef *field,
+                                zval **map_field TSRMLS_DC);
+void map_field_create_with_type(zend_class_entry *ce, upb_fieldtype_t key_type,
+                                upb_fieldtype_t value_type,
+                                const zend_class_entry *msg_ce,
                                 zval **map_field TSRMLS_DC);
 void map_field_free(void* object TSRMLS_DC);
 void* upb_value_memory(upb_value* v);
@@ -392,9 +397,12 @@ struct RepeatedFieldIter {
   long position;
 };
 
-void repeated_field_create_with_type(zend_class_entry* ce,
+void repeated_field_create_with_field(zend_class_entry* ce,
                                      const upb_fielddef* field,
                                      zval** repeated_field TSRMLS_DC);
+void repeated_field_create_with_type(zend_class_entry* ce, upb_fieldtype_t type,
+                                     const zend_class_entry* msg_ce,
+                                     zval** repeated_field TSRMLS_DC);
 // Return the element at the index position from the repeated field. There is
 // not restriction on the type of stored elements.
 void *repeated_field_index_native(RepeatedField *intern, int index TSRMLS_DC);

php/ext/google/protobuf/storage.c

@@ -517,12 +517,12 @@ void layout_init(MessageLayout* layout, void* storage,
       *oneof_case = ONEOF_CASE_NONE;
     } else if (is_map_field(field)) {
       zval_ptr_dtor(property_ptr);
-      map_field_create_with_type(map_field_type, field, property_ptr TSRMLS_CC);
+      map_field_create_with_field(map_field_type, field, property_ptr TSRMLS_CC);
       DEREF(memory, zval**) = property_ptr;
     } else if (upb_fielddef_label(field) == UPB_LABEL_REPEATED) {
       zval_ptr_dtor(property_ptr);
-      repeated_field_create_with_type(repeated_field_type, field,
-                                      property_ptr TSRMLS_CC);
+      repeated_field_create_with_field(repeated_field_type, field,
+                                       property_ptr TSRMLS_CC);
       DEREF(memory, zval**) = property_ptr;
     } else {
       native_slot_init(upb_fielddef_type(field), memory, property_ptr);

php/ext/google/protobuf/type_check.c

@@ -51,6 +51,13 @@ ZEND_BEGIN_ARG_INFO_EX(arg_check_repeated, 0, 0, 2)
   ZEND_ARG_INFO(0, klass)
 ZEND_END_ARG_INFO()
 
+ZEND_BEGIN_ARG_INFO_EX(arg_check_map, 0, 0, 3)
+  ZEND_ARG_INFO(1, val)
+  ZEND_ARG_INFO(0, key_type)
+  ZEND_ARG_INFO(0, value_type)
+  ZEND_ARG_INFO(0, klass)
+ZEND_END_ARG_INFO()
+
 static zend_function_entry util_methods[] = {
   PHP_ME(Util, checkInt32,  arg_check_optional, ZEND_ACC_PUBLIC|ZEND_ACC_STATIC)
   PHP_ME(Util, checkUint32, arg_check_optional, ZEND_ACC_PUBLIC|ZEND_ACC_STATIC)
@@ -63,6 +70,7 @@ static zend_function_entry util_methods[] = {
   PHP_ME(Util, checkString, arg_check_optional, ZEND_ACC_PUBLIC|ZEND_ACC_STATIC)
   PHP_ME(Util, checkBytes,  arg_check_optional, ZEND_ACC_PUBLIC|ZEND_ACC_STATIC)
   PHP_ME(Util, checkMessage, arg_check_message, ZEND_ACC_PUBLIC|ZEND_ACC_STATIC)
+  PHP_ME(Util, checkMapField,    arg_check_map, ZEND_ACC_PUBLIC|ZEND_ACC_STATIC)
   PHP_ME(Util, checkRepeatedField, arg_check_repeated,
          ZEND_ACC_PUBLIC|ZEND_ACC_STATIC)
   ZEND_FE_END
@@ -411,20 +419,112 @@ PHP_METHOD(Util, checkRepeatedField) {
   zval* val;
   long type;
   const zend_class_entry* klass = NULL;
-  if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "Ol|C", &val,
-                            repeated_field_type, &type, &klass) == FAILURE) {
+  if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "zl|C", &val, &type,
+                            &klass) == FAILURE) {
     return;
   }
 
-  RepeatedField *intern =
-      (RepeatedField *)zend_object_store_get_object(val TSRMLS_CC);
-  if (to_fieldtype(type) != intern->type) {
+  if (Z_TYPE_P(val) == IS_ARRAY) {
+    HashTable* table = Z_ARRVAL_P(val);
+    HashPosition pointer;
+    void* memory;
+    zval* repeated_field;
+
+    repeated_field_create_with_type(repeated_field_type, to_fieldtype(type),
+                                    klass, &repeated_field TSRMLS_CC);
+    RepeatedField* intern =
+        (RepeatedField*)zend_object_store_get_object(repeated_field TSRMLS_CC);
+
+    for (zend_hash_internal_pointer_reset_ex(table, &pointer);
+         zend_hash_get_current_data_ex(table, (void**)&memory, &pointer) ==
+         SUCCESS;
+         zend_hash_move_forward_ex(table, &pointer)) {
+      repeated_field_handlers->write_dimension(repeated_field, NULL, *(zval**)memory);
+    }
+
+    Z_DELREF_P(repeated_field);
+    RETURN_ZVAL(repeated_field, 1, 0);
+
+  } else if (Z_TYPE_P(val) == IS_OBJECT) {
+    if (!instanceof_function(Z_OBJCE_P(val), repeated_field_type TSRMLS_CC)) {
+      zend_error(E_USER_ERROR, "Given value is not an instance of %s.",
+                 repeated_field_type->name);
+      return;
+    }
+    RepeatedField* intern =
+        (RepeatedField*)zend_object_store_get_object(val TSRMLS_CC);
+    if (to_fieldtype(type) != intern->type) {
+      zend_error(E_USER_ERROR, "Incorrect repeated field type.");
+      return;
+    }
+    if (klass != NULL && intern->msg_ce != klass) {
+      zend_error(E_USER_ERROR,
+                 "Expect a repeated field of %s, but %s is given.", klass->name,
+                 intern->msg_ce->name);
+      return;
+    }
+    RETURN_ZVAL(val, 1, 0);
+  } else {
     zend_error(E_USER_ERROR, "Incorrect repeated field type.");
     return;
   }
-  if (klass != NULL && intern->msg_ce != klass) {
-    zend_error(E_USER_ERROR, "Expect a repeated field of %s, but %s is given.",
-               klass->name, intern->msg_ce->name);
+
+}
+
+PHP_METHOD(Util, checkMapField) {
+  zval* val;
+  long key_type, value_type;
+  const zend_class_entry* klass = NULL;
+  if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "zll|C", &val, &key_type,
+                            &value_type, &klass) == FAILURE) {
+    return;
+  }
+
+  if (Z_TYPE_P(val) == IS_ARRAY) {
+    HashTable* table = Z_ARRVAL_P(val);
+    HashPosition pointer;
+    zval key, *map_field;
+    void* value;
+
+    map_field_create_with_type(map_field_type, to_fieldtype(key_type),
+                               to_fieldtype(value_type), klass,
+                               &map_field TSRMLS_CC);
+    Map* intern =
+        (Map*)zend_object_store_get_object(map_field TSRMLS_CC);
+
+    for (zend_hash_internal_pointer_reset_ex(table, &pointer);
+         zend_hash_get_current_data_ex(table, (void**)&value, &pointer) ==
+         SUCCESS;
+         zend_hash_move_forward_ex(table, &pointer)) {
+      zend_hash_get_current_key_zval_ex(table, &key, &pointer);
+      map_field_handlers->write_dimension(map_field, &key, *(zval**)value);
+    }
+
+    Z_DELREF_P(map_field);
+    RETURN_ZVAL(map_field, 1, 0);
+  } else if (Z_TYPE_P(val) == IS_OBJECT) {
+    if (!instanceof_function(Z_OBJCE_P(val), map_field_type TSRMLS_CC)) {
+      zend_error(E_USER_ERROR, "Given value is not an instance of %s.",
+                 map_field_type->name);
+      return;
+    }
+    Map* intern = (Map*)zend_object_store_get_object(val TSRMLS_CC);
+    if (to_fieldtype(key_type) != intern->key_type) {
+      zend_error(E_USER_ERROR, "Incorrect map field key type.");
+      return;
+    }
+    if (to_fieldtype(value_type) != intern->value_type) {
+      zend_error(E_USER_ERROR, "Incorrect map field value type.");
+      return;
+    }
+    if (klass != NULL && intern->msg_ce != klass) {
+      zend_error(E_USER_ERROR, "Expect a map field of %s, but %s is given.",
+                 klass->name, intern->msg_ce->name);
+      return;
+    }
+    RETURN_ZVAL(val, 1, 0);
+  } else {
+    zend_error(E_USER_ERROR, "Incorrect map field type.");
     return;
   }
 }

php/src/Google/Protobuf/Internal/GPBUtil.php

@@ -34,6 +34,7 @@ namespace Google\Protobuf\Internal;
 
 use Google\Protobuf\Internal\GPBType;
 use Google\Protobuf\Internal\RepeatedField;
+use Google\Protobuf\Internal\MapField;
 
 class GPBUtil
 {
@@ -175,19 +176,60 @@ class GPBUtil
 
     public static function checkRepeatedField(&$var, $type, $klass = null)
     {
-        if (!$var instanceof RepeatedField) {
-            trigger_error("Expect repeated field.", E_USER_ERROR);
-        }
-        if ($var->getType() != $type) {
-            trigger_error(
-                "Expect repeated field of different type.",
-                E_USER_ERROR);
-        }
-        if ($var->getType() === GPBType::MESSAGE &&
-            $var->getClass() !== $klass) {
-            trigger_error(
-                "Expect repeated field of different message.",
-                E_USER_ERROR);
+        if (!$var instanceof RepeatedField && !is_array($var)) {
+            trigger_error("Expect array.", E_USER_ERROR);
+        }
+        if (is_array($var)) {
+            $tmp = new RepeatedField($type, $klass);
+            foreach ($var as $value) {
+                $tmp[] = $value;
+            }
+            return $tmp;
+        } else {
+            if ($var->getType() != $type) {
+                trigger_error(
+                    "Expect repeated field of different type.",
+                    E_USER_ERROR);
+            }
+            if ($var->getType() === GPBType::MESSAGE &&
+                $var->getClass() !== $klass) {
+                trigger_error(
+                    "Expect repeated field of different message.",
+                    E_USER_ERROR);
+            }
+            return $var;
+        }
+    }
+
+    public static function checkMapField(&$var, $key_type, $value_type, $klass = null)
+    {
+        if (!$var instanceof MapField && !is_array($var)) {
+            trigger_error("Expect dict.", E_USER_ERROR);
+        }
+        if (is_array($var)) {
+            $tmp = new MapField($key_type, $value_type, $klass);
+            foreach ($var as $key => $value) {
+                $tmp[$key] = $value;
+            }
+            return $tmp;
+        } else {
+            if ($var->getKeyType() != $key_type) {
+                trigger_error(
+                    "Expect map field of key type.",
+                    E_USER_ERROR);
+            }
+            if ($var->getValueType() != $value_type) {
+                trigger_error(
+                    "Expect map field of value type.",
+                    E_USER_ERROR);
+            }
+            if ($var->getValueType() === GPBType::MESSAGE &&
+                $var->getValueClass() !== $klass) {
+                trigger_error(
+                    "Expect map field of different value message.",
+                    E_USER_ERROR);
+            }
+            return $var;
         }
     }
 

php/src/Google/Protobuf/Internal/MapField.php

@@ -203,6 +203,30 @@ class MapField implements \ArrayAccess, \IteratorAggregate, \Countable
         $this->klass = $klass;
     }
 
+    /**
+     * @ignore
+     */
+    public function getKeyType()
+    {
+        return $this->key_type;
+    }
+
+    /**
+     * @ignore
+     */
+    public function getValueType()
+    {
+        return $this->value_type;
+    }
+
+    /**
+     * @ignore
+     */
+    public function getValueClass()
+    {
+        return $this->klass;
+    }
+
     /**
      * Return the element at the given key.
      *

php/tests/generated_class_test.php

@@ -6,6 +6,7 @@ require_once('test_base.php');
 require_once('test_util.php');
 
 use Google\Protobuf\Internal\RepeatedField;
+use Google\Protobuf\Internal\MapField;
 use Google\Protobuf\Internal\GPBType;
 use Foo\TestEnum;
 use Foo\TestMessage;
@@ -526,6 +527,26 @@ class GeneratedClassTest extends TestBase
         $this->assertSame($repeated_int32, $m->getRepeatedInt32());
     }
 
+    public function testRepeatedFieldViaArray()
+    {
+        $m = new TestMessage();
+
+        $arr = array();
+        $m->setRepeatedInt32($arr);
+        $this->assertSame(0, count($m->getRepeatedInt32()));
+
+        $arr = array(1, 2.1, "3");
+        $m->setRepeatedInt32($arr);
+        $this->assertTrue($m->getRepeatedInt32() instanceof RepeatedField);
+        $this->assertSame("Google\Protobuf\Internal\RepeatedField",
+                          get_class($m->getRepeatedInt32()));
+        $this->assertSame(3, count($m->getRepeatedInt32()));
+        $this->assertSame(1, $m->getRepeatedInt32()[0]);
+        $this->assertSame(2, $m->getRepeatedInt32()[1]);
+        $this->assertSame(3, $m->getRepeatedInt32()[2]);
+        $this->assertFalse($arr instanceof RepeatedField);
+    }
+
     /**
      * @expectedException PHPUnit_Framework_Error
      */
@@ -568,6 +589,80 @@ class GeneratedClassTest extends TestBase
         $m->setRepeatedMessage($repeated_message);
     }
 
+    #########################################################
+    # Test map field.
+    #########################################################
+
+    public function testMapField()
+    {
+        $m = new TestMessage();
+
+        $map_int32_int32 = new MapField(GPBType::INT32, GPBType::INT32);
+        $m->setMapInt32Int32($map_int32_int32);
+        $this->assertSame($map_int32_int32, $m->getMapInt32Int32());
+    }
+
+    public function testMapFieldViaArray()
+    {
+        $m = new TestMessage();
+
+        $dict = array();
+        $m->setMapInt32Int32($dict);
+        $this->assertSame(0, count($m->getMapInt32Int32()));
+
+        $dict = array(5 => 5, 6.1 => 6.1, "7" => "7");
+        $m->setMapInt32Int32($dict);
+        $this->assertTrue($m->getMapInt32Int32() instanceof MapField);
+        $this->assertSame(3, count($m->getMapInt32Int32()));
+        $this->assertSame(5, $m->getMapInt32Int32()[5]);
+        $this->assertSame(6, $m->getMapInt32Int32()[6]);
+        $this->assertSame(7, $m->getMapInt32Int32()[7]);
+        $this->assertFalse($dict instanceof MapField);
+    }
+
+    /**
+     * @expectedException PHPUnit_Framework_Error
+     */
+    public function testMapFieldWrongTypeFail()
+    {
+        $m = new TestMessage();
+        $a = 1;
+        $m->setMapInt32Int32($a);
+    }
+
+    /**
+     * @expectedException PHPUnit_Framework_Error
+     */
+    public function testMapFieldWrongObjectFail()
+    {
+        $m = new TestMessage();
+        $m->setMapInt32Int32($m);
+    }
+
+    /**
+     * @expectedException PHPUnit_Framework_Error
+     */
+    public function testMapFieldWrongRepeatedTypeFail()
+    {
+        $m = new TestMessage();
+
+        $map_uint32_uint32 = new MapField(GPBType::UINT32, GPBType::UINT32);
+        $m->setMapInt32Int32($map_uint32_uint32);
+    }
+
+    /**
+     * @expectedException PHPUnit_Framework_Error
+     */
+    public function testMapFieldWrongRepeatedMessageClassFail()
+    {
+        $m = new TestMessage();
+
+        $map_int32_message = new MapField(GPBType::INT32,
+                                          GPBType::MESSAGE,
+                                          TestMessage::class);
+        $m->setMapInt32Message($map_int32_message);
+    }
+
     #########################################################
     # Test oneof field.
     #########################################################

src/google/protobuf/compiler/php/php_generator.cc

@@ -439,9 +439,31 @@ void GenerateFieldAccessor(const FieldDescriptor* field, bool is_descriptor,
 
   // Type check.
   if (field->is_map()) {
+    const Descriptor* map_entry = field->message_type();
+    const FieldDescriptor* key = map_entry->FindFieldByName("key");
+    const FieldDescriptor* value = map_entry->FindFieldByName("value");
+    printer->Print(
+        "$arr = GPBUtil::checkMapField($var, "
+        "\\Google\\Protobuf\\Internal\\GPBType::^key_type^, "
+        "\\Google\\Protobuf\\Internal\\GPBType::^value_type^",
+        "key_type", ToUpper(key->type_name()),
+        "value_type", ToUpper(value->type_name()));
+    if (value->cpp_type() == FieldDescriptor::CPPTYPE_MESSAGE) {
+      printer->Print(
+          ", \\^class_name^);\n",
+          "class_name",
+          MessageName(value->message_type(), is_descriptor) + "::class");
+    } else if (value->cpp_type() == FieldDescriptor::CPPTYPE_ENUM) {
+      printer->Print(
+          ", ^class_name^);\n",
+          "class_name",
+          EnumName(value->enum_type(), is_descriptor) + "::class");
+    } else {
+      printer->Print(");\n");
+    }
   } else if (field->is_repeated()) {
     printer->Print(
-        "GPBUtil::checkRepeatedField($var, "
+        "$arr = GPBUtil::checkRepeatedField($var, "
         "\\Google\\Protobuf\\Internal\\GPBType::^type^",
         "type", ToUpper(field->type_name()));
     if (field->cpp_type() == FieldDescriptor::CPPTYPE_MESSAGE) {
@@ -480,6 +502,10 @@ void GenerateFieldAccessor(const FieldDescriptor* field, bool is_descriptor,
     printer->Print(
         "$this->writeOneof(^number^, $var);\n",
         "number", IntToString(field->number()));
+  } else if (field->is_repeated()) {
+    printer->Print(
+        "$this->^name^ = $arr;\n",
+        "name", field->name());
   } else {
     printer->Print(
         "$this->^name^ = $var;\n",