Skip to content

THRIFT-5977: Apply constructor property promotion in PHP runtime library#3456

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

THRIFT-5977: Apply constructor property promotion in PHP runtime library#3456
sveneld merged 1 commit into
apache:masterfrom
sveneld:THRIFT-5977

Conversation

@sveneld
Copy link
Copy Markdown
Contributor

@sveneld sveneld commented May 10, 2026

Summary

Follow-up to THRIFT-5976 (typed properties). After that work, many constructors in lib/php/lib/ were pure 1:1 argument→property assignments — exactly what PHP 8 constructor property promotion replaces. Apply promotion across the library: cleaner declarations, no behavioural change, no LSP impact.

JIRA: https://issues.apache.org/jira/browse/THRIFT-5977 (related to THRIFT-5960).

Scope

Full promote (ctor body becomes empty or a single parent call)

  • Factory/TBinaryProtocolFactorystrictRead, strictWrite.
  • Server/TServer (6 properties) — processor, transport, four factory parameters.
  • Server/TServerSockethost, port (acceptTimeout retained as a regular property, mutated via setAcceptTimeout()).
  • Transport/TMemoryBufferbuf.
  • Transport/TBufferedTransporttransport, rBufSize, wBufSize.
  • Transport/TFramedTransporttransport, read, write.
  • ClassLoader/ThriftClassLoaderapcu, apcu_prefix.

Partial promote (body keeps parent::__construct(...) or one non-trivial line)

  • Protocol/TBinaryProtocolstrictRead, strictWrite; $trans forwarded to parent.
  • Protocol/TMultiplexedProtocolserviceName; $protocol forwarded.
  • StoredMessageProtocolfname, mtype, rseqid; $protocol forwarded.
  • Transport/TSockethost, port, persist; debugHandler retains the ?: 'error_log' fallback in the body.
  • Transport/THttpClienthost, port, scheme, context; uri keeps its prefix-normalisation in the body.
  • Transport/TCurlClienthost, port, scheme; same uri logic.

Bonus fixes surfaced while promoting

  • Transport/TSSLSocket was bypassing parent::__construct entirely. Now properly delegates parent::__construct(self::getSSLHost($host), $port, false, $debugHandler), ensuring TSocket's promoted properties get initialised. getSSLHost made private static since it doesn't use $this.
  • Server/TSSLServerSocket — same pattern: parent::__construct(self::getSSLHost($host), $port); getSSLHost made public static.

Out of scope (intentionally skipped)

  • TPhpStream — bitwise ($mode & MODE_R/MODE_W) ctor logic, not 1:1.
  • Exception hierarchy (TException, TApplicationException, TProtocolException, TTransportException) — dual-mode bridge constructor.
  • TBase, TSimpleJSONProtocol, TJSONProtocol — non-trivial init bodies ($context = new Context(), $reader = new LookaheadReader(), conditional spec/vals processing).
  • TSocketPool — ctor builds $servers and normalises $ports; not 1:1.
  • TBinaryProtocolAccelerated — passes through to parent unchanged after the parent promotion.

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

Checklist

  • Did you create an Apache Jira ticket? — THRIFT-5977
  • 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; promotion is purely syntactic. The only observable diff is that TSSLSocket and TSSLServerSocket now correctly call parent::__construct, which initialises previously-uninitialised inherited typed properties (no functional behaviour change for callers since the SSL classes overrode every method that read those properties).
  • If your change does not involve any code, include [skip ci] anywhere in the commit message to free up build resources.

Client: php

Convert constructors that were pure 1:1 argument→property assignments
to PHP 8 promoted constructor parameters. Pure syntactic refactor with
no behavioural change.

Files (16):
* Factory/TBinaryProtocolFactory, Server/TServer, Server/TServerSocket,
  ClassLoader/ThriftClassLoader, Transport/{TBufferedTransport,
  TFramedTransport, TMemoryBuffer, THttpClient, TCurlClient, TSocket,
  TSSLSocket, Server/TSSLServerSocket} — full or partial promotion.
* Protocol/{TBinaryProtocol, TMultiplexedProtocol},
  StoredMessageProtocol — partial promote (parent::__construct kept).

Bonus fixes surfaced while promoting:
* TFramedTransport: drop the `?TTransport = null` nullable-fiction —
  every method dispatches through `$this->transport->...` with no
  null guard. Now `TTransport` (required); all in-repo call sites
  already pass a transport.
* TSSLSocket / TSSLServerSocket previously bypassed `parent::__construct`
  and assigned inherited properties directly. After 5976 typed those
  parent properties this would leave `$persist` uninitialised in the
  SSL classes. Now both delegate to parent and only initialise the
  SSL-specific `$context`.
* TSocket: replace the `'error_log'` magic string with a public
  `DEFAULT_DEBUG_HANDLER` class constant and switch the fallback to
  `??` for null-default semantics.
* THttpClient / TCurlClient: simplify the URI prefix-normalisation
  with `str_starts_with()`.
* The duplicated SSL host-prefix helper is extracted as a small
  private `ensureSslHostPrefix()` method on each SSL class
  (TSSLSocket and TSSLServerSocket) so the bare `parent::__construct(
  $this->ensureSslHostPrefix($host), …)` call reads cleanly.

Verified inside docker (PHP 8.4 + composer 2 + phpstan 1.12 + phpunit 13):
  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 15:11
@sveneld sveneld merged commit 01e8b4a 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