-
Notifications
You must be signed in to change notification settings - Fork 15.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix binary compatibility in FieldCodec factory methods #6380
Fix binary compatibility in FieldCodec factory methods #6380
Conversation
I'm confused about why we have these defaultValue parameters at all - they never seem to be used. My guess is that that's a separate bug, that the existing methods aren't calling the right constructor. (That should be confirmed with a breaking test before being fixed, and that should probably be in a different PR.) I would make the overloads delegate to the methods accepting a default value though, unless there's a good reason not to. |
Right. I was going to do it in this one but decided to fix it in #5936 when the different default values would be put to use (with extensions being finalized then). |
Why previously there were no test to catch it? |
There was no code to test against. There was no way to generate extension identifiers at the time (before #5936) so rather than add tests for code that couldn't actually be used (unless the consumer decided to implement the interfaces and use the API manually) I decided to wait until the last PR. Then the code could be fully tested properly when unittest.proto could actually be generated in the test project. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Fine to merge (and backport to v3.9.x branch)? |
…rs#6380) * Fix binary compatibility in FieldCodec factory messages * Make default value parameter for current factories required * Route old methods through default value overloads
* Add changelog for 3.9.x * Revert "Make php message class final to avoid mocking (#6277)" (#6324) This reverts commit 7f84a94. This is just temporary. Eventually, we still want to roll forward this change. Some users are complaining they need more time to clean up their code. * Update extract_includes.bat.in File io_win32.h is not in directory google\protobuf\stubs under directory google\protobuf\io * Set oneof case in array constructor (#6351) Forgot to set it previously. * Update protobuf version (#6366) * Drop building wheel for python 3.4 (#6406) https://github.com/matthew-brett/multibuild/pull/240 * Fix binary compatibility in FieldCodec factory methods (#6380) (#6424) * Fix binary compatibility in FieldCodec factory messages * Make default value parameter for current factories required * Route old methods through default value overloads * Remove ExtensionRegistry.Add(params) overload * Rename ExtensionRegistry.Add(IEnumerable<Extension>) overload to AddRange * Edit naming of parameters in Extension classes * * Fix add API warnings to docs for extension APIs * Rename internal ExtensionSet.GetValue to TryGetValue * Disable javadoc error (#6371) * Disable javadoc error Actual fixes of the javadoc will be followed up * Remove duplicated configuration * Update javadoc plugin version * Updated Bazel test script to use most recent Bazel version (#6413) (#6433) I'm not exactly sure why, but this fixes the failing Bazel presubmit test. Using the most recent version seems like a good idea anyway so that we can make sure we're compatible with any new Bazel changes. * [bazel] Add fixes for --incompatible_load_{cc,java,proto}_rules_from_bzl * No need to update version in generated code (#6471) generate_descriptor will handle that * Update protobuf version (#6472)
Fix for #6379.