Skip to content
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

feat: better debug info for PHP messages and repeated fields #12718

Closed
wants to merge 7 commits into from

Conversation

bshaffer
Copy link
Contributor

@bshaffer bshaffer commented May 8, 2023

addresses #12714 by dumping more concise debug info for protobuf messages and repeated fields via the serializeToJsonString function. Additionally, message types which serialize into something other than an array (e.g. Google\Protobuf\Value, Google\Protobuf\Timestamp, etc) are handled in a special way to make their output consistent with other messages.

$m = new Google\Protobuf\DoubleValue();
$m->setValue(1.5);
var_dump($m);

will output

object(Google\Protobuf\DoubleValue)#12 (1) {
  ["value"]=>
  float(1.5)
}

@bshaffer bshaffer requested a review from a team as a code owner May 8, 2023 19:09
@bshaffer bshaffer requested review from ericsalo and removed request for a team May 8, 2023 19:09
@acozzette acozzette added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label May 10, 2023
@haberman
Copy link
Member

Is the output of var_dump something that users would depend on? Can we change it freely without updating a major version?

Suppose that at some point, we wanted to change the output of var_dump() to output text format instead. Would this be a breaking change?

@hlopko hlopko added the php label May 30, 2023
@bshaffer
Copy link
Contributor Author

bshaffer commented Aug 23, 2023

Thanks @haberman. This is a great question. As __debugInfo has "debug" in the name, I feel like it's self-evident that this behavior shouldn't be relied upon in production systems (and probably isn't being relied upon currently, given the output). However, if a major version bump is coming already, I think it'd make sense to have this be a part of it, for continuity.

So, while not strictly necessary to put this behind a major version bump, it would be appropriate to do so.

Any update on when this can be merged?

@bshaffer
Copy link
Contributor Author

Ping on this @haberman ! I think this would be a very great improvement for our customers!

@shawnlindstrom
Copy link

This would have been a huge help when implementing v2 for Cloud Speech a couple weeks ago. @bshaffer thanks for doing this and I hope it gets merged soon.

@haberman haberman added 🅰️ safe for tests Mark a commit as safe to run presubmits over and removed 🅰️ safe for tests Mark a commit as safe to run presubmits over labels Oct 13, 2023
@github-actions github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Oct 13, 2023
@zhangskz zhangskz added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Oct 13, 2023
@github-actions github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Oct 13, 2023
@zhangskz
Copy link
Member

(Note, another LGTM was needed after rebase, since review needs to be at head commit to be imported)

@bshaffer
Copy link
Contributor Author

@zhangskz I submitted a fix for the failing tests, could you approve them to run again?

@zhangskz zhangskz added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Oct 16, 2023
@fowles fowles added 🅰️ safe for tests Mark a commit as safe to run presubmits over and removed 🅰️ safe for tests Mark a commit as safe to run presubmits over labels Oct 16, 2023
@github-actions github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Oct 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants