diff --git a/Makefile.am b/Makefile.am index 631df6f31..15b5a4203 100644 --- 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 diff --git a/php/ext/google/protobuf/message.c b/php/ext/google/protobuf/message.c index 20dd37a76..78f37ede1 100644 --- 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; } diff --git a/php/src/GPBMetadata/Google/Protobuf/Struct.php b/php/src/GPBMetadata/Google/Protobuf/Struct.php index 96b42af41..8e6191dc7 100644 --- 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; } diff --git a/php/src/Google/Protobuf/Descriptor.php b/php/src/Google/Protobuf/Descriptor.php index 986b81e12..36436e2b7 100644 --- 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(); + } } diff --git a/php/src/Google/Protobuf/FieldDescriptor.php b/php/src/Google/Protobuf/FieldDescriptor.php index ac9271f98..6d08cea9d 100644 --- 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(); + } } diff --git a/php/src/Google/Protobuf/Internal/Message.php b/php/src/Google/Protobuf/Internal/Message.php index 77614bc0e..c02d2b451 100644 --- 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; diff --git a/php/src/Google/Protobuf/NullValue.php b/php/src/Google/Protobuf/NullValue.php index a72cbb2ed..61569f8a3 100644 --- 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); diff --git a/php/src/Google/Protobuf/OneofDescriptor.php b/php/src/Google/Protobuf/OneofDescriptor.php index d9736634e..92b4e279d 100644 --- 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(); + } } diff --git a/php/src/Google/Protobuf/Value.php b/php/src/Google/Protobuf/Value.php index 5c1e864c3..20db3cc3e 100644 --- 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`. * diff --git a/php/tests/encode_decode_test.php b/php/tests/encode_decode_test.php index 5442f504d..ea8bd65d5 100644 --- 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(); diff --git a/php/tests/generate_protos.sh b/php/tests/generate_protos.sh index 0c2a5550a..e83c3c1c0 100755 --- 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 diff --git a/php/tests/generated_class_test.php b/php/tests/generated_class_test.php index 053697d2e..f49c4e970 100644 --- 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. ######################################################### diff --git a/php/tests/proto/test.proto b/php/tests/proto/test.proto index 95057090f..368b19ec4 100644 --- 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; diff --git a/php/tests/test.sh b/php/tests/test.sh index b10b57fd1..4beeed523 100755 --- 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.) diff --git a/php/tests/valgrind.supp b/php/tests/valgrind.supp new file mode 100644 index 000000000..e83b0a3df --- /dev/null +++ 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 +} diff --git a/src/google/protobuf/compiler/php/php_generator.cc b/src/google/protobuf/compiler/php/php_generator.cc index 4a6206a0f..a4b7139bd 100644 --- 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" diff --git a/src/google/protobuf/compiler/php/php_generator.h b/src/google/protobuf/compiler/php/php_generator.h index ca9d23a46..f67bb4041 100644 --- 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,