protobuf icon indicating copy to clipboard operation
protobuf copied to clipboard
Published 4 weeks ago • verified publisher icon protocolbuffers

PHP: Fix big numbers (uint64, fixed64) bug

Open zlodes opened this issue 2 years ago • 11 comments

This PR:

  • Fixes the bug mentioned in #9521 (in PHP and C)
  • Improves .gitignore connected to PHP
  • Adds missed BCMath extension dependency used by GPBUtil
  • Fixes test_util.php

Closes #9521

zlodes avatar Oct 29 '23 13:10 zlodes

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

google-cla[bot] avatar Oct 29 '23 13:10 google-cla[bot]

Thank you for the contribution!

mkruskal-google avatar Nov 04 '23 00:11 mkruskal-google

I've fixed uint64 bug both in PHP and C implementations. Both of them had problems with uint64 values bigger than int64 on 64-bit systems.

Also found error in test_util.php: https://github.com/protocolbuffers/protobuf/pull/14552/files#diff-9ab947dc8c87867e1fbb799135a9a936f066df99509a347c30512d7e1dd69329R42 It used negative number as unit64 value.

zlodes avatar Nov 04 '23 23:11 zlodes

When will it be merged? My code is almost no longer able to run properly on a 64-bit machine. 2147483648 => -2147483648, my timestamp cannot exceed 2038-01-19 11:14:08, I hope it can be merged soon. Thanks

brotherbigbao avatar Dec 28 '23 07:12 brotherbigbao

When will it be merged? My code is almost no longer able to run properly on a 64-bit machine. 2147483648 => -2147483648, my timestamp cannot exceed 2038-01-19 11:14:08, I hope it can be merged soon. Thanks

It seems that this PR does not fix uint32？

brotherbigbao avatar Dec 28 '23 07:12 brotherbigbao

@zlodes @brotherbigbao @mkruskal-google

any remain work left to do with this pr ? uint32 fixed ?

calvin2021y avatar Feb 17 '24 12:02 calvin2021y

@calvin2021y I have no idea how to test it on 32bit OS, because it uses Bazel which cannot be complied ob 32bit OS.

So, do we really need 32bit support for now? 🌚

zlodes avatar Feb 17 '24 12:02 zlodes

I think it will be safe to merge without 32 bit fix. since most of php deploy on 64 bit platform. (we can just add note to explain 32 bit php not support)

calvin2021y avatar Feb 17 '24 12:02 calvin2021y

@mkruskal-google

Hi there! Is there a way to run tests for each commit here? There is no Bazel available for 32-bit systems, so, I tried several ways to build it under 32-bit system.

I just want to complete this PR. image

Also tried to run GH Actions in my own repository, but there are private images... :|

zlodes avatar Apr 21 '24 17:04 zlodes

any progress?

calvin2021y avatar May 30 '24 06:05 calvin2021y