Skip to content

THRIFT-5980: Add native method types to PHP Transport hierarchy#3460

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

THRIFT-5980: Add native method types to PHP Transport hierarchy#3460
sveneld merged 1 commit into
apache:masterfrom
sveneld:THRIFT-5980

Conversation

@sveneld
Copy link
Copy Markdown
Contributor

@sveneld sveneld commented May 11, 2026

Migrates the public methods on lib/php/lib/Transport/ from PHPDoc @param/@return annotations to native parameter and return types, plus fixes a long-standing correctness bug in TJSONProtocol::writeJSONDouble that strict-mode typing surfaced.

Completes the typing trajectory for the Transport subtree started by THRIFT-5976 (properties), THRIFT-5977 (ctor promotion), THRIFT-5978 (strict_types) and THRIFT-5979 (Server + Factory method types).

Library

  • TTransport abstractisOpen(): bool, open(): void, close(): void, read(int): string, readAll(int): string, write(string): void, flush(): void.
  • TSocket / TSSLSocket / TSocketPool — open/close/read/write/flush typed; setSendTimeout / setRecvTimeout / setDebug / setNumRetries / etc typed; getHost / getPort typed; setHandle return typed.
  • TBufferedTransport / TFramedTransport / TMemoryBuffer / TNullTransport / TPhpStream — all method overrides typed to match the abstract.
  • TFramedTransport::write keeps the legacy ?int $len = null optional parameter so existing internal callers in TBinaryProtocol / TCompactProtocol still type-check; the non-write path now delegates with just the buffer.
  • TCurlClient / THttpClient — read/write/flush/close/open typed; timeout setters typed as ?float (int widens automatically); addHeaders typed array.

NaN / Infinity round-trip fix

TJSONProtocol::readJSONDouble already parses the quoted strings "NaN"NAN and "Infinity"INF (Thrift JSON convention), but writeJSONDouble previously called json_encode($num) which returns false for NaN/INF; once typed write(string) rejects false, this used to silently emit empty output ({"dbl":}) — locked into integration tests with a #TODO Should be fixed in future comment.

Now writes the conventional quoted strings:

  • NAN"NaN"
  • INF"Infinity"
  • -INF in TJSONProtocolTProtocolException::INVALID_DATA (the read side has no inverse).
  • -INF in TSimpleJSONProtocol"-Infinity" symmetrically (write-only protocol).

The TODO is resolved; integration tests updated to assert the correct outputs.

Other side fixes surfaced by typing

  • TSocket::setSendTimeout / setRecvTimeout: switched from floor($timeout / 1000) to intdiv($timeout, 1000) so the typed int property assignment is exact (floor returns float).
  • TCurlClient::$timeout / $connectionTimeout and THttpClient::$timeout tightened from float|int|null to ?float (int widens automatically per PHP rules).

Tests

  • TBinaryProtocolTest / TCompactProtocolTest / TBufferedTransportTest / TFramedTransportTest — dropped ->willReturn(N) from mocks of the now-void write/open/close/flush. Behavior unchanged; PHPUnit refuses to mock returning a value for void methods.
  • TFramedTransportTest::testWrite — mock with('12345', 5) updated to with('12345') after the non-write path now passes only the buffer to the underlying transport.
  • TCurlClientTest — timeout data provider switched to float literals (10.0 instead of 10) to match the now-typed ?float properties.
  • Integration TJSONProtocolTest / TSimpleJSONProtocolTest — NaN / Infinity write assertions updated from the broken {"dbl":} / {"thing":} outputs to the correct round-trippable strings.

Part of THRIFT-5960 — modernizing PHP runtime library typing.

  • Did you create an Apache Jira ticket? — THRIFT-5980
  • 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 for the typing portion. The NaN/Infinity write change is a wire-format fix that aligns PHP with the Thrift JSON convention already used by TJSONProtocol::readJSONDouble; previously this path produced unreadable output, so callers depending on it were already broken.
  • 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-5980 branch 4 times, most recently from 071bec3 to 84c602a Compare May 11, 2026 19:14
Client: php

Migrate the public methods on lib/php/lib/Transport/ from PHPDoc
@param/@return annotations to native parameter and return types. This
completes the typing trajectory for the Transport subtree started by
THRIFT-5976 (properties), THRIFT-5977 (ctor promotion), THRIFT-5978
(strict_types) and THRIFT-5979 (Server + Factory method types).

Library:
* TTransport abstract: isOpen(): bool, open(): void, close(): void,
  read(int): string, readAll(int): string, write(string): void,
  flush(): void.
* TSocket / TSSLSocket / TSocketPool: open/close/read/write/flush typed;
  setSendTimeout/setRecvTimeout/setDebug/setNumRetries/etc typed; getHost
  /getPort typed; setHandle typed return.
* TBufferedTransport / TFramedTransport / TMemoryBuffer / TNullTransport
  / TPhpStream: all method overrides typed to match the abstract.
* TFramedTransport::write keeps the legacy `?int $len = null` optional
  parameter so existing internal callers in TBinaryProtocol /
  TCompactProtocol still type-check; the non-write path now delegates
  with just the buffer.
* TCurlClient / THttpClient: read/write/flush/close/open typed; timeout
  setters typed as ?float (int widens automatically); addHeaders typed
  array.

Full NaN / +Infinity / -Infinity round-trip for the JSON protocols:
* TJSONProtocol now writes all three IEEE-754 sentinels as the
  conventional quoted tokens ("NaN", "Infinity", "-Infinity") matching
  the Java and C++ TJSONProtocol implementations; the read side gains
  a "-Infinity" branch so the round-trip is symmetric. Token literals
  are hoisted to TOKEN_NAN / TOKEN_POS_INFINITY / TOKEN_NEG_INFINITY
  consts on TJSONProtocol following the existing NAME_* convention,
  so writer and reader can never drift.
* TSimpleJSONProtocol reuses TJSONProtocol's token constants and emits
  the same three tokens for its write-only path.
* The previously corrupted `{"dbl":}` / `{"thing":}` output for non-
  finite doubles (locked into integration tests with #TODO markers) is
  fixed; integration tests now assert the correct round-trippable
  strings, plus a -Infinity case is added on both read and write paths.

Side fixes surfaced by typed transports:
* TSocket::setSendTimeout/setRecvTimeout switched to intdiv() so the
  typed int property assignment is exact (floor returns float).
* TCurlClient::$timeout / $connectionTimeout and THttpClient::$timeout
  tightened from float|int|null to ?float (int widens automatically).
* TBufferedTransport / TFramedTransport: stale "wBuf_" comments
  referring to the old C++-style property name dropped.

Tests:
* TBinaryProtocolTest / TCompactProtocolTest / TBufferedTransportTest
  / TFramedTransportTest: dropped `->willReturn(N)` from mocks of the
  now-void write/open/close/flush. Behavior unchanged; PHPUnit refuses
  to mock returning a value for void methods.
* TFramedTransportTest::testWrite mock with('12345', 5) updated to
  with('12345') after the non-write path now passes only the buffer.
* TCurlClientTest timeout data provider switched to float literals
  (10.0 instead of 10) to match the now-typed ?float properties.
* TJSONProtocolTest gains round-trip cases for NaN, +Infinity, and
  -Infinity through writeAndReadScalarDataProvider.

Generated-by: Claude Opus 4.7
@sveneld sveneld marked this pull request as ready for review May 11, 2026 20:04
@sveneld sveneld merged commit 8561c4e 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