THRIFT-5957: Add phpstan static analysis with CI guardrail#3436
Merged
Conversation
mergeable
Bot
added
php
build and general CI
kpumuk
reviewed
May 2, 2026
sveneld
force-pushed
the
THRIFT-5957
branch
2 times, most recently
from
f03aedc to
05e45cb
Compare
5 tasks
sveneld
force-pushed
the
THRIFT-5957
branch
2 times, most recently
from
d02b563 to
d106d0d
Compare
Client: php
The PHP runtime library at lib/php/lib/ had no static-analysis tooling
in CI. Refactors and type-modernization PRs went in without an
automated safety net.
This change:
* Adds phpstan/phpstan ^1.12 as a require-dev dependency, plus a
"scripts.phpstan" composer alias for local use.
* Adds lib/php/phpstan.neon (level 1) targeting lib/php/lib/ with
test/bootstrap.php for autoload, and a generated baseline that
pins the 5 remaining issues (all "should return X but return
statement is missing" in TJSONProtocol/TSimpleJSONProtocol's
writeStructBegin/writeStructEnd and ThriftClassLoader::findFile —
left for a follow-up ticket).
* Adds a "Run phpstan" step alongside the existing per-language
steps (cppcheck, flake8, phpcs, rubocop) in the sca.yml workflow,
and wires its outcome into the aggregate "Fail if any SCA check
failed" gate. Runs with --error-format=github so findings show up
as inline annotations on PRs.
* Bumps the sca job's setup-php from 7.1 to 8.1 to match the new
project floor (THRIFT-5956) and to satisfy phpstan 1.x's PHP
>= 7.2 requirement.
Also fixes the trivial subset of phpstan findings already discovered
during baseline generation:
* TSocketPool::open: replace extract($this->servers_[$i]) with
explicit $host / $port assignments. Removes 10 "might not be
defined" findings and avoids extract()'s well-known footgun of
silently rebinding existing locals.
* TBufferedTransport::readAll: change the final "elseif" to "else"
so phpstan can prove $data is always assigned.
Level 1 catches undefined variables and obvious type mismatches
without requiring widespread code changes. Higher levels will be
raised in follow-up tickets as the runtime library gains native
types and PHPDoc cleanup.
Out of scope (separate tickets):
* PSR-12 migration / php-cs-fixer.
* declare(strict_types=1) and native types in lib.
* Static analysis of generated PHP under test/Resources/packages/.
* Fixing the remaining 5 missing-return findings.
* CHANGES.md is auto-generated on release; no manual entry here.
Generated-by: Claude Opus 4.7 (1M context)
kpumuk
approved these changes
May 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The PHP runtime library at
lib/php/lib/had no static-analysis tooling in CI. Refactors and upcoming type-modernization PRs would otherwise go in without an automated safety net.Changes:
phpstan/phpstan ^1.12as arequire-devdependency, plus ascripts.phpstancomposer alias for local dev convenience.lib/php/phpstan.neon(level 1) targetinglib/php/lib/withtest/bootstrap.phpfor autoload, and a generatedlib/php/phpstan-baseline.neonthat pins the 5 remaining issues (all "should return X but return statement is missing" inTJSONProtocol/TSimpleJSONProtocolwriteStructBegin/writeStructEndandThriftClassLoader::findFile— left for a follow-up ticket because they need a small protocol-spec decision).lib-php-qualityjob in.github/workflows/build.ymlthat runs phpstan on PHP 8.3 with--error-format=githubso any new finding shows up as an inline annotation on the PR diff.Trivial findings fixed in this PR (rather than baselined — the change is purely cleanup, no behaviour shift):
TSocketPool::open: replacedextract($this->servers_[$i])with explicit$host/$portassignments. Removes 10 "might not be defined" findings and avoidsextract()'s well-known footgun of silently rebinding existing locals from a foreign array.TBufferedTransport::readAll: changed the finalelseif ($have > $len)toelseso phpstan can prove$datais always assigned. Semantically identical; the previous form was the only remaining case after the prior three.Level 1 catches undefined variables and obvious type mismatches without requiring widespread code changes. Higher levels will be raised in follow-up tickets as the runtime library gains native types and PHPDoc cleanup.
Out of scope (separate tickets, in roadmap order):
writeStructBegin/Endreturns 0 or the bytes written by the inner JSON helper).declare(strict_types=1)and native parameter / return / property types inlib/php/lib/.test/Resources/packages/.t_php_generator.ccmodernization.[skip ci]anywhere in the commit message to free up build resources.Generated-by: Claude Opus 4.7 (1M context)