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.
diff --git a/php/ext/google/protobuf/message.c b/php/ext/google/protobuf/message.c
index b32b325..7a1aefa 100644
--- a/php/ext/google/protobuf/message.c
+++ b/php/ext/google/protobuf/message.c
@@ -134,6 +134,10 @@
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 @@
* 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 7cbb30e..e7d76c0 100644
--- a/php/src/Google/Protobuf/Api.php
+++ b/php/src/Google/Protobuf/Api.php
@@ -275,7 +275,7 @@
*/
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 2e0ac99..e803e93 100644
--- a/php/src/Google/Protobuf/Enum.php
+++ b/php/src/Google/Protobuf/Enum.php
@@ -155,7 +155,7 @@
*/
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 ff308e7..c58c573 100644
--- a/php/src/Google/Protobuf/Internal/DescriptorProto.php
+++ b/php/src/Google/Protobuf/Internal/DescriptorProto.php
@@ -256,7 +256,7 @@
*/
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 bbe4a6a..43c33c4 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 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 b9b6342..bd50834 100644
--- a/php/src/Google/Protobuf/Internal/EnumDescriptorProto.php
+++ b/php/src/Google/Protobuf/Internal/EnumDescriptorProto.php
@@ -128,7 +128,7 @@
*/
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 eff1452..0feaea6 100644
--- a/php/src/Google/Protobuf/Internal/EnumValueDescriptorProto.php
+++ b/php/src/Google/Protobuf/Internal/EnumValueDescriptorProto.php
@@ -116,7 +116,7 @@
*/
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 94e5fe1..9cf6f10 100644
--- a/php/src/Google/Protobuf/Internal/FieldDescriptorProto.php
+++ b/php/src/Google/Protobuf/Internal/FieldDescriptorProto.php
@@ -515,7 +515,7 @@
*/
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 d96c7a7..435bd5f 100644
--- a/php/src/Google/Protobuf/Internal/FileDescriptorProto.php
+++ b/php/src/Google/Protobuf/Internal/FileDescriptorProto.php
@@ -375,7 +375,7 @@
*/
public function getOptions()
{
- return isset($this->options) ? $this->options : null;
+ return $this->options;
}
public function hasOptions()
@@ -412,7 +412,7 @@
*/
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 5814f08..96efb02 100644
--- a/php/src/Google/Protobuf/Internal/MethodDescriptorProto.php
+++ b/php/src/Google/Protobuf/Internal/MethodDescriptorProto.php
@@ -180,7 +180,7 @@
*/
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 33cf487..3cb9f25 100644
--- a/php/src/Google/Protobuf/Internal/OneofDescriptorProto.php
+++ b/php/src/Google/Protobuf/Internal/OneofDescriptorProto.php
@@ -79,7 +79,7 @@
*/
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 f60561c..c511247 100644
--- a/php/src/Google/Protobuf/Internal/ServiceDescriptorProto.php
+++ b/php/src/Google/Protobuf/Internal/ServiceDescriptorProto.php
@@ -106,7 +106,7 @@
*/
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 5166a08..31249e5 100644
--- a/php/src/Google/Protobuf/Option.php
+++ b/php/src/Google/Protobuf/Option.php
@@ -101,7 +101,7 @@
*/
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 3f28359..41b9e36 100644
--- a/php/src/Google/Protobuf/Type.php
+++ b/php/src/Google/Protobuf/Type.php
@@ -189,7 +189,7 @@
*/
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 5c0f0c7..540105c 100644
--- a/php/tests/GeneratedClassTest.php
+++ b/php/tests/GeneratedClassTest.php
@@ -472,6 +472,8 @@
{
$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 27b7e14..486c65f 100644
--- a/php/tests/WellKnownTest.php
+++ b/php/tests/WellKnownTest.php
@@ -270,6 +270,8 @@
$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 45eb4c6..f351ccd 100644
--- a/src/google/protobuf/compiler/php/php_generator.cc
+++ b/src/google/protobuf/compiler/php/php_generator.cc
@@ -648,32 +648,21 @@
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 @@
"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() &&
