-
Notifications
You must be signed in to change notification settings - Fork 22
Introduce support for emitting defaults for json responses #226
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
base: master
Are you sure you want to change the base?
Conversation
|
Oh, I just saw #89 has been in flight for a bit, also centered on adding an interface for the servers. RIP, I don't wanna conflict with that issue+PR set as it's been around for a while, but I don't see a ton of feedback. If an interface is undesirable, I'm happy to pull it out of this PR. EDIT: In thinking about it, I'm gonna break this down a bit into a dedicated interface for emitting json defaults and not tie it into a central interface. My reasoning is that if you have tens or hundreds of twirp services, you probably don't want to regenerate all of them at once, you'll want to slow roll, so for that reason when this package introduces new features, they probably want to be in isolated interfaces instead of having a central interface that might force everyone to do big painful migrations. So instead of |
4ba26c7 to
89af80c
Compare
…s new Twirp\ServerInterface
89af80c to
d9038a5
Compare
|
Thanks for opening a PR!
What's the reason for the interface? Is it supposed to be called by something outside of the generated code? (Not saying yes or no, just trying to understand why it's there first, though I'm generally not a huge fan of mutable state when not necessary)
Depends on how much it would cost to keep backward compatibility. If it's like a single line and it's not an ugly hack, it's a no brainer. Otherwise, I don't mind upgrading.
I think it's fair to say that people will notice if it's not working and if they are aware of this feature, they will also know it's bound to a version, so I don't think this is necessary. If we bump the required proto version, installation will fail anyway if they lock the proto version. As for the server interface: I tagged you in an open PR, let's continue the conversation there. |
Yep! I'm using https://github.com/laminas/laminas-stratigility for middleware pipeline driven services, so if I want to make a common shared middleware to enable emitting defaults for json responses across a fleet of services, I prefer to type check against an interface if possible.
ok! I'll keep my backwards compatibility in place then.
SGTM, I agree with that, just wanted to make sure. Thank you for the feedback! I think this is probablyyyyy reviewable as is in that case (note that I'm still waiting on a new protobuf release with this change, as soon as that's cut I can update this PR to reference the "real" version they decide on) |
|
Given we are dropping support for older PHP versions, BC might not be that important. From what I can tell this is also in line with the tiny interface idea we discussed in another issue/PR, so LGTM. You might have to resolve some conflicts once the server registration PR gets merged. |
Overview
By default, protobuf responses do not include default values, even for json. This can make inspecting json responses through curl or custom clients rather confusing, as entire fields may be absent. Guidance from protobuf (https://protobuf.dev/programming-guides/json/#json-options) is that languages may (or may not) provide options to override this behavior and emit all default values. In protocolbuffers/protobuf#23985 I added this option to PHP's protobuf generation tooling, and would love to use it in twirphp (please note a release version of protobuf with this option is not yet cut). I also felt like with this (and the existing
getPathPrefix), it would be nice to have an interface that describes the "global" (as in present on all twirp generated servers) funcs available, so I addedTwirp\ServerInterface.What this PR does / why we need it
getPathPrefixSpecial notes for your reviewer
Before I work on adding tests and promoting this out of a draft state, I wanted some clarity from the maintainers:
setEmitJsonDefaults(true)and does not have the required protobuf package option available? Currently I do a silent noop as we don't have a user provided logger.Does this PR introduce a user-facing change?
Yes.