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.
pull/8670/head
Joshua Haberman 4 years ago
parent 769826e338
commit 75de6aa21a
  1. 5
      php/ext/google/protobuf/message.c
  2. 2
      php/src/Google/Protobuf/Api.php
  3. 2
      php/src/Google/Protobuf/Enum.php
  4. 2
      php/src/Google/Protobuf/Internal/DescriptorProto.php
  5. 2
      php/src/Google/Protobuf/Internal/DescriptorProto/ExtensionRange.php
  6. 2
      php/src/Google/Protobuf/Internal/EnumDescriptorProto.php
  7. 2
      php/src/Google/Protobuf/Internal/EnumValueDescriptorProto.php
  8. 2
      php/src/Google/Protobuf/Internal/FieldDescriptorProto.php
  9. 4
      php/src/Google/Protobuf/Internal/FileDescriptorProto.php
  10. 2
      php/src/Google/Protobuf/Internal/MethodDescriptorProto.php
  11. 2
      php/src/Google/Protobuf/Internal/OneofDescriptorProto.php
  12. 2
      php/src/Google/Protobuf/Internal/ServiceDescriptorProto.php
  13. 2
      php/src/Google/Protobuf/Option.php
  14. 2
      php/src/Google/Protobuf/Type.php
  15. 2
      php/tests/GeneratedClassTest.php
  16. 2
      php/tests/WellKnownTest.php
  17. 41
      src/google/protobuf/compiler/php/php_generator.cc

@ -134,6 +134,10 @@ static void Message_get(Message *intern, const upb_fielddef *f, zval *rv) {
RepeatedField_GetPhpWrapper(rv, msgval.array, TypeInfo_Get(f), RepeatedField_GetPhpWrapper(rv, msgval.array, TypeInfo_Get(f),
&intern->arena); &intern->arena);
} else { } 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); upb_msgval msgval = upb_msg_get(intern->msg, f);
Convert_UpbToPhp(msgval, rv, TypeInfo_Get(f), &intern->arena); 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() * Message_unset_property()
* *
* Object handler for unsetting a property. Called when PHP code calls: * Object handler for unsetting a property. Called when PHP code calls:
* does any of:
* *
* unset($message->foobar); * unset($message->foobar);
* *

@ -275,7 +275,7 @@ class Api extends \Google\Protobuf\Internal\Message
*/ */
public function getSourceContext() public function getSourceContext()
{ {
return isset($this->source_context) ? $this->source_context : null; return $this->source_context;
} }
public function hasSourceContext() public function hasSourceContext()

@ -155,7 +155,7 @@ class Enum extends \Google\Protobuf\Internal\Message
*/ */
public function getSourceContext() public function getSourceContext()
{ {
return isset($this->source_context) ? $this->source_context : null; return $this->source_context;
} }
public function hasSourceContext() public function hasSourceContext()

@ -256,7 +256,7 @@ class DescriptorProto extends \Google\Protobuf\Internal\Message
*/ */
public function getOptions() public function getOptions()
{ {
return isset($this->options) ? $this->options : null; return $this->options;
} }
public function hasOptions() public function hasOptions()

@ -128,7 +128,7 @@ class ExtensionRange extends \Google\Protobuf\Internal\Message
*/ */
public function getOptions() public function getOptions()
{ {
return isset($this->options) ? $this->options : null; return $this->options;
} }
public function hasOptions() public function hasOptions()

@ -128,7 +128,7 @@ class EnumDescriptorProto extends \Google\Protobuf\Internal\Message
*/ */
public function getOptions() public function getOptions()
{ {
return isset($this->options) ? $this->options : null; return $this->options;
} }
public function hasOptions() public function hasOptions()

@ -116,7 +116,7 @@ class EnumValueDescriptorProto extends \Google\Protobuf\Internal\Message
*/ */
public function getOptions() public function getOptions()
{ {
return isset($this->options) ? $this->options : null; return $this->options;
} }
public function hasOptions() public function hasOptions()

@ -515,7 +515,7 @@ class FieldDescriptorProto extends \Google\Protobuf\Internal\Message
*/ */
public function getOptions() public function getOptions()
{ {
return isset($this->options) ? $this->options : null; return $this->options;
} }
public function hasOptions() public function hasOptions()

@ -375,7 +375,7 @@ class FileDescriptorProto extends \Google\Protobuf\Internal\Message
*/ */
public function getOptions() public function getOptions()
{ {
return isset($this->options) ? $this->options : null; return $this->options;
} }
public function hasOptions() public function hasOptions()
@ -412,7 +412,7 @@ class FileDescriptorProto extends \Google\Protobuf\Internal\Message
*/ */
public function getSourceCodeInfo() public function getSourceCodeInfo()
{ {
return isset($this->source_code_info) ? $this->source_code_info : null; return $this->source_code_info;
} }
public function hasSourceCodeInfo() public function hasSourceCodeInfo()

@ -180,7 +180,7 @@ class MethodDescriptorProto extends \Google\Protobuf\Internal\Message
*/ */
public function getOptions() public function getOptions()
{ {
return isset($this->options) ? $this->options : null; return $this->options;
} }
public function hasOptions() public function hasOptions()

@ -79,7 +79,7 @@ class OneofDescriptorProto extends \Google\Protobuf\Internal\Message
*/ */
public function getOptions() public function getOptions()
{ {
return isset($this->options) ? $this->options : null; return $this->options;
} }
public function hasOptions() public function hasOptions()

@ -106,7 +106,7 @@ class ServiceDescriptorProto extends \Google\Protobuf\Internal\Message
*/ */
public function getOptions() public function getOptions()
{ {
return isset($this->options) ? $this->options : null; return $this->options;
} }
public function hasOptions() public function hasOptions()

@ -101,7 +101,7 @@ class Option extends \Google\Protobuf\Internal\Message
*/ */
public function getValue() public function getValue()
{ {
return isset($this->value) ? $this->value : null; return $this->value;
} }
public function hasValue() public function hasValue()

@ -189,7 +189,7 @@ class Type extends \Google\Protobuf\Internal\Message
*/ */
public function getSourceContext() public function getSourceContext()
{ {
return isset($this->source_context) ? $this->source_context : null; return $this->source_context;
} }
public function hasSourceContext() public function hasSourceContext()

@ -472,6 +472,8 @@ class GeneratedClassTest extends TestBase
{ {
$m = new TestMessage(); $m = new TestMessage();
$this->assertNull($m->getOptionalMessage());
$sub_m = new Sub(); $sub_m = new Sub();
$sub_m->setA(1); $sub_m->setA(1);
$m->setOptionalMessage($sub_m); $m->setOptionalMessage($sub_m);

@ -270,6 +270,8 @@ class WellKnownTest extends TestBase {
$m = new Value(); $m = new Value();
$this->assertNull($m->getStructValue());
$m->setNullValue(NullValue::NULL_VALUE); $m->setNullValue(NullValue::NULL_VALUE);
$this->assertSame(NullValue::NULL_VALUE, $m->getNullValue()); $this->assertSame(NullValue::NULL_VALUE, $m->getNullValue());
$this->assertSame("null_value", $m->getKind()); $this->assertSame("null_value", $m->getKind());

@ -648,32 +648,21 @@ void GenerateFieldAccessor(const FieldDescriptor* field, const Options& options,
std::string deprecation_trigger = (field->options().deprecated()) ? "@trigger_error('" + std::string deprecation_trigger = (field->options().deprecated()) ? "@trigger_error('" +
field->name() + " is deprecated.', E_USER_DEPRECATED);\n " : ""; field->name() + " is deprecated.', E_USER_DEPRECATED);\n " : "";
// Emit getter.
if (oneof != NULL) { if (oneof != NULL) {
printer->Print( printer->Print(
"public function get^camel_name^()\n" "public function get^camel_name^()\n"
"{\n" "{\n"
" ^deprecation_trigger^return $this->readOneof(^number^);\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", "}\n\n",
"camel_name", UnderscoresToCamelCase(field->name(), true), "camel_name", UnderscoresToCamelCase(field->name(), true),
"number", IntToString(field->number()), "number", IntToString(field->number()),
"deprecation_trigger", deprecation_trigger); "deprecation_trigger", deprecation_trigger);
} else if (field->has_presence()) { } else if (field->has_presence() && !field->message_type()) {
printer->Print( printer->Print(
"public function get^camel_name^()\n" "public function get^camel_name^()\n"
"{\n" "{\n"
" ^deprecation_trigger^return isset($this->^name^) ? $this->^name^ : ^default_value^;\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", "}\n\n",
"camel_name", UnderscoresToCamelCase(field->name(), true), "camel_name", UnderscoresToCamelCase(field->name(), true),
"name", field->name(), "name", field->name(),
@ -690,6 +679,32 @@ void GenerateFieldAccessor(const FieldDescriptor* field, const Options& options,
"deprecation_trigger", deprecation_trigger); "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 // For wrapper types, generate an additional getXXXUnwrapped getter
if (!field->is_map() && if (!field->is_map() &&
!field->is_repeated() && !field->is_repeated() &&

Loading…
Cancel
Save