Skip to content

THRIFT-5979: Add native method types to PHP Server and Factory classes#3459

Merged
sveneld merged 1 commit into
apache:masterfrom
sveneld:THRIFT-5979
May 11, 2026
Merged

THRIFT-5979: Add native method types to PHP Server and Factory classes#3459
sveneld merged 1 commit into
apache:masterfrom
sveneld:THRIFT-5979

Conversation

@sveneld
Copy link
Copy Markdown
Contributor

@sveneld sveneld commented May 11, 2026

Migrates the public methods on lib/php/lib/Server/ and lib/php/lib/Factory/ from PHPDoc @param/@return annotations to native parameter and return types, plus closes a long-standing factory gap that surfaced during cross-test debugging.

The Server abstractions and the protocol/transport factories are isolated subtrees — they reference TTransport/TProtocol only via composition, so adding native types here does not cascade out to the Transport or Protocol hierarchies (separate sub-tickets).

Server

  • TServer::serve() / stop() typed void.
  • TSimpleServer / TForkingServer match the new abstract signatures, and drop the now-dead if ($transport != null) guards after accept(), which is non-nullable.
  • TForkingServer's protected helpers (handleParent, handleChild, collectChildren) typed.
  • TServerTransport::listen() / close() / acceptImpl() / accept() typed; accept() now returns TTransport (it was already that semantically — it throws on null), acceptImpl() returns ?TTransport, the null check switched to strict ===.
  • TServerSocket overrides typed; acceptImpl() returns ?TSocket.
  • TSSLServerSocket::listen() typed; acceptImpl() returns ?TSSLSocket.

Factory

  • TProtocolFactory interface typed: getProtocol(TTransport): TProtocol.
  • TBinaryProtocolFactory / TCompactProtocolFactory / TJSONProtocolFactory match with covariant concrete return types.
  • TTransportFactoryInterface typed: getTransport(TTransport): TTransport.
  • TTransportFactory / TFramedTransportFactory match with covariant return types where appropriate.
  • New TBinaryProtocolAcceleratedFactory closes the long-standing gap where TBinaryProtocolAccelerated had no library-provided factory — the cross-test harness had to hand-roll one in test/php/TestServer.php. The ad-hoc class is dropped in favor of the library one. The new factory defaults strictWrite=true to match the protocol's own constructor default, distinguishing it from TBinaryProtocolFactory.

Test fixtures (latent issues surfaced by typed signatures)

  • ServerStub::serve/stop now typed void.
  • ServerTransportStub uses constructor property promotion with ?TTransport and matches abstract acceptImpl(): ?TTransport.
  • TSimpleServerTest had stubs of the wrong class for the factories (TServerTransport where TTransport/TProtocol was expected); fixed.
  • TForkingServerTest mocks had accept() returning null to simulate "no connection", which violated the now-typed TTransport return. Replaced with throwing TTransportException, which is what real accept() does on null and what the serve() loop already catches.
  • TBinaryProtocolAcceleratedFactoryTest covers the new factory (7 tests).

Part of THRIFT-5960 — modernizing PHP runtime library typing. Builds on the property typing (THRIFT-5976), constructor promotion (THRIFT-5977), and declare(strict_types=1) (THRIFT-5978).

  • Did you create an Apache Jira ticket? — THRIFT-5979
  • If a ticket exists: Does your pull request title follow the pattern "THRIFT-NNNN: describe my issue"? — yes
  • Did you squash your changes to a single commit? — yes
  • Did you do your best to avoid breaking changes? — yes; signatures are typed exactly where Server/Factory subtrees terminate, no public-API removal, covariant return narrowings only. The new TBinaryProtocolAcceleratedFactory is purely additive.
  • If your change does not involve any code, include [skip ci] anywhere in the commit message to free up build resources. — N/A (code change)

Generated-by: Claude Opus 4.7

@mergeable mergeable Bot added the php label May 11, 2026
@sveneld sveneld force-pushed the THRIFT-5979 branch 4 times, most recently from ff4555e to f633f23 Compare May 11, 2026 16:52
Client: php

Migrate the public methods on lib/php/lib/Server/ and lib/php/lib/Factory/
from PHPDoc @param/@return annotations to native parameter and return
types. The Server abstractions and the protocol/transport factories are
isolated subtrees -- they reference TTransport/TProtocol only via
composition, so adding native types here does not cascade out to the
Transport or Protocol hierarchies (separate sub-tickets).

Server:
* TServer::serve() / stop() typed void.
* TSimpleServer / TForkingServer match the new abstract signatures and
  drop the now-dead `if ($transport != null)` guards after accept(),
  which is non-nullable.
* TForkingServer's protected helpers (handleParent, handleChild,
  collectChildren) typed.
* TServerTransport::listen() / close() / acceptImpl() / accept() typed;
  accept() now returns TTransport (was already that semantically -- it
  throws on null), acceptImpl() returns ?TTransport, the null check
  switched to strict ===.
* TServerSocket overrides typed; acceptImpl() returns ?TSocket.
* TSSLServerSocket::listen() typed; acceptImpl() returns ?TSSLSocket.

Factory:
* TProtocolFactory interface typed: getProtocol(TTransport): TProtocol.
* TBinaryProtocolFactory / TCompactProtocolFactory / TJSONProtocolFactory
  match with covariant concrete return types.
* TTransportFactoryInterface typed: getTransport(TTransport): TTransport.
* TTransportFactory / TFramedTransportFactory match with covariant
  return types where appropriate.
* New TBinaryProtocolAcceleratedFactory closes the long-standing gap
  where TBinaryProtocolAccelerated had no library-provided factory --
  the cross-test harness had to hand-roll one in test/php/TestServer.php.
  The ad-hoc class is dropped in favor of the library one. The new
  factory defaults strictWrite=true to match the protocol's own ctor
  default, distinguishing it from TBinaryProtocolFactory.

Test fixtures (latent issues surfaced by typed signatures):
* ServerStub::serve/stop now typed void.
* ServerTransportStub uses constructor property promotion with
  ?TTransport and matches abstract acceptImpl(): ?TTransport.
* TSimpleServerTest had stubs of the wrong class for the factories
  (TServerTransport where TTransport/TProtocol was expected); fixed.
* TForkingServerTest mocks had accept() returning null to simulate
  "no connection", which violated the now-typed TTransport return.
  Replaced with throwing TTransportException, which is what real
  accept() does on null and what the serve() loop already catches.
* TBinaryProtocolAcceleratedFactoryTest covers the new factory.

Generated-by: Claude Opus 4.7
@sveneld sveneld marked this pull request as ready for review May 11, 2026 17:39
@sveneld sveneld merged commit 8da8cc2 into apache:master May 11, 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