sveneld opened a new pull request, #3449: URL: https://github.com/apache/thrift/pull/3449
## Summary Implements [THRIFT-5959](https://issues.apache.org/jira/browse/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 earlier versions of the library — every other Thrift language target (Python `thrift_spec`, Java `metaDataMap`, Ruby `FIELDS`, Go/C# `Read`/`Write`) uses neither leading nor trailing underscores. Runtime behaviour is unchanged. The diff is purely renames + formatting + tooling/docs. ### What changed - **`phpcs.xml.dist`**: ruleset `PSR2` → `PSR12`; both legacy underscore exclusions removed. - **`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** (PHP-only quirks → cross-language alignment): - `$_TSPEC` → `$tspec` (mirrors Python's `thrift_spec`). - `protected _read` / `_write` on `TBase` and `TException` → `readStruct` / `writeStruct` (cannot drop the underscore alone — would clash 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`, `TJSONProtocol`, etc.) → drop the trailing `_` (~509 internal reference updates). - **C++ generator** (`compiler/cpp/src/thrift/generate/t_php_generator.cc`): - 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_`). - PSR-12 ordering: `public static` / `protected static` / `private static` (was `static public` / `static protected`). - Class constants emit explicit `public` visibility (was bare `const`). - Method declarations now place `{` on the next line for `jsonSerialize`, validators, and REST helpers (control-flow `{` stays same-line as PSR-12 requires). - **Hand-written fixtures** under `lib/php/test/Unit/Lib/.../Fixture/` and `TBaseTest.php` updated alongside the runtime. - **Test suite** updated to use the new property and method names; `AbstractValidatorTestCase` now inspects `validateForRead` / `validateForWrite` instead of the old `_`-prefixed names. - **Generated fixtures** under `lib/php/test/Resources/packages/` are regenerated by `make stubs`. They remain untracked per existing project convention and are excluded from `phpcs` via `phpcs.xml.dist`. ### 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`. 2. Public `TJSONProtocol::$contextStack_` / `$context_` / `$reader_` reads → drop the trailing `_`. 3. Subclasses of `TServer`, `TSocket`, `TBufferedTransport`, `TBinaryProtocol`, `THttpClient`, `TCurlClient`, `TServerSocket`, `TSSLSocket`, `TFramedTransport`, `TPhpStream`, etc. that reference protected trailing-underscore properties → drop the trailing `_`. 4. Subclasses overriding `_read` / `_write` on `TBase` or `TException` → use `readStruct` / `writeStruct`. The JIRA ticket should be labelled with **Breaking-Change** per the contributing guide. ### 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>`) — these are deliberate cross-Thrift snake_case generator-name conventions and PSR-12 does not forbid snake_case method names. ## Test plan - [x] `vendor/bin/phpcs` passes cleanly with the new `PSR12` ruleset (50 cosmetic violations were auto-fixed by `vendor/bin/phpcbf` before the final pass). - [x] `vendor/bin/phpstan analyse -c lib/php/phpstan.neon` reports no issues. - [x] `vendor/bin/phpunit -c lib/php/phpunit.xml` — 629 tests pass, 0 errors, 0 failures (one unrelated pre-existing warning at `TSocketPoolTest.php:210` about an undefined array key in test data; not introduced by this change). - [x] `make stubs` is idempotent and the regenerated fixtures contain only the expected renames + PSR-12 cosmetic differences. - [x] `git diff compiler/cpp/src/thrift/generate/` is limited to `t_php_generator.cc`; no other language generators are affected. - [ ] 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 job (`phpcs` + `phpstan`) is green. - [ ] Cross-language `make precross` for PHP confirms the test extension still builds against the regenerated fixtures. ## Checklist - [x] Did you create an [Apache Jira](https://issues.apache.org/jira/projects/THRIFT/issues/) ticket? ([THRIFT-5959](https://issues.apache.org/jira/browse/THRIFT-5959)) - [x] If a ticket exists: Does your pull request title follow the pattern "THRIFT-NNNN: describe my issue"? - [x] Did you squash your changes to a single commit? - [ ] 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 should be labelled `Breaking-Change`.** - [ ] Not applicable — this change involves code, so `[skip ci]` is not used. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
