THRIFT-5959: Adopt PSR-12 in PHP library and align generator emission

[sveneld]

Summary

Implements THRIFT-5959: adopts PSR-12 across the PHP library and aligns the C++ generator emission with the same standard.

PSR-2 has been frozen since 2019. PSR-12 is its modern successor and what squizlabs/php_codesniffer ^3.10 (already a project dependency) supports out of the box. While reformatting, this PR also cleans up the PHP-only leading/trailing-underscore identifier conventions that the runtime and generator inherited from the original PHP port — every other Thrift language target (Python thrift_spec, Java metaDataMap, Ruby FIELDS, Go/C# Read/Write) uses neither leading nor trailing underscores.

The lint scope now covers the full lib/php/lib/, lib/php/test/, and the regenerated lib/php/test/Resources/packages/ tree (435 fixture files); vendor/bin/phpcs exits 0. Runtime behaviour is unchanged — the diff is renames, formatting, generator output, tooling, and docs.

What changed

Tooling

• phpcs.xml.dist:
  • Ruleset PSR2 → PSR12.
  • Both legacy underscore exclusions removed.
  • Added Generic.Arrays.DisallowLongArraySyntax.
  • Removed the blanket <exclude-pattern>lib/php/test/Resources/packages/*</exclude-pattern>; the regenerated fixtures now lint cleanly via inline phpcs:disable directives in the autogen comment (see below).
• lib/php/coding_standards.md: PSR-2 link replaced with PSR-12; explicit note that Apache Thrift PHP code does not use leading or trailing underscores on identifiers.

Runtime renames (lib/php/lib/)

• public static $_TSPEC → public static $tspec on every generated struct and on TApplicationException. Mirrors Python's thrift_spec.
• Protected _read / _write helpers on TBase and TException → readStruct / writeStruct (the new names avoid clashing with the public abstract read / write, which take a different signature).
• Private helpers _readMap / _readList / _writeMap / _writeList → drop the leading _.
• ~85 trailing-underscore properties across 30 files (TServer, TSocket, TBufferedTransport, TBinaryProtocol, THttpClient, TCurlClient, TServerSocket, TSSLSocket, TFramedTransport, TPhpStream, TJSONProtocol, etc.) → drop the trailing _ (~509 internal reference updates).
• TJSONProtocol::$contextStack / $context / $reader: rename and tighten visibility from public to private. An audit found no callers outside the class — every reference was internal $this->... access — so a deprecation shim was not needed and was deliberately not introduced.
• One-letter $p private property in JSON / SimpleJSON context classes → descriptive $protocol.

Hand-written test fixtures

TestSerializerStruct.php, TestRichException.php, NestedStruct.php, ComplexStruct.php, and TBaseTest.php updated alongside the runtime; AbstractValidatorTestCase now inspects validateForRead / validateForWrite; ReflectionHelper-based property-name string arguments updated.

C++ generator (compiler/cpp/src/thrift/generate/t_php_generator.cc)

Naming alignment with the new runtime:

• Emits $tspec, readStruct(...), writeStruct(...), validateForRead, validateForWrite.
• Service-client emits $input / $output / $seqid (was $input_ / $output_ / $seqid_).
• Enum class emits $names (was $__names).
• REST helper emits $impl (was $impl_).
• Inline-mode UUID locals emit $uuidBin / $uuidHex (was $_uuid_bin / $_uuid_hex).
• Inline-mode strict-binary version check emits TBinaryProtocol::VERSION_1 / VERSION_MASK named constants instead of the 0x80010000 / 0xffff0000 magic numbers (matches what the runtime TBinaryProtocol already does for non-inlined paths).

PSR-12 emission alignment:

• File header reordered to <?php ⇢ blank ⇢ file-level docblock ⇢ blank ⇢ namespace ⇢ blank ⇢ use block (PSR-12 §3).
• Short array syntax [...] for $names, $tspec, type-spec sub-arrays ('key' / 'val' / 'elem'), const values (struct / map / list / set), and local accumulators / default parameter values.
• Class constants emit explicit public visibility.
• public static / protected static ordering instead of static public / static protected.
• Brace-on-next-line for jsonSerialize, validators, REST handlers, REST constructor.
• Inline-mode bodies emit four-space indents (was two-space leftovers from the original PSR-2 port).
• The OOP read/write branches no longer emit a stray blank line before the closing class brace.
• Pre-existing indent bug in the nested 'val' (map) and 'elem' (list / set) sub-specs fixed: indent_down() now precedes the closing-bracket emission so the close lines up with the opening 'val' => / 'elem' =>.
• $json = new stdClass(); instead of new stdClass.
• Single trailing newline at file close (was double in enum and classmap-mode service files).
• Multi-line if (...) { throw ...; } for the strict-binary version check instead of an inline control structure.
• Process-dispatcher emits 'process_' . $fname and ' . $fname . ' with proper PSR-12 concat spacing; unknown-method exception path uses indent() rather than hard-coded four-space prefixes layered on top of indent_up().
• $arr[1] * 4294967296 and $name[] = $value use proper PSR-12 operator spacing.
• Trailing whitespace stripped from empty docblock lines (PHP-local override of generate_php_docstring_comment; the cross-language base t_generator::generate_docstring_comment is untouched).

phpcs:disable directives in the autogen docblock are emitted per file shape, not blanket:

File shape	Disables
Plain struct / enum / interface / REST	(none — clean docblock)
Service args / result helper struct	Squiz.Classes.ValidClassName
Service client / processor	PSR1.Methods.CamelCapsMethodName
Top-level Constant.php	PSR1.Methods.CamelCapsMethodName
Classmap-mode service / Types files	all three (multi-class file containing client + processor + args/result)

The three cross-Thrift sniffs we ever disable cover naming conventions that must remain consistent with the other Thrift language generators (snake_case process_* / send_* / recv_* method names, <service>_<method>_args class names, multi-class classmap files). Rule lists are hoisted to named member vectors so a future rename or addition is a one-line edit.

generate_program_header and generate_service_header collapse into a shared emit_file_header helper; php_autogen_comment delegates to the inherited t_oop_generator::autogen_comment() and splices the optional phpcs:disable block before the closing */, avoiding duplication of the base docblock structure.

C extension (lib/php/src/ext/thrift_protocol/php_thrift_protocol.cpp)

Critical for cross-language tests with PHP server (without this, the accelerated binary protocol crashes with TProtocolException: Attempt deserialize to non-Thrift object):

• All seven zend_read_static_property(..., "_TSPEC", sizeof("_TSPEC")-1, ...) calls now read "tspec" via the idiomatic ZEND_STRL("tspec") Zend macro.
• Pre-existing typo on line 1175 (sizeof("_TPSEC")-1, only working by accident because both literals had the same length) is fixed in the same pass.

Breaking change

This is a deliberate, documented public-API BC break for downstream PHP consumers. Affected surfaces:

1. Class::$_TSPEC reads on every generated struct → must use Class::$tspec. PHP has no __getStatic, so this rename cannot be shimmed; callers must migrate at the same time as the upgrade.
2. Class::$__names reads on enums → must use Class::$names. Same constraint.
3. TJSONProtocol::$contextStack / $context / $reader are now private. Audit found no external callers in the runtime or test suite, so the public surface is removed rather than renamed.
4. Subclasses of TServer / TSocket / TBufferedTransport / TBinaryProtocol / THttpClient / TCurlClient / TServerSocket / TSSLSocket / TFramedTransport / TPhpStream / TSimpleJSONProtocol referencing protected trailing-underscore properties → drop the trailing _ on the new name.
5. Subclasses overriding _read / _write on TBase or TException → use readStruct / writeStruct.

The JIRA ticket needs the Breaking-Change label per the contributing guide (CLI authorization not granted; will be set manually).

Out of scope (deferred to follow-up tickets)

• declare(strict_types=1) — semantic change; warrants its own ticket.
• PHP 8.1 modernization (constructor property promotion, readonly, native enums) — separate refactor.
• Embedded-underscore generated method names (init_FOO, process_<name>, send_<name>, recv_<name>) — deliberate cross-Thrift snake_case generator-name conventions; PSR-12 does not forbid snake_case method names.
• lib/php/src/Thrift.php legacy non-namespaced bundle (not autoloaded since the PSR-4 layout was adopted) — separate cleanup.
• TException::__wakeup() parity with TBase and the dead-code TException::$tmethod local copy — pre-existing inconsistencies on master, deserve their own behavioural-change ticket.
• Pre-existing Undefined array key 1 warning at TSocketPoolTest:210 from a mock data-provider mismatch — unrelated to PSR-12.

Test plan

• vendor/bin/phpcs over lib/php/lib, lib/php/test, and lib/php/test/Resources/packages/ (435 fixture files) — exits 0 with zero output.
• vendor/bin/phpstan analyse -c lib/php/phpstan.neon — no issues.
• vendor/bin/phpunit -c lib/php/phpunit.xml — 629 tests pass, 0 errors, 0 failures (one unrelated pre-existing warning at TSocketPoolTest.php:210).
• make stubs idempotent — diff-free on second run.
• git diff compiler/cpp/src/thrift/generate/ limited to t_php_generator.cc; no other language generators affected (the t_generator.cc base is untouched).
• CI: GitHub Actions lib-php matrix (PHP 8.1 / 8.2 / 8.3 / 8.4 / 8.5) is green.
• CI: lib-php static-code-analysis (phpcs + phpstan) is green.
• CI: cross-language cross-test jobs that exercise PHP-as-server with the accelerated binary protocol (cpp, go, rs, java, py, nodejs, nodets, rb clients) are green — required the C extension tspec lookup fix.

Checklist

• Did you create an Apache Jira ticket? (THRIFT-5959)
• 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? (Will squash before merge — currently 15 commits for reviewability of each step.)
• Did you do your best to avoid breaking changes? Breaking change is intentional and central to this ticket — see "Breaking change" section above. JIRA ticket needs the Breaking-Change label.
• Not applicable — this change involves code, so [skip ci] is not used.

[Jens-G]

Code review

No issues found. Checked for bugs and CLAUDE.md compliance.

🤖 Generated with Claude Code

_{- If this code review was useful, please react with 👍. Otherwise, react with 👎.}