4/5 ✅ Add PHPUnit 13 test suite and CI - #21
Merged
Merged
Conversation
apache/thrift < 0.24.0 is affected by GHSA-8wv5-x4w7-5gww / CVE-2026-43871 — an infinite loop in the PHP, Python, Go and Java bindings, severity high — so the whole previous '>=0.11, <0.17' range is unusable and 0.24.0 is the only release that is not. 0.24.0 itself requires PHP ^8.1; this major goes to ^8.4 so the toolchain can land on current stable releases. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three fixes, all enabled by the new baseline: * socket_create() returns a Socket object since PHP 8.0, so the is_resource($this->socket) guard in connect() was always false and a fresh socket was created on every flush(). Descriptors did not leak — the old socket was collected immediately — but each flush paid for a needless socket_create() plus socket_connect(), and close() was a no-op in practice. * Match the parameter types of the now strictly typed TTransport in 0.24: read(int $len): string and write(string $buf): void. * read() returned '' unconditionally, so the inherited readAll() spun forever in while (strlen($data) < $len). Unreachable for the emit-only agent client, but it is the same infinite-loop shape as CVE-2026-43871. It throws TTransportException now. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The tree was generated by compiler 0.15 while the runtime is now 0.24. Regenerating against jaeger-idl v0.12.0 aligns the two: * declare(strict_types=1) in every file (THRIFT-5986) * native property and constructor types (THRIFT-5991), so 'public $key' becomes 'public ?string $key' — which is what the IDL actually says * native return types on struct methods (THRIFT-5990) * PSR-12 layout (THRIFT-5959) bin/thrift-gen.sh cloned jaeger-idl from a moving 'main', so two runs a month apart could differ with no record of what changed. It now pins jaeger-idl v0.12.0, refuses to run unless the compiler is 0.24.x, generates into a mktemp directory cleaned up by a trap instead of leaving a clone in the working tree, and drops a dead 'rm -rf ../src/Jaeger/Thrift/' from an older layout. Running it twice is now a no-op. BREAKING: 24 classes are gone — AggregationValidator, BaggageRestrictionManager, Dependency and ThrottlingService with their DTOs. They came from IDL definitions Jaeger removed upstream long ago (already absent in jaeger-idl v0.11.4); they are server-side APIs this client never used. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
php-cs-fixer and rector, both configured to skip src/Thrift: that tree is generated by bin/thrift-gen.sh and has to stay byte-for-byte reproducible from the IDL, so reformatting it would be undone by the next regeneration. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Break multi-argument constructors across lines and normalise the spacing between promoted parameter types and their names. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Level 1 — the strictest — over src/, with src/Thrift excluded: its shape is the thrift compiler's contract and it calls thrift_protocol_* from the optional C extension. ClassMustBeFinal is suppressed because this is a library and consumers legitimately extend these classes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Resolve every issue psalm reports at level 1: * type the arrays that cross a public boundary (tags, logs, spans, baggage) so coercions are checked instead of inferred as mixed * handle the false branch of gethostname/getmypid/getmyuid/getmygid/json_encode instead of passing it straight into a string parameter * cast mixed/int and float operands explicitly * give the ArrayAccess and IteratorAggregate implementations their template parameters and real native signatures, dropping #[ReturnTypeWillChange] * align parameter names with the interfaces that declare them, so named arguments work One suppression remains, in Span\Batch\SpanBatch: SpanInterface cannot declare that it extends the generated Thrift\Span even though its only implementation does. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Adds phpunit/phpunit ^13, a phpunit.xml.dist that keeps the generated src/Thrift tree out of coverage, and a Jaeger\Tests\ autoload-dev namespace. composer.json also loses "minimum-stability": "dev" — every dependency now resolves to a stable release — and gains the description, type and keywords that 'composer validate --strict' requires. ext-apcu is declared as a suggestion since only RateLimitingSampler needs it. The three linter configs are widened to cover tests/ as well. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
317 tests, 96.86% line coverage over src/ excluding the generated Thrift code. Table-driven throughout: each case yields a class name plus constructor arguments rather than a ready-made object, because code executed inside a data provider is not attributed to coverage. No mocks or stubs — tests/Fixture holds real implementations: a recording tracer, client and agent, a deterministic id generator and a bound UDP listener. tests/Thrift/SerializationTest.php exercises the generated structs against the apache/thrift runtime over both the binary and compact protocols and emits a real batch over UDP. Since the generated fields are honestly nullable, it narrows them explicitly — which makes the tests stricter, as a round trip must return every 'required' field non-null. Four existing defects are pinned by tests that document them rather than quietly encoding them as intended behaviour: * PeerAddressTag writes the key 'peer.adress' (typo) and PeerIpv4Tag writes 'peer.ip' where OpenTracing specifies 'peer.ipv4' * AdaptiveSampler copies flags and tags from the rate limiter that rejected the trace when the probabilistic sampler is the one that accepted it * StackSpanManager::remove() never removes anything: SplStack::valid() is false until rewind(), and it compares a Span's object hash against a SpanContext's * TextCodec::encode() writes the trace id as '%x%x' without padding, so a traceIdLow shorter than 16 hex digits cannot be split back out on decode Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three jobs: PHPUnit on PHP 8.4 and 8.5, psalm, and a coding-standards job that checks php-cs-fixer and rector in dry-run mode plus 'composer validate --strict'. APCu is enabled on the CLI so the RateLimitingSampler tests run instead of skipping. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Fourth of five stacked PRs for the 4.0 major. Stacked on #20.
What
317 tests, 96.86% line coverage over
src/excluding the generated Thrift code. Plus CI on GitHub Actions: PHPUnit on PHP 8.4 and 8.5, psalm, and a coding-standards job running php-cs-fixer + rector in dry-run mode andcomposer validate --strict.Conventions
Table-driven throughout. Each case yields a class name plus constructor arguments rather than a ready-made object — code executed inside a data provider is not attributed to coverage, and fixing that alone moved coverage from 67.76% to 96.86%.
No mocks or stubs anywhere.
tests/Fixtureholds real implementations: a recording tracer, client and agent, a deterministic id generator, and a bound UDP listener.tests/Thrift/SerializationTest.phpexercises the generated structs against the apache/thrift runtime over both protocols and emits a real batch over UDP. Since #18 made the generated fields honestly nullable, it narrows them explicitly — which makes the tests stricter, as a round trip must return everyrequiredfield non-null.Four defects pinned, not hidden
These tests document existing bugs rather than quietly encoding them as intended behaviour. #22 fixes them.
PeerAddressTagpeer.adress(typo)PeerIpv4Tagpeer.ip, OpenTracing sayspeer.ipv4AdaptiveSamplerStackSpanManager::remove()TextCodec::encode()composer.json
Loses
"minimum-stability": "dev"— every dependency now resolves to a stable release — and gains thedescription,typeandkeywordsthatcomposer validate --strictrequires.ext-apcuis a suggestion, since onlyRateLimitingSamplerneeds it.