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.

Sign up for GitHub

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

Jump to bottom

php: Fix formatting of Duration #6155

Merged
merged 2 commits into from
Jun 18, 2019
Merged

php: Fix formatting of Duration #6155

merged 2 commits into from
Jun 18, 2019

Conversation

lhecker
Copy link
Contributor

@lhecker lhecker commented May 18, 2019 •
edited
Loading

This fixes the following issues for php's native Duration class:

  • Missing nanoseconds. The nanoseconds where divided as a float and implicitly converted to a string before being passed to bcadd (Most bcmath functions accept their parameters as strings!). This can result in a float formatted in scientific/exponential notation, which bcmath doesn't understand.
    E.g. a Duration instance with 123s and 456ns can previously result in an invocation like 
    strval(bcadd('123', '4.56e-7', 9)) === '123.000000000'
    Using a regular sprintf seems like a far simpler solution to me.
  • Durations are supposed to be formatted without trailing zeroes.
    I've added a simple call to rtrim for this.

@lhecker lhecker force-pushed the fix-duration-formatting branch from c8bfcae to 8a8dc23 Compare May 18, 2019 15:32
@lhecker
Copy link
Contributor Author

lhecker commented May 18, 2019

@TeBoring Mergeable is telling me to include a language label but I don't quite understand how I'm supposed to do that. And does this PR belong to the "Various bug fixes" banner that's already in the CHANGES.txt?

@lhecker lhecker changed the title [PHP] Fix formatting of Duration php: Fix formatting of Duration May 18, 2019
@TechProofreader
Copy link
Contributor

I'm having the same problem with my pull request too, so you're not alone. I submitted my pull request to one of the docs in protobuf, and it initially passed all "tests," but then it stopped and said I'm missing labels for languages and a ton of other things even though (1) there's no option for that, and (2) the doc I submitted a pull for contains multiple languages so selecting a single language is impossible.

@TeBoring TeBoring self-assigned this May 19, 2019
@TeBoring
Copy link
Contributor

We have conformance test at https://github.com/protocolbuffers/protobuf/blob/master/conformance/binary_json_conformance_suite.cc
Would you mind also adding the case there?
If other languages cannot pass the test, you can whitelist the test for them in their failure list.

@TeBoring
Copy link
Contributor

To run conformance test, cd conformance; make test_php

@lhecker lhecker force-pushed the fix-duration-formatting branch from 8a8dc23 to 92ce1ed Compare May 19, 2019 21:36
@lhecker
Copy link
Contributor Author

lhecker commented May 19, 2019

@TeBoring Thanks for your advice.
I had to rebase my PR because the initial attempt didn't work with negative nanos.

I tried running the conformance tests by checking out protobuf and running:

./autogen.sh
/configure
make
cd conformance
make test_php

but the runner gets stuck soon. The cause is a "Undefined offset: 123" notice in the stderr output.
It's caused by this line:

protobuf/php/src/Google/Protobuf/Internal/EnumDescriptor.php

Line 42 in b4f1937

return $this->value[$number];

getValueByNumber is called in two different places and both assume that the return value is nullable, even though it's written in such a way that implies that $number is always a valid key.
I think I'm going to fix this as part of this PR as I'm unable to run the conformance tests otherwise.

@lhecker lhecker force-pushed the fix-duration-formatting branch from 92ce1ed to 482c31e Compare May 19, 2019 22:28
@lhecker
Copy link
Contributor Author

lhecker commented May 19, 2019

@TeBoring This PR fixes the DurationHasZeroFractionalDigit conformance test and I removed it from the failure list.

I don't understand why it doesn't fix e.g. the DurationHas3FractionalDigits test though...
Why is there a trailing zero in the first place?

protobuf/conformance/binary_json_conformance_suite.cc

Line 1947 in b4f1937

return value["optionalDuration"].asString() == "1.010s";

Even though there's supposed to be no trailing zeroes when being formatted:

protobuf/src/google/protobuf/duration.proto

Lines 98 to 100 in b4f1937

// encoded in JSON format as "3s", while 3 seconds and 1 nanosecond should
// be expressed in JSON format as "3.000000001s", and 3 seconds and 1
// microsecond should be expressed in JSON format as "3.000001s".

Do you know why that's the case? Is the duration.proto description incorrect?

@TeBoring
Copy link
Contributor

TeBoring commented May 22, 2019 •
edited
Loading

syntax error, unexpected '?' in /tmp/protobuf/protobuf/php/src/Google/Protobuf/Internal/EnumDescriptor.php on line 42

@TeBoring
Copy link
Contributor

In our spec for serialization, we require nanosec has 0/3/6/9 digits precision.
The example in duration.proto does follow our spec.

@lhecker
Copy link
Contributor Author

lhecker commented May 22, 2019

@TeBoring Ah I used the null-coalescing operator - I'm very sorry. 😔
I'll fix my PR until tomorrow and make sure to also properly test using PHP 5.5 this time.
I'll also try to make the duration formatting standard compliant if you don't mind.

@lhecker lhecker force-pushed the fix-duration-formatting branch from 482c31e to a740378 Compare May 28, 2019 09:39
@lhecker
php: Fixed formatting of Duration
f654435 
This fixes:
* Missing nanoseconds. The nanoseconds where divided as a float and
  implicitly converted to a string before being passed to bcadd.
  This can result in a float formatted in scientific/exponential notation,
  which bcmath doesn't understand.
* Durations are supposed to be formatted without trailing zeroes.
@lhecker lhecker force-pushed the fix-duration-formatting branch from a740378 to f654435 Compare May 28, 2019 09:40
@lhecker
Copy link
Contributor Author

lhecker commented May 28, 2019

@TeBoring I've updated this PR finally.
I've run the conformance suite with PHP 5.5 as well this time. (There were some errors like Error parsing text-format protobuf_test_messages.proto3.TestAllTypesProto3: 1:18: Integer out of range (18446744073709551616) though.)

This PR also fixes the 0/3/6/9 digit formatting. If you dislike that I can open a separate PR for that.

@lhecker
Copy link
Contributor Author

lhecker commented Jun 11, 2019

@TeBoring (Just in case you missed it...) I think you can trigger kokoro again.
Also:

This PR also fixes the 0/3/6/9 digit formatting. If you dislike that I can open a separate PR for that.

@TeBoring
Copy link
Contributor

Thanks for the fix!

@TeBoring TeBoring merged commit 43307d4 into protocolbuffers:master Jun 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TeBoring TeBoring TeBoring approved these changes

Assignees

@TeBoring TeBoring

Labels
cla: yes php
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@lhecker @TechProofreader @TeBoring @googlebot @kokoro-team @protobuf-kokoro