Skip to content

Conversation

@SpencerMalone
Copy link
Contributor

@SpencerMalone SpencerMalone commented Oct 20, 2025

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 added Twirp\ServerInterface.

What this PR does / why we need it

  • Allows an opt-in setting for emitting default values when returning JSON
  • Dances around requiring newer protobuf versions by checking if the new protobuf option is declared or not
  • Adds an interface with the new functions + getPathPrefix

Special 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:

  • Are y'all OK with the interface, or do you prefer I not add that?
  • Do you care if I maintain backwards compatibility with older protobuf package+extension versions, or should we just upgrade the minimum required version?
  • Do you want me to log a warning or anything if someone calls 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.

- Added new `Twirp\ServerInterface` for those who need an interface for a generated server's functions available beyond `Psr\Http\Server\RequestHandlerInterface`
- Added  support for `setEmitJsonDefaults`, allowing you to always emit default values for json responses when using protobuf package and extension versions greater than v4.33.0.

@SpencerMalone
Copy link
Contributor Author

SpencerMalone commented Oct 20, 2025

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 Server implements ServerInterface, we'd have: Server implements RequestHandlerInterface, CanEmitJsonDefaults, HasPathPrefix

@sagikazarmark
Copy link
Member

Thanks for opening a PR!

Are y'all OK with the interface, or do you prefer I not add that?

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)

Do you care if I maintain backwards compatibility with older protobuf package+extension versions, or should we just upgrade the minimum required version?

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.

Do you want me to log a warning or anything if someone calls setEmitJsonDefaults(true) and does not have the required protobuf package option available?

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.

@SpencerMalone
Copy link
Contributor Author

SpencerMalone commented Oct 28, 2025

What's the reason for the interface? Is it supposed to be called by something outside of the generated code?

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.

If it's like a single line and it's not an ugly hack

ok! I'll keep my backwards compatibility in place then.

I don't think (logging) this is necessary

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)

@SpencerMalone SpencerMalone marked this pull request as ready for review October 28, 2025 01:45
@SpencerMalone SpencerMalone changed the title Introduce support for emitting defaults for json responses, as well as new Twirp\ServerInterface Introduce support for emitting defaults for json responses Oct 29, 2025
@sagikazarmark
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants