From 7be088202bad3a89498db2e9b19afda9f3929430 Mon Sep 17 00:00:00 2001 From: Paul Yang Date: Wed, 19 Apr 2017 20:03:34 -0700 Subject: Enum defined without package have incorrect class name. (#2988) Fix the bug by sharing the code for generating class name for both message and enum. --- php/tests/generated_class_test.php | 12 ++-- php/tests/memory_leak_test.php | 3 + php/tests/proto/test.proto | 6 ++ php/tests/proto/test_no_namespace.proto | 4 +- src/google/protobuf/compiler/php/php_generator.cc | 82 ++++++++++------------- 5 files changed, 54 insertions(+), 53 deletions(-) diff --git a/php/tests/generated_class_test.php b/php/tests/generated_class_test.php index 39e6c6c4..41d63a60 100644 --- a/php/tests/generated_class_test.php +++ b/php/tests/generated_class_test.php @@ -1,7 +1,7 @@ setOptionalNoNamespaceMessage(new NoNameSpaceMessage()); + $m->getRepeatedNoNamespaceMessage()[] = new NoNameSpaceMessage(); } public function testEnumWithoutNamespace() { - $m = new NoNameSpaceEnum(); + $m = new TestMessage(); + $m->setOptionalNoNamespaceEnum(NoNameSpaceEnum::VALUE_A); + $m->getRepeatedNoNamespaceEnum()[] = NoNameSpaceEnum::VALUE_A; } ######################################################### diff --git a/php/tests/memory_leak_test.php b/php/tests/memory_leak_test.php index 68b6f5be..6b7077f0 100644 --- a/php/tests/memory_leak_test.php +++ b/php/tests/memory_leak_test.php @@ -2,6 +2,8 @@ # phpunit has memory leak by itself. Thus, it cannot be used to test memory leak. +require_once('generated/NoNamespaceEnum.php'); +require_once('generated/NoNamespaceMessage.php'); require_once('generated/PrefixTestPrefix.php'); require_once('generated/Bar/TestInclude.php'); require_once('generated/Foo/TestEnum.php'); @@ -13,6 +15,7 @@ require_once('generated/Foo/TestPhpDoc.php'); require_once('generated/Foo/TestUnpackedMessage.php'); require_once('generated/GPBMetadata/Proto/Test.php'); require_once('generated/GPBMetadata/Proto/TestInclude.php'); +require_once('generated/GPBMetadata/Proto/TestNoNamespace.php'); require_once('generated/GPBMetadata/Proto/TestPrefix.php'); require_once('test_util.php'); diff --git a/php/tests/proto/test.proto b/php/tests/proto/test.proto index 1a47a3f2..cc9bf8c8 100644 --- a/php/tests/proto/test.proto +++ b/php/tests/proto/test.proto @@ -1,6 +1,7 @@ syntax = "proto3"; import 'proto/test_include.proto'; +import 'proto/test_no_namespace.proto'; import 'proto/test_prefix.proto'; package foo; @@ -96,6 +97,11 @@ message TestMessage { // Reserved for non-existing field test. // int32 non_exist = 89; + + NoNamespaceMessage optional_no_namespace_message = 91; + NoNamespaceEnum optional_no_namespace_enum = 92; + repeated NoNamespaceMessage repeated_no_namespace_message = 93; + repeated NoNamespaceEnum repeated_no_namespace_enum = 94; } enum TestEnum { diff --git a/php/tests/proto/test_no_namespace.proto b/php/tests/proto/test_no_namespace.proto index b8c4fdf2..b0f66002 100644 --- a/php/tests/proto/test_no_namespace.proto +++ b/php/tests/proto/test_no_namespace.proto @@ -1,10 +1,10 @@ syntax = "proto3"; -message NoNameSpaceMessage { +message NoNamespaceMessage { int32 a = 1; } -enum NoNameSpaceEnum { +enum NoNamespaceEnum { VALUE_A = 0; VALUE_B = 1; } diff --git a/src/google/protobuf/compiler/php/php_generator.cc b/src/google/protobuf/compiler/php/php_generator.cc index db72ea1a..32f40b2e 100644 --- a/src/google/protobuf/compiler/php/php_generator.cc +++ b/src/google/protobuf/compiler/php/php_generator.cc @@ -84,33 +84,6 @@ std::string RenameEmpty(const std::string& name) { } } -std::string MessagePrefix(const Descriptor* message) { - // Empty cannot be php class name. - if (message->name() == "Empty" && - message->file()->package() == "google.protobuf") { - return "GPB"; - } else { - return (message->file()->options()).php_class_prefix(); - } -} - -std::string MessageName(const Descriptor* message, bool is_descriptor) { - string message_name = message->name(); - const Descriptor* descriptor = message->containing_type(); - while (descriptor != NULL) { - message_name = descriptor->name() + '_' + message_name; - descriptor = descriptor->containing_type(); - } - message_name = MessagePrefix(message) + message_name; - - if (message->file()->package() == "") { - return message_name; - } else { - return PhpName(message->file()->package(), is_descriptor) + '\\' + - message_name; - } -} - std::string MessageFullName(const Descriptor* message, bool is_descriptor) { if (is_descriptor) { return StringReplace(message->full_name(), @@ -131,19 +104,34 @@ std::string EnumFullName(const EnumDescriptor* envm, bool is_descriptor) { } } -std::string EnumClassName(const EnumDescriptor* envm) { - string enum_class_name = envm->name(); - const Descriptor* descriptor = envm->containing_type(); - while (descriptor != NULL) { - enum_class_name = descriptor->name() + '_' + enum_class_name; - descriptor = descriptor->containing_type(); +template +std::string ClassNamePrefix(const DescriptorType* desc) { + // Empty cannot be php class name. + if (desc->name() == "Empty" && + desc->file()->package() == "google.protobuf") { + return "GPB"; + } else { + return (desc->file()->options()).php_class_prefix(); } - return enum_class_name; } -std::string EnumName(const EnumDescriptor* envm, bool is_descriptor) { - string enum_name = EnumClassName(envm); - return PhpName(envm->file()->package(), is_descriptor) + '\\' + enum_name; + +template +std::string FullClassName(const DescriptorType* desc, bool is_descriptor) { + string classname = desc->name(); + const Descriptor* containing = desc->containing_type(); + while (containing != NULL) { + classname = containing->name() + '_' + classname; + containing = containing->containing_type(); + } + classname = ClassNamePrefix(desc) + classname; + + if (desc->file()->package() == "") { + return classname; + } else { + return PhpName(desc->file()->package(), is_descriptor) + '\\' + + classname; + } } std::string PhpName(const std::string& full_name, bool is_descriptor) { @@ -231,7 +219,7 @@ std::string GeneratedMetadataFileName(const std::string& proto_file, std::string GeneratedMessageFileName(const Descriptor* message, bool is_descriptor) { - std::string result = MessageName(message, is_descriptor); + std::string result = FullClassName(message, is_descriptor); for (int i = 0; i < result.size(); i++) { if (result[i] == '\\') { result[i] = '/'; @@ -242,7 +230,7 @@ std::string GeneratedMessageFileName(const Descriptor* message, std::string GeneratedEnumFileName(const EnumDescriptor* en, bool is_descriptor) { - std::string result = EnumName(en, is_descriptor); + std::string result = FullClassName(en, is_descriptor); for (int i = 0; i < result.size(); i++) { if (result[i] == '\\') { result[i] = '/'; @@ -456,12 +444,12 @@ void GenerateFieldAccessor(const FieldDescriptor* field, bool is_descriptor, printer->Print( ", \\^class_name^);\n", "class_name", - MessageName(value->message_type(), is_descriptor) + "::class"); + FullClassName(value->message_type(), is_descriptor) + "::class"); } else if (value->cpp_type() == FieldDescriptor::CPPTYPE_ENUM) { printer->Print( - ", ^class_name^);\n", + ", \\^class_name^);\n", "class_name", - EnumName(value->enum_type(), is_descriptor) + "::class"); + FullClassName(value->enum_type(), is_descriptor) + "::class"); } else { printer->Print(");\n"); } @@ -474,23 +462,23 @@ void GenerateFieldAccessor(const FieldDescriptor* field, bool is_descriptor, printer->Print( ", \\^class_name^);\n", "class_name", - MessageName(field->message_type(), is_descriptor) + "::class"); + FullClassName(field->message_type(), is_descriptor) + "::class"); } else if (field->cpp_type() == FieldDescriptor::CPPTYPE_ENUM) { printer->Print( - ", ^class_name^);\n", + ", \\^class_name^);\n", "class_name", - EnumName(field->enum_type(), is_descriptor) + "::class"); + FullClassName(field->enum_type(), is_descriptor) + "::class"); } else { printer->Print(");\n"); } } else if (field->cpp_type() == FieldDescriptor::CPPTYPE_MESSAGE) { printer->Print( "GPBUtil::checkMessage($var, \\^class_name^::class);\n", - "class_name", MessageName(field->message_type(), is_descriptor)); + "class_name", FullClassName(field->message_type(), is_descriptor)); } else if (field->cpp_type() == FieldDescriptor::CPPTYPE_ENUM) { printer->Print( "GPBUtil::checkEnum($var, \\^class_name^::class);\n", - "class_name", EnumName(field->enum_type(), is_descriptor)); + "class_name", FullClassName(field->enum_type(), is_descriptor)); } else if (field->cpp_type() == FieldDescriptor::CPPTYPE_STRING) { printer->Print( "GPBUtil::checkString($var, ^utf8^);\n", -- cgit v1.2.3