Skip to content

THRIFT-6004: Emit native types on generated PHP service-level methods#3483

Merged
sveneld merged 1 commit into
apache:masterfrom
sveneld:THRIFT-6004
May 15, 2026
Merged

THRIFT-6004: Emit native types on generated PHP service-level methods#3483
sveneld merged 1 commit into
apache:masterfrom
sveneld:THRIFT-6004

Conversation

@sveneld
Copy link
Copy Markdown
Contributor

@sveneld sveneld commented May 15, 2026

Update the PHP generator to emit native parameter and return types on every generated service-level method. Completes the typing pass on the generator side after THRIFT-5986 (declare strict_types), THRIFT-5990 (struct return types), and THRIFT-5991 (struct property types).

Generated signatures (before → after)

-public function testString($thing)                      // Interface
+public function testString(?string $thing): ?string

-public function __construct($input, $output = null)     // Client ctor
+public function __construct(TProtocol $input, ?TProtocol $output = null)

-public function send_testString($thing)                 // Client
+public function send_testString(?string $thing): void

-public function recv_testString()                       // Client
+public function recv_testString(): ?string

-public function __construct($handler)                   // Processor ctor
+public function __construct(object $handler)

-public function process($input, $output)                // Processor
+public function process(TProtocol $input, TProtocol $output): bool

-protected function process_testString($seqid, $input, $output)
+protected function process_testString(int $seqid, TProtocol $input, TProtocol $output): void

-public function __construct($impl)                      // Rest ctor
+public function __construct(object $impl)

-public function testString($request)                    // Rest method
+public function testString(array $request): ?string

Implementation

  • New helper type_to_return(t_type*) returning : void or : ?T. Shares the existing type_to_native() mapping introduced by THRIFT-5991.
  • function_signature() now appends type_to_return() for the return clause.
  • argument_list() now emits ?type_to_native() on every parameter — previously only struct/container parameters were typed.
  • send_* constructs a synthetic t_function(g_type_void, ...) so function_signature() produces the correct : void for the writer half.
  • Processor process() early "method not found" path now return false; instead of bare return; to satisfy : bool.
  • Rest method emission gates return on whether the underlying impl is void.

Drive-by cleanup

ThriftClassLoader::findFileInApcu() (merged in THRIFT-6001) was flagged by PHPStan: the trailing $file !== false ? $file : null was unreachable because the if-branch reassigns $file to findFile() (?string). Rewrote the body to a clear two-step fetch-or-recompute and narrowed the return via is_string().

Validation (Docker thrift-php-dev:local + thrift/thrift-build:ubuntu-focal)

  • Built compiler with the patched generator.
  • Regenerated all 6 fixture variants under lib/php/test/Resources/packages/ (gitignored — regen happens in CI).
  • phpcs — 0 errors
  • phpstan (level 5) — 0 errors
  • phpunit Unit Suite — 637 tests, 0 failures
  • phpunit Integration Suite — 108 tests, 0 failures

Part of umbrella THRIFT-5960.

  • Apache Jira ticket — THRIFT-6004
  • PR title follows the pattern "THRIFT-NNNN: …"
  • Squashed to a single commit
  • Did you do your best to avoid breaking changes? — third-party user code calling generated services with the wrong types will now fail loudly under strict types. Required regen of generated code against the new compiler.

Generated-by: Claude Opus 4.7

Client: php

Update t_php_generator to emit native PHP parameter and return types on
all generated service-level code:

* Interface (*If.php): public methods now declare typed params and
  return — `foo(?string $thing): ?string`.
* Client (*Client.php): typed constructor
  `__construct(TProtocol $input, ?TProtocol $output = null)`; typed
  `$input`/`$output`/`$seqid` properties; typed RPC methods, typed
  `send_*` (synthetic t_function with g_type_void return), typed
  `recv_*` (already synthetic; now inherits the parent
  function_signature return-type machinery).
* Processor (*Processor.php): typed
  `__construct(object $handler)`; typed
  `protected ?object $handler_ = null`; typed
  `process(TProtocol $input, TProtocol $output): bool` (changed the
  early "method not found" `return;` to `return false;` to satisfy the
  declared return type); typed
  `process_NAME(int $seqid, TProtocol $input, TProtocol $output): void`.
* Rest (*Rest.php): typed `__construct(object $impl)`; typed
  `protected object $impl;`; typed method signatures
  `methodName(array $request): ?T`; emit `return` only when the
  underlying impl method returns non-void.

Implementation
--------------

* New helper `type_to_return(t_type*)` renders `: void` or `: ?T` for
  return-type clauses, sharing the existing `type_to_native()` mapping.
* `function_signature()` extended to append `type_to_return()` for the
  return type; `argument_list()` extended to emit `?type_to_native()`
  on every parameter (previously only struct/container types were
  typed).
* `send_*` emission now constructs a synthetic
  `t_function(g_type_void, ...)` so `function_signature()` produces
  the correct `: void` for the writer half of the RPC.

Drive-by cleanup
----------------

`ThriftClassLoader::findFileInApcu()` (merged in THRIFT-6001) was
flagged by PHPStan: the trailing `$file !== false ? $file : null`
arm is unreachable because the if-branch reassigns `$file` to the
findFile() result (`?string`). Rewrote the body to a clear two-step
fetch-or-recompute and narrowed the return through `is_string()` to
satisfy the analyser without relying on a phantom else-arm.

Generated fixtures regenerate via `make stubs` in CI; nothing
checked in changes.

Part of umbrella THRIFT-5960 (PHP modernization).

Generated-by: Claude Opus 4.7
@sveneld sveneld marked this pull request as ready for review May 15, 2026 17:15
@sveneld sveneld merged commit c179da4 into apache:master May 15, 2026
97 of 98 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant