Added proto3 presence support for PHP (#7724)

* WIP. * Added proto3 presence support for PHP. * Added compatibility code for old generated code.
5 years ago · ad5a215359
parent 0d90ff3d72
commit ad5a215359
17 changed files with 262 additions and 43 deletions
--- a/Makefile.am
+++ b/Makefile.am
@ -954,6 +954,7 @@ php_EXTRA_DIST=                                                       \
  php/tests/test_base.php                                             \
  php/tests/test_util.php                                             \
  php/tests/undefined_test.php                                        \
+  php/tests/valgrind.supp                                             \
  php/tests/well_known_test.php                                       \
  php/tests/wrapper_type_setters_test.php

--- a/php/ext/google/protobuf/message.c
+++ b/php/ext/google/protobuf/message.c
@ -108,6 +108,76 @@ static const upb_fielddef *get_field(Message *msg, zval *member) {
  return f;
 }

+/**
+ * Message_has_property()
+ *
+ * Object handler for testing whether a property exists. Called when PHP code
+ * does any of:
+ *
+ *   isset($message->foobar);
+ *   property_exists($message->foobar);
+ *
+ * Note that all properties of generated messages are private, so this should
+ * only be possible to invoke from generated code, which has accessors like this
+ * (if the field has presence):
+ *
+ *   public function hasOptionalInt32()
+ *   {
+ *       return isset($this->optional_int32);
+ *   }
+ */
+static int Message_has_property(zval *obj, zval *member, int has_set_exists,
+                                void **cache_slot) {
+  Message* intern = (Message*)Z_OBJ_P(obj);
+  const upb_fielddef *f = get_field(intern, member);
+
+  if (!f) return 0;
+
+  if (!upb_fielddef_haspresence(f)) {
+    zend_throw_exception_ex(
+        NULL, 0,
+        "Cannot call isset() on field %s which does not have presence.",
+        ZSTR_VAL(intern->desc->class_entry->name));
+    return 0;
+  }
+
+  return upb_msg_has(intern->msg, f);
+}
+
+/**
+ * Message_unset_property()
+ *
+ * Object handler for unsetting a property. Called when PHP code calls:
+ * does any of:
+ *
+ *   unset($message->foobar);
+ *
+ * Note that all properties of generated messages are private, so this should
+ * only be possible to invoke from generated code, which has accessors like this
+ * (if the field has presence):
+ *
+ *   public function clearOptionalInt32()
+ *   {
+ *       unset($this->optional_int32);
+ *   }
+ */
+static void Message_unset_property(zval *obj, zval *member, void **cache_slot) {
+  Message* intern = (Message*)Z_OBJ_P(obj);
+  const upb_fielddef *f = get_field(intern, member);
+
+  if (!f) return;
+
+  if (!upb_fielddef_haspresence(f)) {
+    zend_throw_exception_ex(
+        NULL, 0,
+        "Cannot call unset() on field %s which does not have presence.",
+        ZSTR_VAL(intern->desc->class_entry->name));
+    return;
+  }
+
+  upb_msg_clearfield(intern->msg, f);
+}
+
 /**
 * Message_read_property()
 *
@ -836,6 +906,8 @@ void Message_ModuleInit() {
  h->dtor_obj = Message_dtor;
  h->read_property = Message_read_property;
  h->write_property = Message_write_property;
+  h->has_property = Message_has_property;
+  h->unset_property = Message_unset_property;
  h->get_properties = Message_get_properties;
  h->get_property_ptr_ptr = Message_get_property_ptr_ptr;
 }
--- a/php/src/GPBMetadata/Google/Protobuf/Struct.php
+++ b/php/src/GPBMetadata/Google/Protobuf/Struct.php
@ -15,29 +15,8 @@ class Struct
          return;
        }
        $pool->internalAddGeneratedFile(hex2bin(
-            "0a81050a1c676f6f676c652f70726f746f6275662f7374727563742e7072" .
-            "6f746f120f676f6f676c652e70726f746f6275662284010a065374727563" .
-            "7412330a066669656c647318012003280b32232e676f6f676c652e70726f" .
-            "746f6275662e5374727563742e4669656c6473456e7472791a450a0b4669" .
-            "656c6473456e747279120b0a036b657918012001280912250a0576616c75" .
-            "6518022001280b32162e676f6f676c652e70726f746f6275662e56616c75" .
-            "653a02380122ea010a0556616c756512300a0a6e756c6c5f76616c756518" .
-            "012001280e321a2e676f6f676c652e70726f746f6275662e4e756c6c5661" .
-            "6c7565480012160a0c6e756d6265725f76616c7565180220012801480012" .
-            "160a0c737472696e675f76616c7565180320012809480012140a0a626f6f" .
-            "6c5f76616c75651804200128084800122f0a0c7374727563745f76616c75" .
-            "6518052001280b32172e676f6f676c652e70726f746f6275662e53747275" .
-            "6374480012300a0a6c6973745f76616c756518062001280b321a2e676f6f" .
-            "676c652e70726f746f6275662e4c69737456616c7565480042060a046b69" .
-            "6e6422330a094c69737456616c756512260a0676616c7565731801200328" .
-            "0b32162e676f6f676c652e70726f746f6275662e56616c75652a1b0a094e" .
-            "756c6c56616c7565120e0a0a4e554c4c5f56414c554510004281010a1363" .
-            "6f6d2e676f6f676c652e70726f746f627566420b53747275637450726f74" .
-            "6f50015a316769746875622e636f6d2f676f6c616e672f70726f746f6275" .
-            "662f7074797065732f7374727563743b7374727563747062f80101a20203" .
-            "475042aa021e476f6f676c652e50726f746f6275662e57656c6c4b6e6f77" .
-            "6e5479706573620670726f746f33"
-        ));
+            "0a81050a1c676f6f676c652f70726f746f6275662f7374727563742e70726f746f120f676f6f676c652e70726f746f6275662284010a0653747275637412330a066669656c647318012003280b32232e676f6f676c652e70726f746f6275662e5374727563742e4669656c6473456e7472791a450a0b4669656c6473456e747279120b0a036b657918012001280912250a0576616c756518022001280b32162e676f6f676c652e70726f746f6275662e56616c75653a02380122ea010a0556616c756512300a0a6e756c6c5f76616c756518012001280e321a2e676f6f676c652e70726f746f6275662e4e756c6c56616c7565480012160a0c6e756d6265725f76616c7565180220012801480012160a0c737472696e675f76616c7565180320012809480012140a0a626f6f6c5f76616c75651804200128084800122f0a0c7374727563745f76616c756518052001280b32172e676f6f676c652e70726f746f6275662e537472756374480012300a0a6c6973745f76616c756518062001280b321a2e676f6f676c652e70726f746f6275662e4c69737456616c7565480042060a046b696e6422330a094c69737456616c756512260a0676616c75657318012003280b32162e676f6f676c652e70726f746f6275662e56616c75652a1b0a094e756c6c56616c7565120e0a0a4e554c4c5f56414c554510004281010a13636f6d2e676f6f676c652e70726f746f627566420b53747275637450726f746f50015a316769746875622e636f6d2f676f6c616e672f70726f746f6275662f7074797065732f7374727563743b7374727563747062f80101a20203475042aa021e476f6f676c652e50726f746f6275662e57656c6c4b6e6f776e5479706573620670726f746f33"
+        ), true);

        static::$is_initialized = true;
    }
--- a/php/src/Google/Protobuf/Descriptor.php
+++ b/php/src/Google/Protobuf/Descriptor.php
@ -97,4 +97,12 @@ class Descriptor
    {
        return count($this->internal_desc->getOneofDecl());
    }
+
+    /**
+     * @return int Number of real oneofs in message
+     */
+    public function getRealOneofDeclCount()
+    {
+        return $this->internal_desc->getRealOneofDeclCount();
+    }
 }
--- a/php/src/Google/Protobuf/FieldDescriptor.php
+++ b/php/src/Google/Protobuf/FieldDescriptor.php
@ -114,4 +114,12 @@ class FieldDescriptor
    {
        return $this->internal_desc->isMap();
    }
+
+    /**
+     * @return boolean
+     */
+    public function hasOptionalKeyword()
+    {
+        return $this->internal_desc->hasOptionalKeyword();
+    }
 }
--- a/php/src/Google/Protobuf/Internal/Message.php
+++ b/php/src/Google/Protobuf/Internal/Message.php
@ -225,6 +225,15 @@ class Message
        }
    }

+    protected function hasOneof($number)
+    {
+        $field = $this->desc->getFieldByNumber($number);
+        $oneof = $this->desc->getOneofDecl()[$field->getOneofIndex()];
+        $oneof_name = $oneof->getName();
+        $oneof_field = $this->$oneof_name;
+        return $number === $oneof_field->getNumber();
+    }
+
    protected function writeOneof($number, $value)
    {
        $field = $this->desc->getFieldByNumber($number);
@ -1559,14 +1568,19 @@ class Message
     */
    private function existField($field)
    {
-        $oneof_index = $field->getOneofIndex();
-        if ($oneof_index !== -1) {
-            $oneof = $this->desc->getOneofDecl()[$oneof_index];
-            $oneof_name = $oneof->getName();
-            return $this->$oneof_name->getNumber() === $field->getNumber();
+        $getter = $field->getGetter();
+        $hazzer = "has" . substr($getter, 3);
+
+        if (method_exists($this, $hazzer)) {
+          return $this->$hazzer();
+        } else if ($field->getOneofIndex() !== -1) {
+          // For old generated code, which does not have hazzers for oneof
+          // fields.
+          $oneof = $this->desc->getOneofDecl()[$field->getOneofIndex()];
+          $oneof_name = $oneof->getName();
+          return $this->$oneof_name->getNumber() === $field->getNumber();
        }

-        $getter = $field->getGetter();
        $values = $this->$getter();
        if ($field->isMap()) {
            return count($values) !== 0;
--- a/php/src/Google/Protobuf/NullValue.php
+++ b/php/src/Google/Protobuf/NullValue.php
@ -35,6 +35,7 @@ class NullValue
        return self::$valueToName[$value];
    }

+
    public static function value($name)
    {
        $const = __CLASS__ . '::' . strtoupper($name);
--- a/php/src/Google/Protobuf/OneofDescriptor.php
+++ b/php/src/Google/Protobuf/OneofDescriptor.php
@ -72,4 +72,9 @@ class OneofDescriptor
    {
        return count($this->internal_desc->getFields());
    }
+
+    public function isSynthetic()
+    {
+      return $this->internal_desc->isSynthetic();
+    }
 }
--- a/php/src/Google/Protobuf/Value.php
+++ b/php/src/Google/Protobuf/Value.php
@ -57,6 +57,11 @@ class Value extends \Google\Protobuf\Internal\Message
        return $this->readOneof(1);
    }

+    public function hasNullValue()
+    {
+        return $this->hasOneof(1);
+    }
+
    /**
     * Represents a null value.
     *
@ -83,6 +88,11 @@ class Value extends \Google\Protobuf\Internal\Message
        return $this->readOneof(2);
    }

+    public function hasNumberValue()
+    {
+        return $this->hasOneof(2);
+    }
+
    /**
     * Represents a double value.
     *
@ -109,6 +119,11 @@ class Value extends \Google\Protobuf\Internal\Message
        return $this->readOneof(3);
    }

+    public function hasStringValue()
+    {
+        return $this->hasOneof(3);
+    }
+
    /**
     * Represents a string value.
     *
@ -135,6 +150,11 @@ class Value extends \Google\Protobuf\Internal\Message
        return $this->readOneof(4);
    }

+    public function hasBoolValue()
+    {
+        return $this->hasOneof(4);
+    }
+
    /**
     * Represents a boolean value.
     *
@ -161,6 +181,11 @@ class Value extends \Google\Protobuf\Internal\Message
        return $this->readOneof(5);
    }

+    public function hasStructValue()
+    {
+        return $this->hasOneof(5);
+    }
+
    /**
     * Represents a structured value.
     *
@ -187,6 +212,11 @@ class Value extends \Google\Protobuf\Internal\Message
        return $this->readOneof(6);
    }

+    public function hasListValue()
+    {
+        return $this->hasOneof(6);
+    }
+
    /**
     * Represents a repeated `Value`.
     *
--- a/php/tests/encode_decode_test.php
+++ b/php/tests/encode_decode_test.php
@ -326,6 +326,24 @@ class EncodeDecodeTest extends TestBase

    }

+    public function testEncodeDecodeOptional()
+    {
+      $m = new TestMessage();
+      $this->assertFalse($m->hasTrueOptionalInt32());
+      $data = $m->serializeToString();
+      $this->assertSame("", $data);
+
+      $m->setTrueOptionalInt32(0);
+      $this->assertTrue($m->hasTrueOptionalInt32());
+      $data = $m->serializeToString();
+      $this->assertNotSame("", $data);
+
+      $m2 = new TestMessage();
+      $m2->mergeFromString($data);
+      $this->assertTrue($m2->hasTrueOptionalInt32());
+      $this->assertSame(0, $m2->getTrueOptionalInt32());
+    }
+
    public function testJsonEncodeDecodeOneof()
    {
        $m = new TestMessage();
--- a/php/tests/generate_protos.sh
+++ b/php/tests/generate_protos.sh
@ -7,10 +7,10 @@ cd `dirname $0`
 rm -rf generated
 mkdir -p generated

-find proto -type f -name "*.proto"| xargs ../../src/protoc --php_out=generated -I../../src -I.
+find proto -type f -name "*.proto"| xargs ../../src/protoc --experimental_allow_proto3_optional --php_out=generated -I../../src -I.

 if [ "$1" = "--aggregate_metadata" ]; then
  # Overwrite some of the files to use aggregation.
  AGGREGATED_FILES="proto/test.proto proto/test_include.proto proto/test_import_descriptor_proto.proto"
-  ../../src/protoc --php_out=aggregate_metadata=foo#bar:generated -I../../src -I. $AGGREGATED_FILES
+  ../../src/protoc --experimental_allow_proto3_optional --php_out=aggregate_metadata=foo#bar:generated -I../../src -I. $AGGREGATED_FILES
 fi
--- a/php/tests/generated_class_test.php
+++ b/php/tests/generated_class_test.php
@ -71,6 +71,28 @@ class GeneratedClassTest extends TestBase
        $this->assertSame(MIN_INT32, $m->getOptionalInt32());
    }

+    #########################################################
+    # Test optional int32 field.
+    #########################################################
+
+    public function testOptionalInt32Field()
+    {
+        $m = new TestMessage();
+
+        $this->assertFalse($m->hasTrueOptionalInt32());
+        $this->assertSame(0, $m->getTrueOptionalInt32());
+
+        // Set integer.
+        $m->setTrueOptionalInt32(MAX_INT32);
+        $this->assertTrue($m->hasTrueOptionalInt32());
+        $this->assertSame(MAX_INT32, $m->getTrueOptionalInt32());
+
+        // Clear integer.
+        $m->clearTrueOptionalInt32();
+        $this->assertFalse($m->hasTrueOptionalInt32());
+        $this->assertSame(0, $m->getTrueOptionalInt32());
+    }
+
    #########################################################
    # Test uint32 field.
    #########################################################
--- a/php/tests/proto/test.proto
+++ b/php/tests/proto/test.proto
@ -34,6 +34,27 @@ message TestMessage {
  bar.TestInclude optional_included_message = 18;
  TestMessage recursive = 19;

+  // True optional
+  optional int32 true_optional_int32 = 201;
+  optional int64 true_optional_int64 = 202;
+  optional uint32 true_optional_uint32 = 203;
+  optional uint64 true_optional_uint64 = 204;
+  optional sint32 true_optional_sint32 = 205;
+  optional sint64 true_optional_sint64 = 206;
+  optional fixed32 true_optional_fixed32 = 207;
+  optional fixed64 true_optional_fixed64 = 208;
+  optional sfixed32 true_optional_sfixed32 = 209;
+  optional sfixed64 true_optional_sfixed64 = 210;
+  optional float true_optional_float = 211;
+  optional double true_optional_double = 212;
+  optional bool true_optional_bool = 213;
+  optional string true_optional_string = 214;
+  optional bytes true_optional_bytes = 215;
+
+  optional TestEnum true_optional_enum = 216;
+  optional Sub true_optional_message = 217;
+  optional bar.TestInclude true_optional_included_message = 218;
+
  // Repeated
  repeated    int32 repeated_int32    = 31;
  repeated    int64 repeated_int64    = 32;
--- a/php/tests/test.sh
+++ b/php/tests/test.sh
@ -51,8 +51,8 @@ done

 export ZEND_DONT_UNLOAD_MODULES=1
 export USE_ZEND_ALLOC=0
-valgrind --leak-check=yes php -dextension=../ext/google/protobuf/modules/protobuf.so memory_leak_test.php
-valgrind --leak-check=yes php -d protobuf.keep_descriptor_pool_after_request=1 -dextension=../ext/google/protobuf/modules/protobuf.so memory_leak_test.php
+valgrind --suppressions=valgrind.supp --leak-check=yes php -dextension=../ext/google/protobuf/modules/protobuf.so memory_leak_test.php
+valgrind --suppressions=valgrind.supp --leak-check=yes php -d protobuf.keep_descriptor_pool_after_request=1 -dextension=../ext/google/protobuf/modules/protobuf.so memory_leak_test.php

 # TODO(teboring): Only for debug (phpunit has memory leak which blocks this beging used by
 # regresssion test.)
--- a/php/tests/valgrind.supp
+++ b/php/tests/valgrind.supp
@ -0,0 +1,12 @@
+{
+   PHP_Equal_Val
+   Memcheck:Cond
+   fun:zend_string_equal_val
+}
+
+{
+   PHP_ScanDir_Tail
+   Memcheck:Cond
+   obj:/usr/bin/php7.3
+   fun:__scandir64_tail
+}
--- a/src/google/protobuf/compiler/php/php_generator.cc
+++ b/src/google/protobuf/compiler/php/php_generator.cc
@ -624,15 +624,17 @@ void GenerateField(const FieldDescriptor* field, io::Printer* printer,
    printer->Print(
        "private $^name^;\n",
        "name", field->name());
-  } else if (field->containing_oneof()) {
+  } else if (field->real_containing_oneof()) {
    // Oneof fields are handled by GenerateOneofField.
    return;
  } else {
+    std::string initial_value =
+        field->has_presence() ? "null" : DefaultForField(field);
    GenerateFieldDocComment(printer, field, is_descriptor, kFieldProperty);
    printer->Print(
-        "protected $^name^ = ^default^;\n",
+        "protected $^name^ = ^initial_value^;\n",
        "name", field->name(),
-        "default", DefaultForField(field));
+        "initial_value", initial_value);
  }

  if (is_descriptor) {
@ -652,20 +654,41 @@ void GenerateOneofField(const OneofDescriptor* oneof, io::Printer* printer) {

 void GenerateFieldAccessor(const FieldDescriptor* field, bool is_descriptor,
                           io::Printer* printer) {
-  const OneofDescriptor* oneof = field->containing_oneof();
+  const OneofDescriptor* oneof = field->real_containing_oneof();

  // Generate getter.
+  GenerateFieldDocComment(printer, field, is_descriptor, kFieldGetter);
+
  if (oneof != NULL) {
-    GenerateFieldDocComment(printer, field, is_descriptor, kFieldGetter);
    printer->Print(
        "public function get^camel_name^()\n"
        "{\n"
        "    return $this->readOneof(^number^);\n"
+        "}\n\n"
+        "public function has^camel_name^()\n"
+        "{\n"
+        "    return $this->hasOneof(^number^);\n"
        "}\n\n",
        "camel_name", UnderscoresToCamelCase(field->name(), true),
        "number", IntToString(field->number()));
+  } else if (field->has_presence()) {
+    printer->Print(
+        "public function get^camel_name^()\n"
+        "{\n"
+        "    return isset($this->^name^) ? $this->^name^ : ^default_value^;\n"
+        "}\n\n"
+        "public function has^camel_name^()\n"
+        "{\n"
+        "    return isset($this->^name^);\n"
+        "}\n\n"
+        "public function clear^camel_name^()\n"
+        "{\n"
+        "    unset($this->^name^);\n"
+        "}\n\n",
+        "camel_name", UnderscoresToCamelCase(field->name(), true),
+        "name", field->name(),
+        "default_value", DefaultForField(field));
  } else {
-    GenerateFieldDocComment(printer, field, is_descriptor, kFieldGetter);
    printer->Print(
        "public function get^camel_name^()\n"
        "{\n"
@ -878,7 +901,7 @@ void GenerateMessageToPool(const string& name_prefix, const Descriptor* message,
          "value", ToUpper(val->type_name()),
          "number", StrCat(field->number()),
          "other", EnumOrMessageSuffix(val, true));
-    } else if (!field->containing_oneof()) {
+    } else if (!field->real_containing_oneof()) {
      printer->Print(
          "->^label^('^field^', "
          "\\Google\\Protobuf\\Internal\\GPBType::^type^, ^number^^other^)\n",
@ -891,7 +914,7 @@ void GenerateMessageToPool(const string& name_prefix, const Descriptor* message,
  }

  // oneofs.
-  for (int i = 0; i < message->oneof_decl_count(); i++) {
+  for (int i = 0; i < message->real_oneof_decl_count(); i++) {
    const OneofDescriptor* oneof = message->oneof_decl(i);
    printer->Print("->oneof(^name^)\n",
                   "name", oneof->name());
@ -1414,7 +1437,7 @@ void GenerateMessageFile(const FileDescriptor* file, const Descriptor* message,
    const FieldDescriptor* field = message->field(i);
    GenerateField(field, &printer, is_descriptor);
  }
-  for (int i = 0; i < message->oneof_decl_count(); i++) {
+  for (int i = 0; i < message->real_oneof_decl_count(); i++) {
    const OneofDescriptor* oneof = message->oneof_decl(i);
    GenerateOneofField(oneof, &printer);
  }
@ -1443,7 +1466,7 @@ void GenerateMessageFile(const FileDescriptor* file, const Descriptor* message,
    const FieldDescriptor* field = message->field(i);
    GenerateFieldAccessor(field, is_descriptor, &printer);
  }
-  for (int i = 0; i < message->oneof_decl_count(); i++) {
+  for (int i = 0; i < message->real_oneof_decl_count(); i++) {
    const OneofDescriptor* oneof = message->oneof_decl(i);
    printer.Print(
      "/**\n"
--- a/src/google/protobuf/compiler/php/php_generator.h
+++ b/src/google/protobuf/compiler/php/php_generator.h
@ -55,6 +55,11 @@ class PROTOC_EXPORT Generator : public CodeGenerator {
                   const std::string& parameter,
                   GeneratorContext* generator_context,
                   std::string* error) const override;
+
+  uint64_t GetSupportedFeatures() const override {
+    return FEATURE_PROTO3_OPTIONAL;
+  }
+
 private:
  bool Generate(
      const FileDescriptor* file,