From 75de6aa21a1039251eb7f747f5b84164a90a8e5f Mon Sep 17 00:00:00 2001 From: Joshua Haberman Date: Thu, 27 May 2021 09:55:45 -0700 Subject: [PATCH] Fixed sub-message getters for well-known types when message is unset. The well-known types generate C code into wkt.inc, and this C code was not testing isset($msg->submsg_field) like the generated code does: ```php // PHP generated getter: checks isset(). public function getOptions() { return isset($this->options) ? $this->options : null; } ``` ```c // C generated getter, does not check upb_msg_has() static PHP_METHOD(google_protobuf_Value, getListValue) { Message* intern = (Message*)Z_OBJ_P(getThis()); const upb_fielddef *f = upb_msgdef_ntofz(intern->desc->msgdef, "list_value"); zval ret; Message_get(intern, f, &ret); RETURN_COPY_VALUE(&ret); } ``` This led to an error where we wnuld try to get a sub-message field from upb when it `upb_msg_has(msg, field) == false`, which is an error according to upb. There are two possible fixes for this bug. A guiding principle is that we want the generated C code in wkt.inc to have the same behavior as PHP generated code. Following this principle, the two possible fixes are: 1. Change the code generator for wkt.inc to check upb_msg_has(f) before calling Message_get(). This would match the isset() check that the The PHP generated code does, and we would leave the PHP code unchanged. 2. Change Message_get() to itself perform the upb_msg_has(f) check for sub-message fields. This means that generated code would no longer need to perform an isset() check, so we would want to remove this check from the PHP generated code also to avoid a redundant check. Both of these are reasonable fixes, and it is not immediately obvious which is better. (1) has the benefit of resolving this case when we are in more specialized code (a getter function that already knows this is a sub-message field), and therefore avoids performing the check later in more generic code that would have to test the type again. On the other hand, the isset() check is not needed for the pure PHP implementation, as an unset PHP variable will return `null` anyway. And for the C extension, we'd rather check upb_msg_has() at the C level instead of PHP. So this change implements (2). The generated code in wkt.inc remains unchanged, and the PHP generated code for sub-message fields is changed to remove the isset() check. --- php/ext/google/protobuf/message.c | 5 ++- php/src/Google/Protobuf/Api.php | 2 +- php/src/Google/Protobuf/Enum.php | 2 +- .../Protobuf/Internal/DescriptorProto.php | 2 +- .../DescriptorProto/ExtensionRange.php | 2 +- .../Protobuf/Internal/EnumDescriptorProto.php | 2 +- .../Internal/EnumValueDescriptorProto.php | 2 +- .../Internal/FieldDescriptorProto.php | 2 +- .../Protobuf/Internal/FileDescriptorProto.php | 4 +- .../Internal/MethodDescriptorProto.php | 2 +- .../Internal/OneofDescriptorProto.php | 2 +- .../Internal/ServiceDescriptorProto.php | 2 +- php/src/Google/Protobuf/Option.php | 2 +- php/src/Google/Protobuf/Type.php | 2 +- php/tests/GeneratedClassTest.php | 2 + php/tests/WellKnownTest.php | 2 + .../protobuf/compiler/php/php_generator.cc | 41 +++++++++++++------ 17 files changed, 50 insertions(+), 28 deletions(-) diff --git a/php/ext/google/protobuf/message.c b/php/ext/google/protobuf/message.c index b32b325555dc..7a1aefa6f729 100644 --- a/php/ext/google/protobuf/message.c +++ b/php/ext/google/protobuf/message.c @@ -134,6 +134,10 @@ static void Message_get(Message *intern, const upb_fielddef *f, zval *rv) { RepeatedField_GetPhpWrapper(rv, msgval.array, TypeInfo_Get(f), &intern->arena); } else { + if (upb_fielddef_issubmsg(f) && !upb_msg_has(intern->msg, f)) { + ZVAL_NULL(rv); + return; + } upb_msgval msgval = upb_msg_get(intern->msg, f); Convert_UpbToPhp(msgval, rv, TypeInfo_Get(f), &intern->arena); } @@ -280,7 +284,6 @@ static int Message_has_property(PROTO_VAL *obj, PROTO_STR *member, * Message_unset_property() * * Object handler for unsetting a property. Called when PHP code calls: - * does any of: * * unset($message->foobar); * diff --git a/php/src/Google/Protobuf/Api.php b/php/src/Google/Protobuf/Api.php index 7cbb30eb4284..e7d76c01d6cf 100644 --- a/php/src/Google/Protobuf/Api.php +++ b/php/src/Google/Protobuf/Api.php @@ -275,7 +275,7 @@ public function setVersion($var) */ public function getSourceContext() { - return isset($this->source_context) ? $this->source_context : null; + return $this->source_context; } public function hasSourceContext() diff --git a/php/src/Google/Protobuf/Enum.php b/php/src/Google/Protobuf/Enum.php index 2e0ac9987b6b..e803e930d8f2 100644 --- a/php/src/Google/Protobuf/Enum.php +++ b/php/src/Google/Protobuf/Enum.php @@ -155,7 +155,7 @@ public function setOptions($var) */ public function getSourceContext() { - return isset($this->source_context) ? $this->source_context : null; + return $this->source_context; } public function hasSourceContext() diff --git a/php/src/Google/Protobuf/Internal/DescriptorProto.php b/php/src/Google/Protobuf/Internal/DescriptorProto.php index ff308e7eeb14..c58c5739e57f 100644 --- a/php/src/Google/Protobuf/Internal/DescriptorProto.php +++ b/php/src/Google/Protobuf/Internal/DescriptorProto.php @@ -256,7 +256,7 @@ public function setOneofDecl($var) */ public function getOptions() { - return isset($this->options) ? $this->options : null; + return $this->options; } public function hasOptions() diff --git a/php/src/Google/Protobuf/Internal/DescriptorProto/ExtensionRange.php b/php/src/Google/Protobuf/Internal/DescriptorProto/ExtensionRange.php index bbe4a6a84f9f..43c33c4a9103 100644 --- a/php/src/Google/Protobuf/Internal/DescriptorProto/ExtensionRange.php +++ b/php/src/Google/Protobuf/Internal/DescriptorProto/ExtensionRange.php @@ -128,7 +128,7 @@ public function setEnd($var) */ public function getOptions() { - return isset($this->options) ? $this->options : null; + return $this->options; } public function hasOptions() diff --git a/php/src/Google/Protobuf/Internal/EnumDescriptorProto.php b/php/src/Google/Protobuf/Internal/EnumDescriptorProto.php index b9b634282906..bd50834f8183 100644 --- a/php/src/Google/Protobuf/Internal/EnumDescriptorProto.php +++ b/php/src/Google/Protobuf/Internal/EnumDescriptorProto.php @@ -128,7 +128,7 @@ public function setValue($var) */ public function getOptions() { - return isset($this->options) ? $this->options : null; + return $this->options; } public function hasOptions() diff --git a/php/src/Google/Protobuf/Internal/EnumValueDescriptorProto.php b/php/src/Google/Protobuf/Internal/EnumValueDescriptorProto.php index eff1452eed4e..0feaea6f182f 100644 --- a/php/src/Google/Protobuf/Internal/EnumValueDescriptorProto.php +++ b/php/src/Google/Protobuf/Internal/EnumValueDescriptorProto.php @@ -116,7 +116,7 @@ public function setNumber($var) */ public function getOptions() { - return isset($this->options) ? $this->options : null; + return $this->options; } public function hasOptions() diff --git a/php/src/Google/Protobuf/Internal/FieldDescriptorProto.php b/php/src/Google/Protobuf/Internal/FieldDescriptorProto.php index 94e5fe12ecc9..9cf6f10cfd2a 100644 --- a/php/src/Google/Protobuf/Internal/FieldDescriptorProto.php +++ b/php/src/Google/Protobuf/Internal/FieldDescriptorProto.php @@ -515,7 +515,7 @@ public function setJsonName($var) */ public function getOptions() { - return isset($this->options) ? $this->options : null; + return $this->options; } public function hasOptions() diff --git a/php/src/Google/Protobuf/Internal/FileDescriptorProto.php b/php/src/Google/Protobuf/Internal/FileDescriptorProto.php index d96c7a78ecc5..435bd5fb6f06 100644 --- a/php/src/Google/Protobuf/Internal/FileDescriptorProto.php +++ b/php/src/Google/Protobuf/Internal/FileDescriptorProto.php @@ -375,7 +375,7 @@ public function setExtension($var) */ public function getOptions() { - return isset($this->options) ? $this->options : null; + return $this->options; } public function hasOptions() @@ -412,7 +412,7 @@ public function setOptions($var) */ public function getSourceCodeInfo() { - return isset($this->source_code_info) ? $this->source_code_info : null; + return $this->source_code_info; } public function hasSourceCodeInfo() diff --git a/php/src/Google/Protobuf/Internal/MethodDescriptorProto.php b/php/src/Google/Protobuf/Internal/MethodDescriptorProto.php index 5814f0885296..96efb02d24a1 100644 --- a/php/src/Google/Protobuf/Internal/MethodDescriptorProto.php +++ b/php/src/Google/Protobuf/Internal/MethodDescriptorProto.php @@ -180,7 +180,7 @@ public function setOutputType($var) */ public function getOptions() { - return isset($this->options) ? $this->options : null; + return $this->options; } public function hasOptions() diff --git a/php/src/Google/Protobuf/Internal/OneofDescriptorProto.php b/php/src/Google/Protobuf/Internal/OneofDescriptorProto.php index 33cf487a8a2b..3cb9f25c212d 100644 --- a/php/src/Google/Protobuf/Internal/OneofDescriptorProto.php +++ b/php/src/Google/Protobuf/Internal/OneofDescriptorProto.php @@ -79,7 +79,7 @@ public function setName($var) */ public function getOptions() { - return isset($this->options) ? $this->options : null; + return $this->options; } public function hasOptions() diff --git a/php/src/Google/Protobuf/Internal/ServiceDescriptorProto.php b/php/src/Google/Protobuf/Internal/ServiceDescriptorProto.php index f60561c1822c..c511247692cb 100644 --- a/php/src/Google/Protobuf/Internal/ServiceDescriptorProto.php +++ b/php/src/Google/Protobuf/Internal/ServiceDescriptorProto.php @@ -106,7 +106,7 @@ public function setMethod($var) */ public function getOptions() { - return isset($this->options) ? $this->options : null; + return $this->options; } public function hasOptions() diff --git a/php/src/Google/Protobuf/Option.php b/php/src/Google/Protobuf/Option.php index 5166a08db615..31249e513114 100644 --- a/php/src/Google/Protobuf/Option.php +++ b/php/src/Google/Protobuf/Option.php @@ -101,7 +101,7 @@ public function setName($var) */ public function getValue() { - return isset($this->value) ? $this->value : null; + return $this->value; } public function hasValue() diff --git a/php/src/Google/Protobuf/Type.php b/php/src/Google/Protobuf/Type.php index 3f2835927396..41b9e3602f84 100644 --- a/php/src/Google/Protobuf/Type.php +++ b/php/src/Google/Protobuf/Type.php @@ -189,7 +189,7 @@ public function setOptions($var) */ public function getSourceContext() { - return isset($this->source_context) ? $this->source_context : null; + return $this->source_context; } public function hasSourceContext() diff --git a/php/tests/GeneratedClassTest.php b/php/tests/GeneratedClassTest.php index 5c0f0c70d0ee..540105c376b7 100644 --- a/php/tests/GeneratedClassTest.php +++ b/php/tests/GeneratedClassTest.php @@ -472,6 +472,8 @@ public function testMessageField() { $m = new TestMessage(); + $this->assertNull($m->getOptionalMessage()); + $sub_m = new Sub(); $sub_m->setA(1); $m->setOptionalMessage($sub_m); diff --git a/php/tests/WellKnownTest.php b/php/tests/WellKnownTest.php index 27b7e1463cec..486c65ffca8e 100644 --- a/php/tests/WellKnownTest.php +++ b/php/tests/WellKnownTest.php @@ -270,6 +270,8 @@ public function testStruct() $m = new Value(); + $this->assertNull($m->getStructValue()); + $m->setNullValue(NullValue::NULL_VALUE); $this->assertSame(NullValue::NULL_VALUE, $m->getNullValue()); $this->assertSame("null_value", $m->getKind()); diff --git a/src/google/protobuf/compiler/php/php_generator.cc b/src/google/protobuf/compiler/php/php_generator.cc index 45eb4c619637..f351ccd2a48e 100644 --- a/src/google/protobuf/compiler/php/php_generator.cc +++ b/src/google/protobuf/compiler/php/php_generator.cc @@ -648,32 +648,21 @@ void GenerateFieldAccessor(const FieldDescriptor* field, const Options& options, std::string deprecation_trigger = (field->options().deprecated()) ? "@trigger_error('" + field->name() + " is deprecated.', E_USER_DEPRECATED);\n " : ""; + // Emit getter. if (oneof != NULL) { printer->Print( "public function get^camel_name^()\n" "{\n" " ^deprecation_trigger^return $this->readOneof(^number^);\n" - "}\n\n" - "public function has^camel_name^()\n" - "{\n" - " ^deprecation_trigger^return $this->hasOneof(^number^);\n" "}\n\n", "camel_name", UnderscoresToCamelCase(field->name(), true), "number", IntToString(field->number()), "deprecation_trigger", deprecation_trigger); - } else if (field->has_presence()) { + } else if (field->has_presence() && !field->message_type()) { printer->Print( "public function get^camel_name^()\n" "{\n" " ^deprecation_trigger^return isset($this->^name^) ? $this->^name^ : ^default_value^;\n" - "}\n\n" - "public function has^camel_name^()\n" - "{\n" - " ^deprecation_trigger^return isset($this->^name^);\n" - "}\n\n" - "public function clear^camel_name^()\n" - "{\n" - " ^deprecation_trigger^unset($this->^name^);\n" "}\n\n", "camel_name", UnderscoresToCamelCase(field->name(), true), "name", field->name(), @@ -690,6 +679,32 @@ void GenerateFieldAccessor(const FieldDescriptor* field, const Options& options, "deprecation_trigger", deprecation_trigger); } + // Emit hazzers/clear. + if (oneof) { + printer->Print( + "public function has^camel_name^()\n" + "{\n" + " ^deprecation_trigger^return $this->hasOneof(^number^);\n" + "}\n\n", + "camel_name", UnderscoresToCamelCase(field->name(), true), + "number", IntToString(field->number()), + "deprecation_trigger", deprecation_trigger); + } else if (field->has_presence()) { + printer->Print( + "public function has^camel_name^()\n" + "{\n" + " ^deprecation_trigger^return isset($this->^name^);\n" + "}\n\n" + "public function clear^camel_name^()\n" + "{\n" + " ^deprecation_trigger^unset($this->^name^);\n" + "}\n\n", + "camel_name", UnderscoresToCamelCase(field->name(), true), + "name", field->name(), + "default_value", DefaultForField(field), + "deprecation_trigger", deprecation_trigger); + } + // For wrapper types, generate an additional getXXXUnwrapped getter if (!field->is_map() && !field->is_repeated() &&