Skip to content

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

Closed
bshaffer wants to merge 7 commits intoprotocolbuffers:mainfrom
bshaffer:php-add-debuginfo
Closed

feat: better debug info for PHP messages and repeated fields#12718
bshaffer wants to merge 7 commits intoprotocolbuffers:mainfrom
bshaffer:php-add-debuginfo

Conversation

@bshaffer
Copy link
Copy Markdown
Contributor

@bshaffer bshaffer commented May 8, 2023 •
edited
Loading

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
Copy Markdown
Member

haberman commented May 15, 2023

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
Copy Markdown
Contributor Author

bshaffer commented Aug 23, 2023 •
edited
Loading

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
Copy Markdown
Contributor Author

bshaffer commented Sep 26, 2023

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

@shawnlindstrom
Copy link
Copy Markdown

shawnlindstrom commented Oct 12, 2023

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
Copy Markdown
Member

zhangskz commented Oct 13, 2023

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

@bshaffer
Copy link
Copy Markdown
Contributor Author

bshaffer commented Oct 16, 2023

@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
@bshaffer bshaffer deleted the php-add-debuginfo branch October 17, 2023 15:35
@bshaffer bshaffer mentioned this pull request Nov 28, 2023
Open
@Keleset-Web Keleset-Web mentioned this pull request Jul 1, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@haberman haberman haberman approved these changes

@zhangskz zhangskz zhangskz approved these changes

@ericsalo ericsalo Awaiting requested review from ericsalo ericsalo is a code owner automatically assigned from protocolbuffers/protobuf-php

+1 more reviewer

@fowles fowles fowles approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

php

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@bshaffer @haberman @shawnlindstrom @zhangskz @fowles @hlopko @acozzette