Modernize PHP support from PHP 7.1+ to PHP 8.2+

[greggdonovan]

Summary

This PR modernizes Thrift's PHP support from PHP 7.1+ to PHP 8.2+ as the minimum baseline, adding full type declarations throughout the library and code generator.

Infrastructure Changes

• Updated composer.json: PHP ^8.2, PHPUnit ^10.5 || ^11.0, added Phan ^5.4 for static analysis
• Updated CI PHP version matrix to [8.2, 8.3, 8.4, 8.5]
• Updated phpunit.xml for PHPUnit 10+ compatibility (replaced deprecated attributes)
• Added Phan static analysis configuration at lib/php/.phan/config.php

Library Modernization

• Added typed properties and method signatures to all 57 PHP library files
• Type classes now use public const int typed constants
• Transport layer: typed abstract methods (isOpen(): bool, read(int $len): string, etc.)
• Protocol layer: typed write/read methods with : int return types
• Server and Factory layers: typed properties and method signatures
• Tests: Converted @dataProvider annotations to #[DataProvider] attributes for PHPUnit 10+

Code Generator Changes

• Added php_type_declaration() helper function for Thrift-to-PHP type mapping
• Enum constants now emit public const int NAME = value;
• Struct properties now use nullable types (?string $name = null)
• Constructor parameters typed (?array $vals = null)
• read() and write() methods now have : int return types

Cross-Test Integration

• Added lib-php to cross-test job dependencies
• Added php to server_lang and client_lang matrices
• Added PHP 8.2 setup step with required extensions
• Updated test/php/Handler.php with full type declarations

Test plan

• CI passes for PHP 8.2, 8.3, 8.4, 8.5
• Phan static analysis passes
• PHPUnit tests pass
• Cross-tests with PHP server/client work

🤖 Generated with Claude Code

[greptile-apps]

Greptile Overview

Greptile Summary

This PR modernizes Thrift's PHP support from 7.1+ to 8.2+ as the minimum baseline, adding comprehensive type declarations throughout the library and code generator.

Major Changes:

• Infrastructure: Updated to PHP 8.2+, PHPUnit 10+, added Phan static analysis
• Library: Added typed properties and method signatures to all 57 PHP files
• Code Generator: Added php_type_declaration() helper to emit typed properties and methods
• Tests: Migrated to PHPUnit 10+ attributes (#[DataProvider]) and made data providers static
• Cross-Tests: Integrated PHP 8.2 into CI cross-test matrix

Key Implementation Details:

• Type constants now use public const int declarations
• Transport/Protocol methods have explicit return types (: bool, : int, : string, : void)
• Generated struct properties use nullable types with ? prefix
• Read/write methods return : int for byte counts
• Phan configuration includes strict checking with PhanUndeclaredProperty suppressed for dynamic properties

Issues Found:

• The code generator unconditionally adds ? prefix to all struct properties (line 962 in t_php_generator.cc), making all fields nullable regardless of whether they're optional in the Thrift definition. This may not match the intended semantics for required fields.

Confidence Score: 4/5

• PR is mostly safe to merge with one semantic issue to address regarding nullable struct properties
• The modernization is well-executed with comprehensive type declarations, proper PHPUnit migration, and good CI integration. However, the unconditional nullable struct properties may create semantic mismatches with required fields in Thrift definitions
• Pay close attention to compiler/cpp/src/thrift/generate/t_php_generator.cc line 962 regarding nullable property generation logic

Important Files Changed

Filename	Overview
compiler/cpp/src/thrift/generate/t_php_generator.cc	Added PHP 8.2+ type declaration support with new php_type_declaration() helper, typed enum constants, nullable struct properties, and : int return types for read/write methods
composer.json	Updated PHP requirement to ^8.2, PHPUnit to `^10.5
.github/workflows/build.yml	Updated PHP matrix to [8.2, 8.3, 8.4, 8.5], added Phan static analysis step, added PHP to cross-test matrices with PHP 8.2 setup
lib/php/lib/Transport/TTransport.php	Added complete type declarations: isOpen(): bool, read(int $len): string, write(string $buf): void, etc.
lib/php/lib/Protocol/TProtocol.php	Added typed property protected TTransport $trans_ and method signatures with : int return types for all write/read methods

Sequence Diagram

sequenceDiagram
    participant Dev as Developer
    participant Compiler as Thrift Compiler
    participant Generator as t_php_generator.cc
    participant PHP as Generated PHP Code
    participant PHPUnit as PHPUnit 10+
    participant Phan as Phan Analyzer

    Dev->>Compiler: Define .thrift file with structs/services
    Compiler->>Generator: Parse AST and invoke PHP generator
    
    Generator->>Generator: Call php_type_declaration() for each field
    Note over Generator: Maps Thrift types to PHP 8.2+ types<br/>(string, int, bool, float, array, \Class)
    
    Generator->>PHP: Generate struct with nullable typed properties<br/>public ?string $field = null;
    Generator->>PHP: Generate read/write methods<br/>public function read($input): int
    Generator->>PHP: Generate enum constants<br/>public const int NAME = value;
    
    Dev->>PHPUnit: Run test suite
    PHPUnit->>PHP: Execute tests with #[DataProvider] attributes
    PHP-->>PHPUnit: Return typed values
    PHPUnit-->>Dev: Test results
    
    Dev->>Phan: Run static analysis
    Phan->>PHP: Analyze type declarations and compatibility
    Note over Phan: Strict checking enabled<br/>PhanUndeclaredProperty suppressed
    Phan-->>Dev: Analysis results
    
    Dev->>Dev: CI runs PHP 8.2-8.5 matrix
    Note over Dev: Cross-tests with Java, Go, Rust, C++

Loading

[greptile-apps]

_{5 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

Unconditionally prefixing ? to all types creates invalid PHP syntax for certain combinations. For example, ?array already indicates nullability, so properties become public ?array $field = null; which is correct. However, for struct types that return \Namespace\ClassName, the generated code becomes public ?\Namespace\ClassName $field = null;, which is syntactically correct but semantically questionable if the field is required (non-optional in the Thrift definition). The ? should only be added for optional fields or when the field can truly be null.

Prompt To Fix With AI

This is a comment left during a code review.
Path: compiler/cpp/src/thrift/generate/t_php_generator.cc
Line: 962:962

Comment:
Unconditionally prefixing `?` to all types creates invalid PHP syntax for certain combinations. For example, `?array` already indicates nullability, so properties become `public ?array $field = null;` which is correct. However, for struct types that return `\Namespace\ClassName`, the generated code becomes `public ?\Namespace\ClassName $field = null;`, which is syntactically correct but semantically questionable if the field is required (non-optional in the Thrift definition). The `?` should only be added for optional fields or when the field can truly be null.

How can I resolve this? If you propose a fix, please make it concise.