Skip to content

THRIFT-5976: Add native types to PHP library properties (PHPDoc @var → declared types)#3455

Merged
sveneld merged 1 commit into
apache:masterfrom
sveneld:THRIFT-5976
May 10, 2026
Merged

THRIFT-5976: Add native types to PHP library properties (PHPDoc @var → declared types)#3455
sveneld merged 1 commit into
apache:masterfrom
sveneld:THRIFT-5976

Conversation

@sveneld
Copy link
Copy Markdown
Contributor

@sveneld sveneld commented May 9, 2026

Summary

Convert PHPDoc @var T annotations on properties throughout lib/php/lib/ to native PHP typed properties. PHP 8.1+ enforces these at runtime and phpstan can verify access. This is a no-LSP-cascade additive change — typed properties don't impose covariance constraints on subclasses beyond requiring redeclarations to use the same type.

JIRA: https://issues.apache.org/jira/browse/THRIFT-5976 (sub-task of THRIFT-5960).

Scope

  • 33 files in lib/php/lib/ (Base, ClassLoader, Exception, Factory, Protocol incl. JSON / SimpleJSON contexts, Server, Transport, StoredMessageProtocol, TMultiplexedProcessor) — properties typed bool / int / string / float / array / object plus class types (TTransport, TServerTransport, TProtocol, TTransportFactoryInterface, TProtocolFactory, …).
  • Resource handles (TPhpStream::$inStream/$outStream, TSocket::$handle, THttpClient::$handle, TServerSocket::$listener, TSSLSocket::$context) kept untyped with @var resource|null PHPDoc since PHP has no native resource type.
  • Static arrays (TBase::$tmethod, TException::$tmethod, TApplicationException::$tspec, TCompactProtocol::$ctypes/$ttypes) declared static array with shape annotations.
  • Constructor property promotion in JSON / SimpleJSON contexts and TBinaryProtocolFactory where the ctor body was simply argument→property assignment.
  • Dropped nullable-fiction on PairContext::$protocol — methods always dispatch through it, so null was never a valid state.
  • SimpleJSON\MapContext — remove the shadowing private $protocol (parent StructContext owns it; MapContext never reads it directly), type the constructor argument as TSimpleJSONProtocol and forward to parent.
  • TJSONProtocol::$context / $reader typed against BaseContext / LookaheadReader (short names; uses already present).

Notable fixes surfaced by typing

  • TSocket::$port = '9090' (string default while PHPDoc said int) → 9090.
  • TSocketPool::__construct now passes '' instead of null to parent TSocket::__construct since TSocket::$host became typed string. $this->host is reassigned per-server in TSocketPool::open(), so the empty default is never observed externally.
  • TCurlClient::$response typed string|false|null to reflect both the null-initialised state and curl_exec's string|false return.
  • TMultiplexedProcessor::$serviceProcessorMap was implicitly null and promoted to a map on first registerProcessor call; explicitly initialised to [].

Test fixtures

  • JsonProtocolStub now extends TJSONProtocol (concrete class, no abstract methods to satisfy) so the context properties can be typed concretely. Keeps only the readJSONSyntaxChar override that records call arguments.
  • SimpleJsonProtocolStub removed — after being rewritten as extends TSimpleJSONProtocol with no overrides it was equivalent to the real class. Tests now instantiate TSimpleJSONProtocol directly.

Out of scope (separate sub-tickets under THRIFT-5960)

  • Method parameter / return types.
  • declare(strict_types=1) rollout.
  • PHP code generator (t_php_generator.cc) changes.
  • phpstan ratchet level 1 → 5.

Verification (in PHP 8.4 / composer 2 / phpstan 1.12 / phpunit 13 docker)

composer phpstan         OK (level 1, baseline unchanged)
vendor/bin/phpcs         OK
phpunit Unit suite       629 tests, 1907 assertions, 0 errors
phpunit Integration      106 tests,  166 assertions, 0 errors

Checklist

  • Did you create an Apache Jira ticket? — THRIFT-5976
  • If a ticket exists: Does your pull request title follow the pattern "THRIFT-NNNN: describe my issue"?
  • Did you squash your changes to a single commit?
  • Did you do your best to avoid breaking changes? — yes; native typed properties tighten what was already documented in PHPDoc. Edge cases (timeout int|float|null, resource-typed handles, mock-friendly contexts) preserved.
  • If your change does not involve any code, include [skip ci] anywhere in the commit message to free up build resources.

@mergeable mergeable Bot added the php label May 9, 2026
@sveneld sveneld marked this pull request as draft May 9, 2026 17:34
Client: php

Convert PHPDoc `@var T` annotations on properties throughout
`lib/php/lib/` to native PHP typed properties. PHP 8.1+ enforces these
at runtime, and phpstan can verify access. This is a no-LSP-cascade
additive change: typed properties don't impose covariance constraints
on subclasses beyond requiring a redeclaration to use the same type.

Scope (33 files in `lib/php/lib/` + 2 test fixtures):

* Properties typed natively: bool / int / string / float / array / object
  + class types (TTransport, TServerTransport, TProtocol,
  TTransportFactoryInterface, TProtocolFactory, …) across Base, Exception,
  Factory, Protocol (incl. JSON / SimpleJSON contexts), Server, Transport,
  StoredMessageProtocol, TMultiplexedProcessor, ClassLoader.
* Resource handles (`TPhpStream::$inStream/$outStream`, `TSocket::$handle`,
  `THttpClient::$handle`, `TServerSocket::$listener`, `TSSLSocket::$context`)
  kept untyped with `@var resource|null` since PHP has no native `resource`
  type.
* Static `$tmethod` / `$tspec` arrays declared as `static array` with
  shape-PHPDoc.
* Constructor property promotion in JSON / SimpleJSON contexts and
  factories where the ctor body was simply argument→property assignment.
* Drop the `?TJSONProtocol = null` nullable on `PairContext::$protocol`:
  the methods immediately dispatch through it, so null was never a valid
  state. Same property on the rest of the JSON / SimpleJSON contexts is
  now non-nullable too.
* `SimpleJSON\MapContext`: remove the shadowing private `$protocol`
  (parent `StructContext` owns it; `MapContext` never reads it directly),
  type the constructor argument as `TSimpleJSONProtocol` and forward to
  parent.
* `TJSONProtocol::$context` / `$reader` typed against the existing
  `BaseContext` / `LookaheadReader` imports (short names).

Notable fixes surfaced by typing:

* `TSocket::$port = '9090'` (string default while PHPDoc said int) → 9090.
* `TSocketPool::__construct` now passes `''` instead of `null` to parent
  `TSocket::__construct` since `TSocket::$host` became typed `string`.
  `$this->host` is reassigned per-server in `TSocketPool::open()` so the
  empty default is never observed externally.
* `TCurlClient::$response` typed `string|false|null` to reflect both the
  null-initialised state and `curl_exec`'s `string|false` return type.
* `TMultiplexedProcessor::$serviceProcessorMap` was implicitly null and
  promoted to a map on first `registerProcessor` call; explicitly
  initialised to `[]`.

Test fixtures:

* `lib/php/test/Unit/Lib/Protocol/Fixture/JsonProtocolStub.php`: extends
  `TJSONProtocol` (concrete class, no abstract methods to satisfy) so the
  context properties can be typed concretely. Keeps only the
  `readJSONSyntaxChar` override that records call arguments for inspection.
* `lib/php/test/Unit/Lib/Protocol/Fixture/SimpleJsonProtocolStub.php`:
  removed. After being rewritten as `extends TSimpleJSONProtocol` with no
  overrides it was equivalent to the real class; tests now instantiate
  `TSimpleJSONProtocol` directly.

Out of scope (deferred follow-ups):

* Method parameter / return types — separate sub-tickets under
  THRIFT-5960.
* `declare(strict_types=1)` rollout — separate sub-ticket.
* PHP code generator (`compiler/cpp/src/thrift/generate/t_php_generator.cc`)
  changes — separate sub-ticket.
* phpstan ratchet level 1 → 5 — separate sub-ticket.

Verification (PHP 8.4 + composer 2 + phpstan 1.12 + phpunit 13 in docker):

  composer phpstan         OK (level 1, baseline unchanged)
  vendor/bin/phpcs         OK
  phpunit Unit suite       629 tests, 1907 assertions, 0 errors
  phpunit Integration      106 tests,  166 assertions, 0 errors

Parent: THRIFT-5960

Generated-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@sveneld sveneld marked this pull request as ready for review May 10, 2026 12:07
@sveneld sveneld merged commit c774fab into apache:master May 10, 2026
93 of 94 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant