Skip to content

Conversation

@bshaffer
Copy link
Contributor

@bshaffer bshaffer commented Jan 12, 2026

This PR modifies php_generator.cc to:

  • Add PHP typehints to setter method signatures for all types (for 64-bit integers, the type hint is int|string to maintain compatibility with 32-bit systems)
  • Remove redundant GPBUtil::check... for primitive types and messages (the PHP type hint provides sufficient validation)
  • Retain GPBUtil::check... for 32 and 64-bit integers, strings (for UTF-8 validation), enums, maps, and repeated fields

Additionally makes the following changes:

  • Updates descriptor protos with generator changes
  • Suppresses float to int "loss of precision" warnings in tests which expect it
  • Removes compatibility in tests with PHPUnit 6, which is no longer needed
  • Proper casing in gencode for true and false (instead of True and False)

… checks

This commit modifies the php_generator.cc file to:
- Add PHP typehints to setter method signatures for all types (for 64-bit integers, the type hint is 'int|string' to maintain compatibility with 32-bit systems)
- Remove redundant GPBUtil::check... for primitive types (the PHP type hint provides sufficient validation)
- Retain GPBUtil::check... for 32 and 64-bit integers, strings (for UTF-8 validation), messages, enums, maps, and repeated fields
@bshaffer bshaffer requested a review from a team as a code owner January 12, 2026 22:16
@bshaffer bshaffer requested review from ericsalo and removed request for a team January 12, 2026 22:16
@bshaffer bshaffer added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jan 12, 2026
@github-actions github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jan 12, 2026
@mkruskal-google
Copy link
Member

Looks like you have some test failures on 32-bit and aarch64 :(

@bshaffer bshaffer added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jan 13, 2026
@github-actions github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jan 13, 2026
@bshaffer bshaffer added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jan 13, 2026
@github-actions github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jan 13, 2026
@bshaffer bshaffer added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jan 13, 2026
@github-actions github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jan 13, 2026
@bshaffer
Copy link
Contributor Author

@mkruskal-google all passing now!

@bshaffer bshaffer added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jan 13, 2026
@github-actions github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jan 13, 2026
@anandolee
Copy link
Contributor

Looks like still failing?

@bshaffer
Copy link
Contributor Author

@anandolee no, they're passing.. looks like the job was stuck

@bshaffer
Copy link
Contributor Author

bshaffer commented Jan 13, 2026

@mkruskal-google one thing worth consideration - the calls to GPBUtil::check... have been removed for bool, float, and double. That means the following methods are no longer being called:

    public static function checkFloat(&$var)
    {
        if (is_float($var) || is_numeric($var)) {
            $var = unpack("f", pack("f", $var))[1];
        } else {
            throw new \Exception("Expect float.");
        }
    }

    public static function checkDouble(&$var)
    {
        if (is_float($var) || is_numeric($var)) {
            $var = floatval($var);
        } else {
            throw new \Exception("Expect float.");
        }
    }

    public static function checkBool(&$var)
    {
        if (is_array($var) || is_object($var)) {
            throw new \Exception("Expect boolean.");
        }
        $var = boolval($var);
    }
  • in checkBool, the bool typehint should exhibit the same behavior thanks to the is_array($var) || is_object($var) check
  • in checkFloat, the is_float and is_numeric will perform the same as the typecheck, but the unpack("f", pack("f", $var))[1]; is converting the 64-bit PHP float to a 32-bit float. That needs to be preserved.
  • in checkDouble, the floatval result should exhibit the same behavior as the typehint

@bshaffer
Copy link
Contributor Author

bshaffer commented Jan 14, 2026

I have added back checkFloat, as the casting to a 32-bit float is critical. The other two (checkBool and checkFloat) aren't doing anything important

breaking change implication

Anyone who is running gencode in strict mode (e.g. declare(strict_types=1);) will experience a breaking change, as the float and bool type hints will reject everything outside those types (such as numeric and boolean strings). However, I believe this is WAI

@bshaffer bshaffer added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jan 14, 2026
@github-actions github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jan 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants