Skip to content

Commit

Permalink
Append field number to accessors if there is conflict (#6169)
Browse files Browse the repository at this point in the history 
Previously, foo_value and foo (wrapper field) will both have get/setFooValue
  • Loading branch information
@TeBoring
TeBoring authored May 24, 2019
1 parent e038b1e commit 9a8ef05
Show file tree
Hide file tree
Showing 3 changed files with 104 additions and 13 deletions.
11 changes: 11 additions & 0 deletions php/tests/proto/test_wrapper_type_setters.proto
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,3 +24,14 @@ message TestWrapperSetters {

map<string, google.protobuf.StringValue> map_string_value = 13;
}

message TestWrapperAccessorConflicts {
int32 normal_vs_wrapper_value = 1;
google.protobuf.Int32Value normal_vs_wrapper = 2;

int32 normal_vs_normal_value = 3;
int32 normal_vs_normal = 4;

google.protobuf.Int32Value wrapper_vs_wrapper_value = 5;
google.protobuf.Int32Value wrapper_vs_wrapper = 6;
}
38 changes: 38 additions & 0 deletions php/tests/wrapper_type_setters_test.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,44 @@

class WrapperTypeSettersTest extends TestBase
{
public function testConflictNormalVsWrapper()
{
$m = new Foo\TestWrapperAccessorConflicts();

$m->setNormalVsWrapperValue1(1);
$this->assertSame(1, $m->getNormalVsWrapperValue1());

$m->setNormalVsWrapperValue2(1);
$this->assertSame(1, $m->getNormalVsWrapperValue2());

$wrapper = new Int32Value(["value" => 1]);
$m->setNormalVsWrapper($wrapper);
$this->assertSame(1, $m->getNormalVsWrapper()->getValue());
}

public function testConflictNormalVsNormal()
{
$m = new Foo\TestWrapperAccessorConflicts();

$m->setNormalVsNormalValue(1);
$this->assertSame(1, $m->getNormalVsNormalValue());

$m->setNormalVsNormal(1);
$this->assertSame(1, $m->getNormalVsNormal());
}

public function testConflictWrapperVsWrapper()
{
$m = new Foo\TestWrapperAccessorConflicts();

$m->setWrapperVsWrapperValueValue(1);
$this->assertSame(1, $m->getWrapperVsWrapperValueValue());

$wrapper = new Int32Value(["value" => 1]);
$m->setWrapperVsWrapperValue5($wrapper);
$this->assertSame(1, $m->getWrapperVsWrapperValue5()->getValue());
}

/**
* @dataProvider gettersAndSettersDataProvider
*/
Expand Down
68 changes: 55 additions & 13 deletions src/google/protobuf/compiler/php/php_generator.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -655,27 +655,58 @@ void GenerateOneofField(const OneofDescriptor* oneof, io::Printer* printer) {

void GenerateFieldAccessor(const FieldDescriptor* field, bool is_descriptor,
io::Printer* printer) {
bool need_other_name_for_accessor = false;
bool need_other_name_for_wrapper_accessor = false;
const Descriptor* desc = field->containing_type();

if (!field->is_repeated() &&
field->cpp_type() == FieldDescriptor::CPPTYPE_MESSAGE &&
IsWrapperType(field)) {
// Check if there is any field called xxx_value
const FieldDescriptor* other =
desc->FindFieldByName(StrCat(field->name(), "_value"));
if (other != NULL) {
need_other_name_for_wrapper_accessor = true;
}
}

if (strings::EndsWith(field->name(), "_value")) {
std::size_t pos = (field->name()).find("_value");
string name = (field->name()).substr(0, pos);
const FieldDescriptor* other = desc->FindFieldByName(name);
if (other != NULL &&
!other->is_repeated() &&
other->cpp_type() == FieldDescriptor::CPPTYPE_MESSAGE &&
IsWrapperType(other)) {
need_other_name_for_accessor = true;
}
}

const OneofDescriptor* oneof = field->containing_oneof();

// Generate getter.
if (oneof != NULL) {
GenerateFieldDocComment(printer, field, is_descriptor, kFieldGetter);
printer->Print(
"public function get^camel_name^()\n"
"public function get^camel_name^^field_number^()\n"
"{\n"
" return $this->readOneof(^number^);\n"
"}\n\n",
"camel_name", UnderscoresToCamelCase(field->name(), true),
"number", IntToString(field->number()));
"number", IntToString(field->number()),
"field_number", need_other_name_for_accessor ?
StrCat(field->number()) : "");
} else {
GenerateFieldDocComment(printer, field, is_descriptor, kFieldGetter);
printer->Print(
"public function get^camel_name^()\n"
"public function get^camel_name^^field_number^()\n"
"{\n"
" return $this->^name^;\n"
"}\n\n",
"camel_name", UnderscoresToCamelCase(field->name(), true), "name",
field->name());
"camel_name", UnderscoresToCamelCase(field->name(), true),
"name", field->name(),
"field_number", need_other_name_for_accessor ?
StrCat(field->number()) : "");
}

// For wrapper types, generate an additional getXXXValue getter
Expand All @@ -684,21 +715,28 @@ void GenerateFieldAccessor(const FieldDescriptor* field, bool is_descriptor,
field->cpp_type() == FieldDescriptor::CPPTYPE_MESSAGE &&
IsWrapperType(field)) {
GenerateWrapperFieldGetterDocComment(printer, field);

printer->Print(
"public function get^camel_name^Value()\n"
"public function get^camel_name^Value^field_number1^()\n"
"{\n"
" $wrapper = $this->get^camel_name^();\n"
" $wrapper = $this->get^camel_name^^field_number2^();\n"
" return is_null($wrapper) ? null : $wrapper->getValue();\n"
"}\n\n",
"camel_name", UnderscoresToCamelCase(field->name(), true));
"camel_name", UnderscoresToCamelCase(field->name(), true),
"field_number1", need_other_name_for_wrapper_accessor ?
StrCat(field->number()) : "",
"field_number2", need_other_name_for_accessor ?
StrCat(field->number()) : "");
}

// Generate setter.
GenerateFieldDocComment(printer, field, is_descriptor, kFieldSetter);
printer->Print(
"public function set^camel_name^($var)\n"
"public function set^camel_name^^field_number^($var)\n"
"{\n",
"camel_name", UnderscoresToCamelCase(field->name(), true));
"camel_name", UnderscoresToCamelCase(field->name(), true),
"field_number", need_other_name_for_accessor ?
StrCat(field->number()) : "");

Indent(printer);

Expand Down Expand Up @@ -798,13 +836,17 @@ void GenerateFieldAccessor(const FieldDescriptor* field, bool is_descriptor,
IsWrapperType(field)) {
GenerateWrapperFieldSetterDocComment(printer, field);
printer->Print(
"public function set^camel_name^Value($var)\n"
"public function set^camel_name^Value^field_number1^($var)\n"
"{\n"
" $wrappedVar = is_null($var) ? null : new \\^wrapper_type^(['value' => $var]);\n"
" return $this->set^camel_name^($wrappedVar);\n"
" return $this->set^camel_name^^field_number2^($wrappedVar);\n"
"}\n\n",
"camel_name", UnderscoresToCamelCase(field->name(), true),
"wrapper_type", LegacyFullClassName(field->message_type(), is_descriptor));
"wrapper_type", LegacyFullClassName(field->message_type(), is_descriptor),
"field_number1", need_other_name_for_wrapper_accessor ?
StrCat(field->number()) : "",
"field_number2", need_other_name_for_accessor ?
StrCat(field->number()) : "");
}

// Generate has method for proto2 only.
Expand Down

0 comments on commit 9a8ef05

Please sign in to comment.