Complete Broadcasting correctness and SDK lifecycles - #469
Merged
Conversation
Document that package and application TestState callbacks run after the test application has been destroyed and therefore cannot resolve container services.\n\nUse the framework subscriber for first-party optional packages, reserve registrars for process-local state that survives application teardown, and leave external resource cleanup with the corresponding integration-test traits.
Resolve Redis connection prefixes through the Redis-owned configuration boundary and separate native Cluster prefixing from Lua argument formatting. Authorization now removes exactly one leading physical prefix before logical channel normalization and guard selection. Resolve built-in Pusher, Reverb, and Ably broadcasters directly while preserving the complete explicit custom-pool surface. Construct only the selected broadcast wrapper, remove the redundant manager self-binding, and use canonical container resolution without changing Laravel extension points. Add counterfactual coverage for prefix precedence, Cluster and Lua publication, authorization normalization, direct SDK access, custom pool lifecycle behavior, enum dispatch boundaries, wrapper construction, and exact unique lock identities.
Release ordinary Ably channel objects after both successful and failed publication so dynamic channel names cannot accumulate for the worker lifetime. Retain explicitly encrypted channels because their cached object owns the cipher configuration required by later broadcasts. Make presence JSON encoding failure-truthful and preserve the SDK's real one-message publish call behind one narrow annotation suppression for its contradictory duplicate method declarations. Exercise the SDK's supported fake transport seam to prove release, failure cleanup, encrypted-channel retention, repeated-name boundedness, and invalid presence data without reflection or live network calls.
Gate Pusher JSONP decoding behind an explicit per-connection option instead of treating any request callback as permission to return executable JavaScript. Keep ordinary JSON as the default and preserve the existing two-argument broadcaster constructor through an optional parameter. Ship explicit false defaults for Pusher and Reverb while retaining connection-array replacement semantics. Remove built-in SDK pool configuration now that those concurrency-safe clients are manager-cached directly. Cover default JSON, configured JSONP, callback absence, connection defaults, and retained Reverb path configuration.
Normalize custom broadcast names through enum_value at the transport boundary so string-backed, integer-backed, and unit enum results reach the broadcaster as strings. Preserve enum identity at wrapper clone boundaries while ordinary event objects remain isolated by cloning. Widen the ShouldBroadcast channel contract to its documented Laravel surface by accepting one Channel or string as well as arrays; the existing runtime normalization already supports each form. Add coverage for every enum name and clone shape, single-string channels, ordinary clone isolation, and existing queue option behavior.
Narrow channel-handler normalization to the callable and class-string inputs the implementation supports, with a matching callable return contract. Remove contradictory docblock types rather than widening the runtime boundary. Make Log broadcaster payload encoding throw on invalid user data instead of silently publishing an invalid representation. Preserve valid JSON formatting and all established authorization behavior. Add focused invalid-payload coverage and complete native test method typing across the touched broadcaster tests.
Replace the assignment inside the toOthers dispatch assertion with a strict comparison so the test fails when the socket identifier is not propagated. This corrects a vacuous assertion without changing production behavior. Complete native void return types in the touched integration test while preserving the existing anonymous-event coverage.
Declare the Routing and PSR logging dependencies used by the split package and remove stale direct Auth and Cache requirements whose behavior is owned through existing package boundaries. Keep Redis optional because it is resolved lazily only when that driver is selected. Add a focused split-metadata regression covering required packages, optional SDK and Redis suggestions, provider discovery, and the absence of false dependencies so subtree releases remain self-contained.
Document enum broadcast names, single-string channels, and explicit JSONP opt-in at the public Broadcasting surface. Clarify that built-in SDK drivers resolve directly while pooling remains available only for drivers applications explicitly mark poolable. Add concise package provenance and intentional Hypervel differences without duplicating the user guide. Remove the completed built-in pooling todo and stale documentation that implied automatic broadcaster pooling.
Record the verified Broadcasting findings, upstream and SDK evidence, ownership decisions, exact implementation boundaries, counterfactual test matrix, and final validation requirements. Carry forward the audit's general anti-overengineering rules so post-compaction work retains the same standards without depending on the core plan. The design rejects speculative registries and compatibility machinery while requiring complete fixes at the owning boundary.
Mark Broadcasting complete in the package checklist and reset the active routing state. Close the Events, Support, Redis, Contracts, and Queue dependency revalidations at both their index and owning-ledger locations. Add the final Broadcasting ledger entry with its inspected surface, accepted and rejected boundaries, implementation result, counterfactual versus guard coverage, performance profile, Laravel-facing compatibility, validation, and review status. Record why the corrected Redis, built-in-pool, and anonymous-socket assertions are truthful test repairs rather than weakened expectations.
Drop the README note about worker-wide channel registration. Applications and packages register channel authorization during boot in both Laravel and Hypervel, so the underlying storage lifetime is not a practical public difference users need to account for. Keep the method-level lifecycle warning for framework maintainers and retain only actionable public differences in the package README.
|
Warning Review limit reached
Next review available in: 25 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughBroadcasting now supports enum names and string channels, corrects Redis prefix handling, disables JSONP by default, uses direct built-in SDK clients, releases ordinary Ably channels, throws JSON errors, updates dependencies and documentation, and adds focused regression coverage. ChangesBroadcasting correctness
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant BroadcastManager
participant Broadcaster
participant SDK_or_Redis
BroadcastManager->>Broadcaster: Resolve configured driver
BroadcastManager->>Broadcaster: Dispatch event or authenticate channel
Broadcaster->>SDK_or_Redis: Publish or encode transport payload
SDK_or_Redis-->>Broadcaster: Return response or error
Broadcaster-->>BroadcastManager: Return result or throw exception
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
Greptile SummaryThe PR completes the Broadcasting package audit, aligning event and channel inputs with current Laravel behavior while revising Redis prefix handling and built-in SDK lifecycles.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| src/broadcasting/src/BroadcastManager.php | Revises driver construction, queuing wrapper selection, SDK ownership, Redis prefix resolution, JSONP configuration, and explicit custom-driver pooling. |
| src/broadcasting/src/BroadcastEvent.php | Normalizes enum event names, wraps single-string channels, and preserves enum identity while cloning ordinary events. |
| src/broadcasting/src/Broadcasters/RedisBroadcaster.php | Separates native cluster prefixing from Lua channel prefixing and consistently removes only one leading prefix during authorization. |
| src/broadcasting/src/Broadcasters/AblyBroadcaster.php | Throws on authentication JSON failures and releases ordinary channel objects after both successful and failed publication while retaining cipher-configured channels. |
| src/broadcasting/src/Broadcasters/PusherBroadcaster.php | Makes executable JSONP authentication responses conditional on explicit connection configuration. |
| src/broadcasting/src/BroadcastServiceProvider.php | Removes a redundant explicit manager singleton because the container auto-caches the unbound concrete manager. |
| src/contracts/src/Broadcasting/ShouldBroadcast.php | Expands the public channel return contract to accept a single string. |
| src/foundation/config/broadcasting.php | Disables JSONP by default and removes superseded built-in broadcaster pool defaults. |
| src/broadcasting/composer.json | Replaces stale direct dependencies with the routing and PSR logging dependencies used by the package. |
Reviews (2): Last reviewed commit: "test(broadcasting): assert the JSONP cal..." | Re-trigger Greptile
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/Broadcasting/PusherBroadcasterTest.php`:
- Around line 303-306: Update the test around validAuthenticationResponse to
verify the JSONP wrapper content, asserting that the returned response body
starts with the myCallback callback rather than only checking its JsonResponse
type. Keep the existing response-type assertion if useful, but ensure the test
fails when the callback is ignored.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6b70525b-5250-415f-8301-85c96c9163ce
📒 Files selected for processing (29)
AGENTS.mddocs/plans/2026-07-12-0900-framework-coroutine-state-lifecycle-audit.mddocs/plans/2026-07-12-0915-framework-coroutine-state-lifecycle-audit-ledger.mddocs/plans/2026-08-03-1218-broadcasting-correctness-direct-sdk-lifecycles-and-current-parity.mddocs/todo.mdsrc/boost/docs/broadcasting.mdsrc/boost/docs/pools.mdsrc/broadcasting/README.mdsrc/broadcasting/composer.jsonsrc/broadcasting/src/BroadcastEvent.phpsrc/broadcasting/src/BroadcastManager.phpsrc/broadcasting/src/BroadcastServiceProvider.phpsrc/broadcasting/src/Broadcasters/AblyBroadcaster.phpsrc/broadcasting/src/Broadcasters/Broadcaster.phpsrc/broadcasting/src/Broadcasters/LogBroadcaster.phpsrc/broadcasting/src/Broadcasters/PusherBroadcaster.phpsrc/broadcasting/src/Broadcasters/RedisBroadcaster.phpsrc/contracts/src/Broadcasting/ShouldBroadcast.phpsrc/foundation/config/broadcasting.phptests/Broadcasting/AblyBroadcasterTest.phptests/Broadcasting/BroadcastEventTest.phptests/Broadcasting/BroadcasterTest.phptests/Broadcasting/LogBroadcasterTest.phptests/Broadcasting/PackageMetadataTest.phptests/Broadcasting/PusherBroadcasterTest.phptests/Broadcasting/RedisBroadcasterTest.phptests/Foundation/FoundationConfigTest.phptests/Integration/Broadcasting/BroadcastManagerTest.phptests/Integration/Broadcasting/SendingBroadcastsViaAnonymousEventTest.php
💤 Files with no reviewable changes (2)
- docs/todo.md
- src/broadcasting/src/BroadcastServiceProvider.php
Capture the explicitly enabled Pusher authorization response and assert that its content begins with Symfony's JSONP callback wrapper. The previous class-only assertion also passed for an ordinary JSON response, so it could not detect removal of withCallback(). The new assertion proves the configured callback is applied without coupling the regression to the complete serialized payload. The focused Pusher suite and the complete composer fix gate pass.
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
1 participant
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.
Summary
This completes the Broadcasting package audit and brings its supported surface in line with current Laravel while retaining Hypervel's coroutine and custom-pool capabilities.
The change fixes Redis prefix ownership across standalone, Cluster, Lua, and authorization paths; resolves built-in SDK broadcasters directly; bounds Ably's worker-lifetime channel cache; makes Pusher JSONP an explicit opt-in; supports enum event names and single-string channels; and makes arbitrary-data JSON failures visible at transport boundaries.
It also removes superseded pool defaults, dead fallback code, false package dependencies, and stale documentation. The audit records and all carried cross-package revalidations are updated as complete.
For more details, see
docs/plans/2026-08-03-1218-broadcasting-correctness-direct-sdk-lifecycles-and-current-parity.md.Redis correctness
ARGVchannel names, which phpredis does not transform.SDK ownership and lifecycle
Public behavior
ShouldBroadcast::broadcastOn()in addition to channel objects and arrays.jsonpconnection option before returning executable Pusher JSONP responses.Failure handling and cleanup
toOthers()assertion so it verifies socket propagation instead of assigning it.Metadata and documentation
Performance and compatibility
Built-in drivers no longer pay pool checkout or proxy overhead and remain manager-cached. Wrapper selection removes a discarded object, clone, and repeated metadata reads. Redis configuration is resolved once when constructing the driver. Authorization adds one leading-prefix comparison and slice. Ably adds one in-memory release after the existing network publication while preventing unbounded retained channel objects.
The change adds no request-time lock, registry, retry loop, coroutine-context lookup, serialization layer, or additional network round trip. No supported Laravel public API or protected extension point is removed.
Validation
Summary by CodeRabbit
New Features
Bug Fixes
Documentation