fix(php): do not throw deprecated warning on field getters for default values #21033
Conversation
…t values (protocolbuffers#17788) backports fix for protocolbuffers#13428 to 25.x branch
bshaffer
changed the title
github-actions
bot
removed
the
🅰️ safe for tests
|
Hi Brent, from April 7 we will drop 25 support for everything except Java |
|
@anandolee When you say "drop", do you mean "release"? And if so can we really try to get this merged before then? |
github-actions
bot
removed
the
🅰️ safe for tests
|
@anandolee My customer would very much appreciate this being merged. |
|
Expanding on this to say that because the googleapi public protos are on protobuf 25, all the client libraries generate messages using that protobuf version. We are trying to update it, but it's difficult since we need to do it for ALL languages at once, and there are some breaking change concerns with some languages for upgrading to 26. This is a simple fix which is backported to 25 so we can resolve the issue for our customers now, instead of having to wait to upgrade protobuf for all the client libraries (which may be soon, and it may not be). |
|
Brent, we will discuss about the release tomorrow. +Mike for PHP review |
| @@ -239,6 +239,20 @@ std::string DefaultForField(const FieldDescriptor* field) { | |||
| } | |||
| } | |||
|
|
|||
| std::string DeprecatedConditionalForField(const FieldDescriptor* field) { | |||
There was a problem hiding this comment.
The current behavior of throwing seems obviously bad, but I don't think it makes sense to care if the field is a default value or not in protobuf semantics; is this actually an idiomatic PHP shape of behavior?
I would have expected the WAI behavior to just log an error on deprecated fields that are either get or set, and to not care about the default value and to never throw?
In general, if you have an implicit presence bool and read it and it has false, you're still using a deprecated path that should be moved off (if its deprecated then callers may stop setting it and it incorrectly has value false when it should have been true, logic reading it is wrong).
There was a problem hiding this comment.
The issue is that this check is performed internally. This is why customers get deprecated warnings in their logs even though they have not used the field. For this reason, we use checking in this way to say "if this field has been modified by the user, issue the deprecation warning"
The only other option would be to remove the deprecation warning entirely. This is fine with me, but we've already released the fix in this PR in the main branch of protobuf, so I'd rather keep it as-is unless there's a real concern with this implementation.
There was a problem hiding this comment.
@esrauchg Do you still have any addl concerns here for this backport, or is this good to approve + release?
There was a problem hiding this comment.
I do think its suboptimal that explicit calls to read fields from the application code won't log if the value is the default value ("deprecated" could also mean "things may stop setting this even though the field is still there, if you read it and get the default value it might be right or might be bogus").
But I don't think we need to hold up the backport here; "deprecation messages when the application isn't doing anything" is more bad than "incorrectly not having coverage of getters that happen to be default" is bad.
zhangskz
added
php
back-port
github-actions
bot
removed
the
🅰️ safe for tests
|
Thank you @zhangskz ! |
|
Hey @bshaffer ! Is there a timeline to update the googleapi public protos to use v25.7? Thanks for the effort thus far 🙏 |
|
@TomBrouws Hey Tom! We are currently blocked because Java can't upgrade to |
|
I spoke too soon - it's looking promising for us to make this change. I'll keep this bug updated on our progress! |
backports the fix in #17788 for the
25.xbranch, fixes #13428 in 25.x branch