[7392] fix conformance test in PHP JSON parser #16743
Conversation
antongrbin
force-pushed
the
anton--7392--fix-unknown-enum-php
branch
2 times, most recently
from
0498647 to
77bb25f
Compare
antongrbin
force-pushed
the
anton--7392--fix-unknown-enum-php
branch
from
77bb25f to
367aa3d
Compare
|
@antongrbin I can take a look |
|
@bshaffer thank you for the review offer && polite ping :) |
|
@bshaffer friendly ping :) |
JasonLunn
added
the
🅰️ safe for tests
github-actions
bot
removed
the
🅰️ safe for tests
There was a problem hiding this comment.
This is really the behavior we want? If an array/map is unknown, it throws an exception unless other valid values exist, at which point the unknown value is ignored?
That does not seem like intuitive behavior to me... what is the justification for this?
EDIT: On second look, I see that the difference between the tests which throw an exception and the tests which do not throw an exception is the true parameter being passed to mergeFromJsonString. This was not obvious, since these tests ALSO differ in that in one, the UNKNOWN field is alone, and in the others, there are two other values in the map/repeated fields.
| "{\"repeated_enum\":[ | ||
| \"ONE\", | ||
| \"UNKNOWN_ENUM\", | ||
| \"ZERO\" | ||
| ]}", |
There was a problem hiding this comment.
Why are we using values of ONE and ZERO in some tests, and using ONLY the UNKNOWN_ENUM in others? Let's keep this consistent, or test using both if that's what's desired. Otherwise, changing the JSON string input betwen tests is confusing (as what we are actually testing is the second parameter being true).
There was a problem hiding this comment.
Great catch! I now made the JSON string inputs equal in each test pair.
# Motivation This PR fixes failing JSON conformance tests for php with name `IgnoreUnknownEnumStringValue*`. The JSON parsing spec was discussed in #7392. Recent similar changes in other languages: - Python: 86abf35 - Swift: apple/swift-protobuf#1345 - C#: #15758 - C++: #16479 Note: this PR is equivalent to #16743. I had to create a new one since I lost access to noom/protobuf in the meantime (switched companies recently). Closes #19376 COPYBARA_INTEGRATE_REVIEW=#19376 from antongrbin:anton--7392--php-newbranch 641a28a PiperOrigin-RevId: 699989555
Motivation
This PR fixes failing JSON conformance tests for php with name
IgnoreUnknownEnumStringValue*.The JSON parsing spec was discussed in #7392.
Recent similar changes in other languages: