Fix PhpDoc comments for message accessors to include "|null".

Message accessors will return null when when the field is not set, so this should be reflected in the PhpDoc. Also updated the code generator for the well-known types to reflect the edits made in protocolbuffers#8105. Also explicitly check for upb_msg_has() in the oneof accessor, so we are not implicitly relying on unset message fields returning NULL at the upb level.
haberman · Dec 11, 2020 · 5f844c7 · 5f844c7
1 parent 26c0fbc
commit 5f844c7
Show file tree

Hide file tree

Showing 16 changed files with 60 additions and 54 deletions.
diff --git a/php/ext/google/protobuf/message.c b/php/ext/google/protobuf/message.c
@@ -968,6 +968,10 @@ PHP_METHOD(Message, readOneof) {
                      (int)field_num);
   }
 
+  if (upb_fielddef_issubmsg(f) && !upb_msg_has(intern->msg, f)) {
+    RETURN_NULL();
+  }
+
   {
     upb_msgval msgval = upb_msg_get(intern->msg, f);
     const Descriptor *subdesc = Descriptor_GetFromFieldDef(f);

diff --git a/php/src/Google/Protobuf/Api.php b/php/src/Google/Protobuf/Api.php
diff --git a/php/src/Google/Protobuf/Enum.php b/php/src/Google/Protobuf/Enum.php
diff --git a/php/src/Google/Protobuf/Internal/DescriptorProto.php b/php/src/Google/Protobuf/Internal/DescriptorProto.php
diff --git a/php/src/Google/Protobuf/Internal/DescriptorProto/ExtensionRange.php b/php/src/Google/Protobuf/Internal/DescriptorProto/ExtensionRange.php
diff --git a/php/src/Google/Protobuf/Internal/EnumDescriptorProto.php b/php/src/Google/Protobuf/Internal/EnumDescriptorProto.php
diff --git a/php/src/Google/Protobuf/Internal/EnumValueDescriptorProto.php b/php/src/Google/Protobuf/Internal/EnumValueDescriptorProto.php
diff --git a/php/src/Google/Protobuf/Internal/FieldDescriptorProto.php b/php/src/Google/Protobuf/Internal/FieldDescriptorProto.php
diff --git a/php/src/Google/Protobuf/Internal/FileDescriptorProto.php b/php/src/Google/Protobuf/Internal/FileDescriptorProto.php
diff --git a/php/src/Google/Protobuf/Internal/MethodDescriptorProto.php b/php/src/Google/Protobuf/Internal/MethodDescriptorProto.php
diff --git a/php/src/Google/Protobuf/Internal/OneofDescriptorProto.php b/php/src/Google/Protobuf/Internal/OneofDescriptorProto.php
diff --git a/php/src/Google/Protobuf/Internal/ServiceDescriptorProto.php b/php/src/Google/Protobuf/Internal/ServiceDescriptorProto.php
diff --git a/php/src/Google/Protobuf/Option.php b/php/src/Google/Protobuf/Option.php
diff --git a/php/src/Google/Protobuf/Type.php b/php/src/Google/Protobuf/Type.php
diff --git a/php/src/Google/Protobuf/Value.php b/php/src/Google/Protobuf/Value.php
diff --git a/src/google/protobuf/compiler/php/php_generator.cc b/src/google/protobuf/compiler/php/php_generator.cc
@@ -95,7 +95,6 @@ std::string GeneratedMetadataFileName(const FileDescriptor* file,
                                       const Options& options);
 std::string UnderscoresToCamelCase(const std::string& name,
                                    bool cap_first_letter);
-std::string BinaryToHex(const std::string& binary);
 void Indent(io::Printer* printer);
 void Outdent(io::Printer* printer);
 void GenerateAddFilesToPool(const FileDescriptor* file, const Options& options,
@@ -600,27 +599,6 @@ std::string UnderscoresToCamelCase(const std::string& name,
   return result;
 }
 
-std::string BinaryToHex(const std::string& binary) {
-  std::string dest;
-  size_t i;
-  unsigned char symbol[16] = {
-    '0', '1', '2', '3',
-    '4', '5', '6', '7',
-    '8', '9', 'a', 'b',
-    'c', 'd', 'e', 'f',
-  };
-
-  dest.resize(binary.size() * 2);
-  char* append_ptr = &dest[0];
-
-  for (i = 0; i < binary.size(); i++) {
-    *append_ptr++ = symbol[(binary[i] & 0xf0) >> 4];
-    *append_ptr++ = symbol[binary[i] & 0x0f];
-  }
-
-  return dest;
-}
-
 void Indent(io::Printer* printer) {
   printer->Indent();
   printer->Indent();
@@ -1757,8 +1735,11 @@ void GenerateFieldDocComment(io::Printer* printer, const FieldDescriptor* field,
       "php_type", PhpSetterTypeName(field, options));
     printer->Print(" * @return $this\n");
   } else if (function_type == kFieldGetter) {
-    printer->Print(" * @return ^php_type^\n",
-      "php_type", PhpGetterTypeName(field, options));
+    bool can_return_null = field->has_presence() &&
+                           field->cpp_type() == FieldDescriptor::CPPTYPE_MESSAGE;
+    printer->Print(" * @return ^php_type^^maybe_null^\n",
+      "php_type", PhpGetterTypeName(field, options),
+      "maybe_null", can_return_null ? "|null" : "");
   }
   printer->Print(" */\n");
 }
@@ -1858,7 +1839,7 @@ void GenerateCEnum(const EnumDescriptor* desc, io::Printer* printer) {
       "  const upb_enumdef *e = upb_symtab_lookupenum(symtab, \"$name$\");\n"
       "  const char *name;\n"
       "  zend_long value;\n"
-      "  if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, \"l\", &value) ==\n"
+      "  if (zend_parse_parameters(ZEND_NUM_ARGS(), \"l\", &value) ==\n"
       "      FAILURE) {\n"
       "    return;\n"
       "  }\n"
@@ -1880,7 +1861,7 @@ void GenerateCEnum(const EnumDescriptor* desc, io::Printer* printer) {
       "  char *name = NULL;\n"
       "  size_t name_len;\n"
       "  int32_t num;\n"
-      "  if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, \"s\", &name,\n"
+      "  if (zend_parse_parameters(ZEND_NUM_ARGS(), \"s\", &name,\n"
       "                            &name_len) == FAILURE) {\n"
       "    return;\n"
       "  }\n"
@@ -1895,8 +1876,8 @@ void GenerateCEnum(const EnumDescriptor* desc, io::Printer* printer) {
       "}\n"
       "\n"
       "static zend_function_entry $c_name$_phpmethods[] = {\n"
-      "  PHP_ME($c_name$, name, NULL, ZEND_ACC_PUBLIC | ZEND_ACC_STATIC)\n"
-      "  PHP_ME($c_name$, value, NULL, ZEND_ACC_PUBLIC | ZEND_ACC_STATIC)\n"
+      "  PHP_ME($c_name$, name, arginfo_void, ZEND_ACC_PUBLIC | ZEND_ACC_STATIC)\n"
+      "  PHP_ME($c_name$, value, arginfo_void, ZEND_ACC_PUBLIC | ZEND_ACC_STATIC)\n"
       "  ZEND_FE_END\n"
       "};\n"
       "\n"
@@ -1990,24 +1971,45 @@ void GenerateCMessage(const Descriptor* message, io::Printer* printer) {
       "camel_name", UnderscoresToCamelCase(oneof->name(), true));
   }
 
+  switch (message->well_known_type()) {
+    case Descriptor::WELLKNOWNTYPE_ANY:
+      printer->Print(
+          "ZEND_BEGIN_ARG_INFO_EX(arginfo_is, 0, 0, 1)\n"
+          "  ZEND_ARG_INFO(0, proto)\n"
+          "ZEND_END_ARG_INFO()\n"
+          "\n"
+      );
+      break;
+    case Descriptor::WELLKNOWNTYPE_TIMESTAMP:
+      printer->Print(
+          "ZEND_BEGIN_ARG_INFO_EX(arginfo_timestamp_fromdatetime, 0, 0, 1)\n"
+          "  ZEND_ARG_INFO(0, datetime)\n"
+          "ZEND_END_ARG_INFO()\n"
+          "\n"
+      );
+      break;
+    default:
+      break;
+  }
+
   printer->Print(
       "static zend_function_entry $c_name$_phpmethods[] = {\n"
-      "  PHP_ME($c_name$, __construct, NULL, ZEND_ACC_PUBLIC)\n",
+      "  PHP_ME($c_name$, __construct, arginfo_void, ZEND_ACC_PUBLIC)\n",
       "c_name", c_name);
 
   for (int i = 0; i < message->field_count(); i++) {
     auto field = message->field(i);
     printer->Print(
-      "  PHP_ME($c_name$, get$camel_name$, NULL, ZEND_ACC_PUBLIC)\n"
-      "  PHP_ME($c_name$, set$camel_name$, NULL, ZEND_ACC_PUBLIC)\n",
+      "  PHP_ME($c_name$, get$camel_name$, arginfo_void, ZEND_ACC_PUBLIC)\n"
+      "  PHP_ME($c_name$, set$camel_name$, arginfo_setter, ZEND_ACC_PUBLIC)\n",
       "c_name", c_name,
       "camel_name", UnderscoresToCamelCase(field->name(), true));
   }
 
   for (int i = 0; i < message->real_oneof_decl_count(); i++) {
     auto oneof = message->oneof_decl(i);
     printer->Print(
-      "  PHP_ME($c_name$, get$camel_name$, NULL, ZEND_ACC_PUBLIC)\n",
+      "  PHP_ME($c_name$, get$camel_name$, arginfo_void, ZEND_ACC_PUBLIC)\n",
       "c_name", c_name,
       "camel_name", UnderscoresToCamelCase(oneof->name(), true));
   }
@@ -2016,15 +2018,15 @@ void GenerateCMessage(const Descriptor* message, io::Printer* printer) {
   switch (message->well_known_type()) {
     case Descriptor::WELLKNOWNTYPE_ANY:
       printer->Print(
-        "  PHP_ME($c_name$, is, NULL, ZEND_ACC_PUBLIC)\n"
-        "  PHP_ME($c_name$, pack, NULL, ZEND_ACC_PUBLIC)\n"
-        "  PHP_ME($c_name$, unpack, NULL, ZEND_ACC_PUBLIC)\n",
+        "  PHP_ME($c_name$, is, arginfo_is, ZEND_ACC_PUBLIC)\n"
+        "  PHP_ME($c_name$, pack, arginfo_setter, ZEND_ACC_PUBLIC)\n"
+        "  PHP_ME($c_name$, unpack, arginfo_void, ZEND_ACC_PUBLIC)\n",
         "c_name", c_name);
       break;
     case Descriptor::WELLKNOWNTYPE_TIMESTAMP:
       printer->Print(
-        "  PHP_ME($c_name$, fromDateTime, NULL, ZEND_ACC_PUBLIC)\n"
-        "  PHP_ME($c_name$, toDateTime, NULL, ZEND_ACC_PUBLIC)\n",
+        "  PHP_ME($c_name$, fromDateTime, arginfo_timestamp_fromdatetime, ZEND_ACC_PUBLIC)\n"
+        "  PHP_ME($c_name$, toDateTime, arginfo_void, ZEND_ACC_PUBLIC)\n",
         "c_name", c_name);
       break;
     default:
@@ -2154,7 +2156,7 @@ void GenerateCWellKnownTypes(const std::vector<const FileDescriptor*>& files,
         "}\n"
         "\n"
         "static zend_function_entry $metadata_c_name$_methods[] = {\n"
-        "  PHP_ME($metadata_c_name$, initOnce, NULL, ZEND_ACC_PUBLIC | ZEND_ACC_STATIC)\n"
+        "  PHP_ME($metadata_c_name$, initOnce, arginfo_void, ZEND_ACC_PUBLIC | ZEND_ACC_STATIC)\n"
         "  ZEND_FE_END\n"
         "};\n"
         "\n"