fix: remove extension exception

[bshaffer]

• for parity with the native implementation (changing the implementation to match the message would break BC)
• the return value is already checked everywhere this method is called (1, 2, 3, 4)

There's also an issue we've seen here when an API removes fields, and we want to keep the methods in order to preserve BC. The protobuf message throws an exception (only in the extension) because the fields exist, even if they're never used. It would be easier to just removed this unnecessary exception.

[haberman]

There's also an issue we've seen here when an API removes fields, and we want to keep the methods in order to preserve BC.

Can you give an example of what you mean?

If a proto message removes a field, I don't think we should allow API references to that field. Otherwise a user could refer to any field name, even if it never existed.

[bshaffer]

Can you give an example of what you mean?

An API removes a field fieldA. We keep the PHP methods getFieldA and setFieldA, because rather than throw a PHP Fatal error in functioning code, it results in a No-Op. It's extra work (and also another thing to remember) to remove the property also because otherwise the extension throws an exception. This isn't the main reason to remove this behavior though - the main reason is to have the extension and native implementation function the same.

If a proto message removes a field, I don't think we should allow API references to that field.

If our libraries are GA, we have to preserve the PHP surface. Otherwise it would be a breaking change. So the no-op is the way to go.

Otherwise a user could refer to any field name, even if it never existed.

I don't understand how this would be true

[haberman]

An API removes a field fieldA.

It would be helpful to frame this scenario around .proto files specifically. Are you saying something like this?

Before:

message Foo {
  optional int32 field_a = 1;
  optional int32 field_b = 2;
}

After:

message Foo {
  optional int32 field_b = 2;
}

What I hear you saying is that you want $foo->getFieldA() to work (return null?) even when used with the "after" proto. Is that correct?

[bshaffer]

@haberman I think the misunderstanding is that we want to keep the previously generated protobuf file in favor of the new one. We don't want any protobuf message to be able to set any property. We've had to modify it currently, and remove the setting of the properties, because otherwise the extension throws an exception (because we are still updating the metadata).

e.g. in the example you provided:

// After:
class Foo extends Message
{
    // these methods are still here, because removing them would be breaking, but they are now a no-op
    public function setFieldA($var);
    public function getFieldA();

    public function setFieldB($var);
    public function getFieldB();
}

It's a hack, but it's a necessary one when APIs make breaking changes. Also, again, this isn't the main reason for this change. The main reason is for consistency.

[haberman]

I'm just trying to understand the desired behavior. Can you show an example of the .proto file input, and some PHP code that uses the resulting generated class, and the behavior you'd like it to have?

If we have the input:

// After
message Foo {
  optional int32 field_b = 2;
}

It is impossible to generate the following output, because protoc has no way of knowing that field_a ever existed:

// After:
class Foo extends Message
{
    public function setFieldA($var);
    public function getFieldA();

    public function setFieldB($var);
    public function getFieldB();
}