Add support of google/protobuf v5.x - #15
Merged
Merged
Conversation
`MapField` iteration order is an implementation detail of the protobuf runtime, and it changed: google/protobuf 4.33 already yields the two entries of the tags map back to front, so the strict comparison against ['baz', 'baf'] fails because `===` on arrays also compares key order. The test broke without anything in this package changing. Sort the entries by key before comparing, so the assertion checks what it means — the tag values that were set — instead of the runtime's hash order.
Widen the constraint the same way #13 did for v4.x. Nothing in the source needs to change: the DTOs come from roadrunner-php/roadrunner-api-dto, which already requires google/protobuf ^4.31.1 || ^5.34.0, so this package's upper bound is the only thing keeping protobuf 5 out of an installation. The practical effect is on downstream packages: spiral/roadrunner-bridge ^4.0 depends on this package, so an application that wants the bridge together with protobuf 5 is forced back to protobuf 4.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe Protobuf dependency constraint now includes v5.x, PHPUnit uses a compatibility bootstrap for repeated fields, and the publish RPC test normalizes tag-map ordering before comparison. ChangesProtobuf compatibility
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
`google/protobuf` moved `RepeatedField` out of the `Internal` namespace: 4.x left a deprecated shim behind, 5.x removed the file entirely. The `class_alias()` for the old FQN sits at the bottom of the new class file, so it is only registered once the new class has been loaded -- autoloading `Google\Protobuf\Internal\RepeatedField` on its own fails on 5.x. Tests now reference the public `Google\Protobuf\RepeatedField` and a test bootstrap back-fills that FQN on protobuf 3.x, which still ships only the `Internal` one. This also silences the deprecation notice emitted by the 4.x shim. `MapField` is left untouched -- it has not been moved and still lives in `Google\Protobuf\Internal` in every version, including upstream `main`. Verified with 121 tests passing on PHP 8.1/protobuf 3.22.0 (--prefer-lowest), PHP 8.2/protobuf 4.33.6, PHP 8.2/8.5/protobuf 5.35.1, and PHP 8.4 with the protobuf C extension loaded. Assisted-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
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.
Allows
google/protobuf5.x, the same way #13 allowed 4.x — the constraint becomes^3.7 || ^4.0 || ^5.0. No source change is needed: the DTOs come fromroadrunner-php/roadrunner-api-dto, which already requiresgoogle/protobuf: ^4.31.1 || ^5.34.0, so this package's upper bound is the only thing that keepsprotobuf 5 out of an installation.
Why it matters downstream:
spiral/roadrunner-bridge^4.0 depends on this package, so anapplication that wants the bridge together with
google/protobuf5.x cannot have both — Composereither fails to resolve or downgrades protobuf to 4.x.
The test commit
RPCCentrifugoApiTest::testPublishcompares the publish tags with===against['baz', 'baf'],which also compares key order. The iteration order of a protobuf
MapFieldis an implementationdetail of the runtime and it changed: on
google/protobuf4.33 a two-entry map already iterates backto front (
[1 => 'baf', 0 => 'baz']), so the test fails there — before this constraint change,on 4.x. The first commit sorts the entries by key so the assertion checks the tag values it means to
check. Without it CI on this branch would be red for a reason unrelated to protobuf 5.
Verification
php 8.4.23, full suite green in both matrix legs CI uses:prefer-stablegoogle/protobuf v5.35.1,api-dto v1.14.1prefer-lowestgoogle/protobuf v4.33.6,api-dto v1.8.0Note on the
static analysisjob: it is already failing on2.xindependently of this PR. Psalm 6(the
require-devconstraint is>= 5.8) reports 48MissingOverrideAttribute/ClassMustBeFinalissues; the count is identical with and without this branch, so it is left alone here.
🤖 Generated with Claude Code
Summary by CodeRabbit
Compatibility
Bug Fixes
Tests