From 6d84da5e2d42a8c83373c0bc6d7ed0ca6f78aade Mon Sep 17 00:00:00 2001 From: Brent Shaffer Date: Tue, 20 Aug 2024 07:05:09 -0700 Subject: [PATCH] fix: do not throw deprecated warning on field getters for default values (#17788) fixes https://github.com/protocolbuffers/protobuf/issues/13428 Wraps deprecated field getters in a conditional to see if the value has been set before throwing the warning. This is because this method is used internally, so users can get deprecated warnings even when they are not using the field. Closes #17788 COPYBARA_INTEGRATE_REVIEW=https://github.com/protocolbuffers/protobuf/pull/17788 from bshaffer:wrap-deprecation-in-conditional 084c87dc12da44000900364218269db3ac845601 PiperOrigin-RevId: 665346195 --- php/tests/GeneratedClassTest.php | 86 +++++++++ php/tests/proto/test.proto | 177 ++++++++++-------- .../protobuf/compiler/php/php_generator.cc | 49 ++++- 3 files changed, 223 insertions(+), 89 deletions(-) diff --git a/php/tests/GeneratedClassTest.php b/php/tests/GeneratedClassTest.php index 11b11e34c9cb..7683b7526825 100644 --- a/php/tests/GeneratedClassTest.php +++ b/php/tests/GeneratedClassTest.php @@ -101,6 +101,92 @@ public function testDeprecatedInt32Field() $this->assertSame(4, $deprecationCount); } + public function testDeprecatedFieldGetterDoesNotThrowWarning() + { + // temporarily change error handler to capture the deprecated errors + $deprecationCount = 0; + set_error_handler(function ($errno, $errstr) use (&$deprecationCount) { + if (false !== strpos($errstr, ' is deprecated.')) { + $deprecationCount++; + } + }, E_USER_DEPRECATED); + + // does not throw warning + $message = new TestMessage(); + $message->getDeprecatedInt32(); + $message->getDeprecatedOptionalInt32(); + $message->getDeprecatedInt32ValueUnwrapped(); // wrapped field + $message->getDeprecatedInt32Value(); // wrapped field + $message->getDeprecatedOneofInt32(); // oneof field + $message->getDeprecatedOneof(); // oneof field + $message->getDeprecatedRepeatedInt32(); // repeated field + $message->getDeprecatedMapInt32Int32(); // map field + $message->getDeprecatedAny(); // any field + $message->getDeprecatedMessage(); // message field + $message->getDeprecatedEnum(); // enum field + + restore_error_handler(); + + $this->assertEquals(0, $deprecationCount); + } + + public function testDeprecatedFieldGetterThrowsWarningWithValue() + { + $message = new TestMessage([ + 'deprecated_int32' => 1, + 'deprecated_optional_int32' => 1, + 'deprecated_int32_value' => new \Google\Protobuf\Int32Value(['value' => 1]), + 'deprecated_oneof_int32' => 1, + 'deprecated_repeated_int32' => [1], + 'deprecated_map_int32_int32' => [1 => 1], + 'deprecated_any' => new \Google\Protobuf\Any(['type_url' => 'foo', 'value' => 'bar']), + 'deprecated_message' => new TestMessage(), + 'deprecated_enum' => 1, + ]); + + // temporarily change error handler to capture the deprecated errors + $deprecationCount = 0; + set_error_handler(function ($errno, $errstr) use (&$deprecationCount) { + if (false !== strpos($errstr, ' is deprecated.')) { + $deprecationCount++; + } + }, E_USER_DEPRECATED); + + $message->getDeprecatedInt32(); + $message->getDeprecatedOptionalInt32(); + $message->getDeprecatedInt32ValueUnwrapped(); // wrapped field unwrapped + $message->getDeprecatedInt32Value(); // wrapped field + $message->getDeprecatedOneofInt32(); // oneof field + $message->getDeprecatedRepeatedInt32(); // repeated field + $message->getDeprecatedMapInt32Int32(); // map field + $message->getDeprecatedAny(); // any field + $message->getDeprecatedMessage(); // message field + $message->getDeprecatedEnum(); // enum field + + // oneof field (should never warn) + $message->getDeprecatedOneof(); + + restore_error_handler(); + + $this->assertEquals(10, $deprecationCount); + } + + public function testDeprecatedFieldWarningsOnSerialize() + { + set_error_handler(function ($errno, $errstr) { + if (false !== strpos($errstr, ' is deprecated.')) { + throw new \Exception($errstr); + } + }, E_USER_DEPRECATED); + + $message = new TestMessage(); + $message->serializeToJsonString(); + + restore_error_handler(); + + $this->assertTrue(true, 'No deprecation warning on serialize'); + } + ######################################################### # Test optional int32 field. ######################################################### diff --git a/php/tests/proto/test.proto b/php/tests/proto/test.proto index 4217800507b0..d385bb7bb39c 100644 --- a/php/tests/proto/test.proto +++ b/php/tests/proto/test.proto @@ -1,16 +1,16 @@ syntax = "proto3"; +package foo; + import 'google/protobuf/any.proto'; -import 'google/protobuf/wrappers.proto'; import 'google/protobuf/struct.proto'; +import 'google/protobuf/wrappers.proto'; +import 'proto/test_empty_php_namespace.proto'; import 'proto/test_include.proto'; import 'proto/test_no_namespace.proto'; import 'proto/test_php_namespace.proto'; -import 'proto/test_empty_php_namespace.proto'; import 'proto/test_prefix.proto'; -package foo; - message TestMessage { // Singular int32 optional_int32 = 1; @@ -56,63 +56,63 @@ message TestMessage { optional bar.TestInclude true_optional_included_message = 218; // Repeated - repeated int32 repeated_int32 = 31; - repeated int64 repeated_int64 = 32; - repeated uint32 repeated_uint32 = 33; - repeated uint64 repeated_uint64 = 34; - repeated sint32 repeated_sint32 = 35; - repeated sint64 repeated_sint64 = 36; - repeated fixed32 repeated_fixed32 = 37; - repeated fixed64 repeated_fixed64 = 38; + repeated int32 repeated_int32 = 31; + repeated int64 repeated_int64 = 32; + repeated uint32 repeated_uint32 = 33; + repeated uint64 repeated_uint64 = 34; + repeated sint32 repeated_sint32 = 35; + repeated sint64 repeated_sint64 = 36; + repeated fixed32 repeated_fixed32 = 37; + repeated fixed64 repeated_fixed64 = 38; repeated sfixed32 repeated_sfixed32 = 39; repeated sfixed64 repeated_sfixed64 = 40; - repeated float repeated_float = 41; - repeated double repeated_double = 42; - repeated bool repeated_bool = 43; - repeated string repeated_string = 44; - repeated bytes repeated_bytes = 45; + repeated float repeated_float = 41; + repeated double repeated_double = 42; + repeated bool repeated_bool = 43; + repeated string repeated_string = 44; + repeated bytes repeated_bytes = 45; repeated TestEnum repeated_enum = 46; repeated Sub repeated_message = 47; repeated TestMessage repeated_recursive = 48; oneof my_oneof { - int32 oneof_int32 = 51; - int64 oneof_int64 = 52; - uint32 oneof_uint32 = 53; - uint64 oneof_uint64 = 54; - uint32 oneof_sint32 = 55; - uint64 oneof_sint64 = 56; - uint32 oneof_fixed32 = 57; - uint64 oneof_fixed64 = 58; + int32 oneof_int32 = 51; + int64 oneof_int64 = 52; + uint32 oneof_uint32 = 53; + uint64 oneof_uint64 = 54; + uint32 oneof_sint32 = 55; + uint64 oneof_sint64 = 56; + uint32 oneof_fixed32 = 57; + uint64 oneof_fixed64 = 58; uint32 oneof_sfixed32 = 59; uint64 oneof_sfixed64 = 60; - double oneof_double = 61; - float oneof_float = 62; - bool oneof_bool = 63; - string oneof_string = 64; - bytes oneof_bytes = 65; - TestEnum oneof_enum = 66; - Sub oneof_message = 67; + double oneof_double = 61; + float oneof_float = 62; + bool oneof_bool = 63; + string oneof_string = 64; + bytes oneof_bytes = 65; + TestEnum oneof_enum = 66; + Sub oneof_message = 67; } - map map_int32_int32 = 71; - map map_int64_int64 = 72; - map map_uint32_uint32 = 73; - map map_uint64_uint64 = 74; - map map_sint32_sint32 = 75; - map map_sint64_sint64 = 76; - map map_fixed32_fixed32 = 77; - map map_fixed64_fixed64 = 78; + map map_int32_int32 = 71; + map map_int64_int64 = 72; + map map_uint32_uint32 = 73; + map map_uint64_uint64 = 74; + map map_sint32_sint32 = 75; + map map_sint64_sint64 = 76; + map map_fixed32_fixed32 = 77; + map map_fixed64_fixed64 = 78; map map_sfixed32_sfixed32 = 79; map map_sfixed64_sfixed64 = 80; - map map_int32_float = 81; - map map_int32_double = 82; - map map_bool_bool = 83; - map map_string_string = 84; - map map_int32_bytes = 85; - map map_int32_enum = 86; - map map_int32_message = 87; + map map_int32_float = 81; + map map_int32_double = 82; + map map_bool_bool = 83; + map map_string_string = 84; + map map_int32_bytes = 85; + map map_int32_enum = 86; + map map_int32_message = 87; map map_recursive = 88; @@ -149,13 +149,31 @@ message TestMessage { map map_string_struct = 124; // deprecated field - int32 deprecated_optional_int32 = 125 [deprecated=true]; + int32 deprecated_int32 = 125 [deprecated = true]; + // deprecated optional field + optional int32 deprecated_optional_int32 = 126 [deprecated = true]; + // deprecated wrapped field + google.protobuf.Int32Value deprecated_int32_value = 127 [deprecated = true]; + // deprecated oneof + oneof deprecated_oneof { + int32 deprecated_oneof_int32 = 128 [deprecated = true]; + } + // deprecated repeated field + repeated int32 deprecated_repeated_int32 = 129 [deprecated = true]; + // deprecated map + map deprecated_map_int32_int32 = 130 [deprecated = true]; + // deprecated any + google.protobuf.Any deprecated_any = 131 [deprecated = true]; + // deprecated message + TestMessage deprecated_message = 132 [deprecated = true]; + // deprecated enum + NestedEnum deprecated_enum = 133 [deprecated = true]; } enum TestEnum { ZERO = 0; - ONE = 1; - TWO = 2; + ONE = 1; + TWO = 2; ECHO = 3; // Test reserved name. } @@ -173,38 +191,38 @@ message EmptyAnySerialization { } message TestPackedMessage { - repeated int32 repeated_int32 = 90 [packed = true]; - repeated int64 repeated_int64 = 91 [packed = true]; - repeated uint32 repeated_uint32 = 92 [packed = true]; - repeated uint64 repeated_uint64 = 93 [packed = true]; - repeated sint32 repeated_sint32 = 94 [packed = true]; - repeated sint64 repeated_sint64 = 95 [packed = true]; - repeated fixed32 repeated_fixed32 = 96 [packed = true]; - repeated fixed64 repeated_fixed64 = 97 [packed = true]; - repeated sfixed32 repeated_sfixed32 = 98 [packed = true]; - repeated sfixed64 repeated_sfixed64 = 99 [packed = true]; - repeated float repeated_float = 100 [packed = true]; - repeated double repeated_double = 101 [packed = true]; - repeated bool repeated_bool = 102 [packed = true]; - repeated TestEnum repeated_enum = 103 [packed = true]; + repeated int32 repeated_int32 = 90 [packed = true]; + repeated int64 repeated_int64 = 91 [packed = true]; + repeated uint32 repeated_uint32 = 92 [packed = true]; + repeated uint64 repeated_uint64 = 93 [packed = true]; + repeated sint32 repeated_sint32 = 94 [packed = true]; + repeated sint64 repeated_sint64 = 95 [packed = true]; + repeated fixed32 repeated_fixed32 = 96 [packed = true]; + repeated fixed64 repeated_fixed64 = 97 [packed = true]; + repeated sfixed32 repeated_sfixed32 = 98 [packed = true]; + repeated sfixed64 repeated_sfixed64 = 99 [packed = true]; + repeated float repeated_float = 100 [packed = true]; + repeated double repeated_double = 101 [packed = true]; + repeated bool repeated_bool = 102 [packed = true]; + repeated TestEnum repeated_enum = 103 [packed = true]; } // Need to be in sync with TestPackedMessage. message TestUnpackedMessage { - repeated int32 repeated_int32 = 90 [packed = false]; - repeated int64 repeated_int64 = 91 [packed = false]; - repeated uint32 repeated_uint32 = 92 [packed = false]; - repeated uint64 repeated_uint64 = 93 [packed = false]; - repeated sint32 repeated_sint32 = 94 [packed = false]; - repeated sint64 repeated_sint64 = 95 [packed = false]; - repeated fixed32 repeated_fixed32 = 96 [packed = false]; - repeated fixed64 repeated_fixed64 = 97 [packed = false]; - repeated sfixed32 repeated_sfixed32 = 98 [packed = false]; - repeated sfixed64 repeated_sfixed64 = 99 [packed = false]; - repeated float repeated_float = 100 [packed = false]; - repeated double repeated_double = 101 [packed = false]; - repeated bool repeated_bool = 102 [packed = false]; - repeated TestEnum repeated_enum = 103 [packed = false]; + repeated int32 repeated_int32 = 90 [packed = false]; + repeated int64 repeated_int64 = 91 [packed = false]; + repeated uint32 repeated_uint32 = 92 [packed = false]; + repeated uint64 repeated_uint64 = 93 [packed = false]; + repeated sint32 repeated_sint32 = 94 [packed = false]; + repeated sint64 repeated_sint64 = 95 [packed = false]; + repeated fixed32 repeated_fixed32 = 96 [packed = false]; + repeated fixed64 repeated_fixed64 = 97 [packed = false]; + repeated sfixed32 repeated_sfixed32 = 98 [packed = false]; + repeated sfixed64 repeated_sfixed64 = 99 [packed = false]; + repeated float repeated_float = 100 [packed = false]; + repeated double repeated_double = 101 [packed = false]; + repeated bool repeated_bool = 102 [packed = false]; + repeated TestEnum repeated_enum = 103 [packed = false]; } // /**/@<>&\{ @@ -236,8 +254,7 @@ message TestReverseFieldOrder { string b = 1; } -message testLowerCaseMessage { -} +message testLowerCaseMessage {} enum testLowerCaseEnum { VALUE = 0; diff --git a/src/google/protobuf/compiler/php/php_generator.cc b/src/google/protobuf/compiler/php/php_generator.cc index ef8cdcefe555..d7f5fa65e0b0 100644 --- a/src/google/protobuf/compiler/php/php_generator.cc +++ b/src/google/protobuf/compiler/php/php_generator.cc @@ -219,6 +219,20 @@ std::string DefaultForField(const FieldDescriptor* field) { } } +std::string DeprecatedConditionalForField(const FieldDescriptor* field) { + if (field->is_repeated()) { + return absl::StrCat("$this->", field->name(), "->count() !== 0"); + } + if (field->real_containing_oneof() != nullptr) { + return absl::StrCat("$this->hasOneof(", field->number(), ")"); + } + if (field->has_presence()) { + return absl::StrCat("isset($this->", field->name(), ")"); + } + return absl::StrCat("$this->", field->name(), " !== ", + field->has_presence() ? "null" : DefaultForField(field)); +} + std::string GeneratedMetadataFileName(const FileDescriptor* file, const Options& options) { absl::string_view proto_file = file->name(); @@ -574,6 +588,12 @@ void GenerateFieldAccessor(const FieldDescriptor* field, const Options& options, ? absl::StrCat("@trigger_error('", field->name(), " is deprecated.', E_USER_DEPRECATED);\n ") : ""; + std::string deprecation_trigger_with_conditional = + (field->options().deprecated()) + ? absl::StrCat("if (" + DeprecatedConditionalForField(field), + ") {\n ", deprecation_trigger, + "}\n ") + : ""; // Emit getter. if (oneof != nullptr) { @@ -584,7 +604,7 @@ void GenerateFieldAccessor(const FieldDescriptor* field, const Options& options, "}\n\n", "camel_name", UnderscoresToCamelCase(field->name(), true), "number", IntToString(field->number()), "deprecation_trigger", - deprecation_trigger); + deprecation_trigger_with_conditional); } else if (field->has_presence() && !field->message_type()) { printer->Print( "public function get^camel_name^()\n" @@ -594,7 +614,7 @@ void GenerateFieldAccessor(const FieldDescriptor* field, const Options& options, "}\n\n", "camel_name", UnderscoresToCamelCase(field->name(), true), "name", field->name(), "default_value", DefaultForField(field), - "deprecation_trigger", deprecation_trigger); + "deprecation_trigger", deprecation_trigger_with_conditional); } else { printer->Print( "public function get^camel_name^()\n" @@ -602,7 +622,8 @@ void GenerateFieldAccessor(const FieldDescriptor* field, const Options& options, " ^deprecation_trigger^return $this->^name^;\n" "}\n\n", "camel_name", UnderscoresToCamelCase(field->name(), true), "name", - field->name(), "deprecation_trigger", deprecation_trigger); + field->name(), "deprecation_trigger", + deprecation_trigger_with_conditional); } // Emit hazzers/clear. @@ -614,20 +635,22 @@ void GenerateFieldAccessor(const FieldDescriptor* field, const Options& options, "}\n\n", "camel_name", UnderscoresToCamelCase(field->name(), true), "number", IntToString(field->number()), "deprecation_trigger", - deprecation_trigger); + deprecation_trigger_with_conditional); } else if (field->has_presence()) { printer->Print( "public function has^camel_name^()\n" "{\n" - " ^deprecation_trigger^return isset($this->^name^);\n" - "}\n\n" + " ^deprecation_trigger_with_conditional^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); + "deprecation_trigger", deprecation_trigger, + "deprecation_trigger_with_conditional", + deprecation_trigger_with_conditional); } // For wrapper types, generate an additional getXXXUnwrapped getter @@ -642,7 +665,8 @@ void GenerateFieldAccessor(const FieldDescriptor* field, const Options& options, "$this->readWrapperValue(\"^field_name^\");\n" "}\n\n", "camel_name", UnderscoresToCamelCase(field->name(), true), "field_name", - field->name(), "deprecation_trigger", deprecation_trigger); + field->name(), "deprecation_trigger", + deprecation_trigger_with_conditional); } // Generate setter. @@ -654,7 +678,8 @@ void GenerateFieldAccessor(const FieldDescriptor* field, const Options& options, Indent(printer); - if (field->options().deprecated()) { + if (field->options().deprecated() && !field->is_map() && + !field->is_repeated()) { printer->Print("^deprecation_trigger^", "deprecation_trigger", deprecation_trigger); } @@ -712,6 +737,12 @@ void GenerateFieldAccessor(const FieldDescriptor* field, const Options& options, UnderscoresToCamelCase(field->cpp_type_name(), true)); } + if (field->options().deprecated() && + (field->is_map() || field->is_repeated())) { + printer->Print("if ($arr->count() !== 0) {\n ^deprecation_trigger^}\n", + "deprecation_trigger", deprecation_trigger); + } + if (oneof != nullptr) { printer->Print("$this->writeOneof(^number^, $var);\n", "number", IntToString(field->number()));