[4.x] BroadcastingConfigBootstrapper: fix central instances persisting in tenant context#1448
Open
[4.x] BroadcastingConfigBootstrapper: fix central instances persisting in tenant context#1448
Conversation
Test that BroadcastingConfigBootstrapper correctly maps tenant properties to broadcasting config/credentials, and that the credentials don't leak when switching contexts. Also add the `$config` property to `TestingBroadcaster` so that we can access the credentials used by the broadcaster.
…ators Test that TenancyBroadcastManager inherits the custom driver creators from the central BroadcastManager.
…anager's custom creators
…tenant's broadcaster on `bootstrap()`
…ed `Broadcasting\Factory` instance After initializing tenancy, calls like `Broadcast::auth()` use the central `BroadcastManager`. Clearing the facade's resolved `Broadcasting\Factory` instance fixes that problem.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1448 +/- ##
============================================
+ Coverage 86.03% 86.10% +0.06%
- Complexity 1156 1157 +1
============================================
Files 184 184
Lines 3381 3389 +8
============================================
+ Hits 2909 2918 +9
+ Misses 472 471 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Moved binding `Broadcaster` to the bootstrapper.
Test that the bound Broadcaster instance inherits the channels too. Also test that the channels aren't lost when switching context to another tenant.
Tests from BroadcastingTest moved to the appropriate bootstrapper test files. The new tenant credentials test has assertions equal to both the original property -> config mapping test and the config -> credentials test.
Tests now use datasets with all drivers that are in `TenancyBroadcastManager::$tenantBroadcasters` by default plus the custom driver. Also add assertions for updating the tenant properties/config in tenant context.
… order
Previously, credential mappings from `$mapPresets` overrode mappings defined in `$credentialsMap`. If someone used pusher/reverb/ably and wanted to override some of that preset's mappings, e.g. use 'pusher_app_key' instead of 'pusher_key' by specifying 'pusher_app_key' in `$credentialsMap`, the preset's mapping ('pusher_key') would still be used.
Adding 'reverb' to `TenancyBroadcastManager::$tenantBroadcasters` will make these tests pass.
Contributor
📝 WalkthroughWalkthroughBroadcast credential preset precedence flipped; central BroadcastManager custom creators are copied into the tenancy manager; broadcaster resolution is routed through the tenant manager with channel transfer when applicable; Broadcast facade’s resolved factory is cleared on bootstrap and revert; Changes
Sequence DiagramsequenceDiagram
participant Central as Central BroadcastManager
participant Bootstrap as BroadcastingConfigBootstrapper
participant TenantMgr as TenancyBroadcastManager
participant Broadcaster as Broadcaster
participant Facade as Broadcast Facade
Bootstrap->>Central: read config and customCreators
Bootstrap->>TenantMgr: instantiate TenancyBroadcastManager
TenantMgr->>TenantMgr: extend(...) for each central customCreator
note right of TenantMgr: mapPresets merged first (presets take precedence)
Bootstrap->>TenantMgr: tenantMgr.connection() -> create Broadcaster
TenantMgr->>Broadcaster: construct tenant-scoped Broadcaster
Broadcaster->>Broadcaster: pass channels from original broadcaster (if both are Broadcaster instances)
Bootstrap->>Facade: clear resolved Factory binding
Facade->>TenantMgr: subsequent resolves use tenant manager.connection()
Bootstrap->>Bootstrap: revert()
Bootstrap->>Facade: clear resolved Factory binding on revert
Facade->>Central: subsequent resolves use central manager
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Bootstrappers/BroadcastingConfigBootstrapper.php`:
- Around line 96-98: The closure passed to $this->app->extend currently declares
an unused Broadcaster $broadcaster parameter which triggers static analysis;
change the callback signature to remove that parameter so it becomes a
zero-argument closure (e.g., function () { ... } or an arrow fn) and return
$this->app->make(BroadcastManager::class)->connection(); leaving the extend call
and references to Broadcaster::class and BroadcastManager::class unchanged.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 36a92dc5-b8bc-4656-be92-2b81ddd77b4c
📒 Files selected for processing (6)
src/Bootstrappers/BroadcastingConfigBootstrapper.phpsrc/Overrides/TenancyBroadcastManager.phptests/Bootstrappers/BroadcastChannelPrefixBootstrapperTest.phptests/Bootstrappers/BroadcastingConfigBootstrapperTest.phptests/BroadcastingTest.phptests/Etc/TestingBroadcaster.php
💤 Files with no reviewable changes (1)
- tests/BroadcastingTest.php
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes tenant broadcasting isolation issues where central broadcasting instances/credentials could leak into tenant context, especially for /broadcasting/auth and custom broadcast drivers.
Changes:
- Clear the resolved
Broadcastfacade factory so it re-resolves the (tenant) broadcast manager after tenancy bootstraps. - Ensure
TenancyBroadcastManagerinherits custom driver creators from the centralBroadcastManager. - Rework/expand tests (including moving prior broadcasting tests into more appropriate bootstrapper test files) and add Reverb coverage.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/Bootstrappers/BroadcastingConfigBootstrapper.php |
Clears Broadcast facade resolved factory, copies custom creators to tenant manager, and ensures the bound broadcaster resolves via the current manager. |
src/Overrides/TenancyBroadcastManager.php |
Re-resolves tenant broadcasters and passes channels from the “original” broadcaster to the newly resolved one. |
tests/Etc/TestingBroadcaster.php |
Extends test broadcaster to retain resolved config for assertions. |
tests/BroadcastingTest.php |
Removed; tests relocated into more targeted bootstrapper test files. |
tests/Bootstrappers/BroadcastingConfigBootstrapperTest.php |
Adds/updates tests for credential mapping, facade behavior, and custom driver creator inheritance. |
tests/Bootstrappers/BroadcastChannelPrefixBootstrapperTest.php |
Adds tests for tenant_channel / global_channel helpers and channel registration behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/Bootstrappers/BroadcastingConfigBootstrapperTest.php`:
- Around line 21-26: The test currently overwrites
TenancyBroadcastManager::$tenantBroadcasters with a hard-coded array; instead
capture and restore the original value so tests don’t drift if production
defaults change: in the beforeEach store the original list into a local static
or test-scoped variable (e.g., $originalTenantBroadcasters =
TenancyBroadcastManager::$tenantBroadcasters) before modifying it for the test,
and in afterEach set TenancyBroadcastManager::$tenantBroadcasters back to that
saved $originalTenantBroadcasters; apply the same save-and-restore pattern for
the other test blocks that currently hard-code
TenancyBroadcastManager::$tenantBroadcasters.
- Line 73: Update the inline comment in the test to reference the dynamic
connection key used by the assertion: change the mistaken
"broadcasting.connections.testing.key" wording to
"broadcasting.connections.{$driver}.key" (or equivalent
"broadcasting.connections.{driver}.key") so it matches the assertion in
BroadcastingConfigBootstrapperTest and clarifies that the tenant's testing_key
maps to the driver-specific broadcasting.connections entry.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a47c7533-7284-47f9-b6ee-82f33837df1a
📒 Files selected for processing (1)
tests/Bootstrappers/BroadcastingConfigBootstrapperTest.php
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/Bootstrappers/BroadcastingConfigBootstrapperTest.php (1)
29-40:⚠️ Potential issue | 🟠 MajorTighten the revert assertion so it can fail.
At Line 40,
TenancyBroadcastManagerstill satisfiesinstanceof BroadcastManager, so this test passes even if the tenant manager never gets reverted. Check that the resolved manager is notTenancyBroadcastManagerbefore initialization and aftertenancy()->end().♻️ Suggested fix
- expect(app(BroadcastManager::class))->toBeInstanceOf(BroadcastManager::class); + expect(app(BroadcastManager::class)) + ->toBeInstanceOf(BroadcastManager::class) + ->not->toBeInstanceOf(TenancyBroadcastManager::class); @@ - expect(app(BroadcastManager::class))->toBeInstanceOf(BroadcastManager::class); + expect(app(BroadcastManager::class)) + ->toBeInstanceOf(BroadcastManager::class) + ->not->toBeInstanceOf(TenancyBroadcastManager::class);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Bootstrappers/BroadcastingConfigBootstrapperTest.php` around lines 29 - 40, The test currently only asserts the resolved BroadcastManager is an instance of BroadcastManager, which allows TenancyBroadcastManager to satisfy the assertion; change the assertions to explicitly assert that the resolved manager is not an instance of TenancyBroadcastManager before tenancy initialization and again after tenancy()->end(). Locate the test function (test 'BroadcastingConfigBootstrapper binds TenancyBroadcastManager...') and replace the initial and final expect(...) calls that check app(BroadcastManager::class) with explicit negative assertions against TenancyBroadcastManager while keeping the positive assertion during tenancy initialization.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/Bootstrappers/BroadcastingConfigBootstrapperTest.php`:
- Around line 103-110: After updating the broadcasting config for the tenant you
should assert that any previously resolved BroadcasterContract still holds the
stale credentials; add an assertion verifying app(BroadcasterContract::class)
(or the container-resolved Broadcaster instance obtained earlier) still has the
original 'key' value (not 'new_tenant1_key') after calling
config(["broadcasting.connections.{$driver}.key" => 'new_tenant1_key']), in
addition to the existing checks against BroadcastManager::class and
Broadcast::driver().
- Around line 20-26: The tests are failing to reset the cached broadcaster
stored by BroadcastingConfigBootstrapper::__construct() which memoizes
static::$broadcaster (using ??=), so add a reset of that static between dataset
runs: in the before/after hooks that currently clear
BroadcastingConfigBootstrapper::$credentialsMap and
TenancyBroadcastManager::$tenantBroadcasters, also set
BroadcastingConfigBootstrapper::$broadcaster = null (or unset the static cache)
so each dataset will reinitialize its own broadcaster mapping rather than
reusing the memoized instance.
---
Outside diff comments:
In `@tests/Bootstrappers/BroadcastingConfigBootstrapperTest.php`:
- Around line 29-40: The test currently only asserts the resolved
BroadcastManager is an instance of BroadcastManager, which allows
TenancyBroadcastManager to satisfy the assertion; change the assertions to
explicitly assert that the resolved manager is not an instance of
TenancyBroadcastManager before tenancy initialization and again after
tenancy()->end(). Locate the test function (test 'BroadcastingConfigBootstrapper
binds TenancyBroadcastManager...') and replace the initial and final expect(...)
calls that check app(BroadcastManager::class) with explicit negative assertions
against TenancyBroadcastManager while keeping the positive assertion during
tenancy initialization.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 90bccdb7-82ed-4c73-81bf-5c180ba16a09
📒 Files selected for processing (1)
tests/Bootstrappers/BroadcastingConfigBootstrapperTest.php
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/Bootstrappers/BroadcastingConfigBootstrapperTest.php`:
- Around line 25-29: The afterEach teardown does not reset
BroadcastingConfigBootstrapper::$mapPresets which can leak state between tests;
update the afterEach hook to clear or restore
BroadcastingConfigBootstrapper::$mapPresets (alongside
BroadcastingConfigBootstrapper::$broadcaster and::$credentialsMap and
TenancyBroadcastManager::$tenantBroadcasters) so tests are isolated—find the
afterEach closure and add a line that resets
BroadcastingConfigBootstrapper::$mapPresets to its default/empty value.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9ac5e4b9-8344-4d19-b83c-28fdccb95687
📒 Files selected for processing (1)
tests/Bootstrappers/BroadcastingConfigBootstrapperTest.php
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/Bootstrappers/BroadcastingConfigBootstrapperTest.php`:
- Around line 180-204: The test currently asserts exact array order of
app(BroadcastManager::class)->customCreators which is fragile; change the
assertions to be order-insensitive by comparing sets/ sorted keys instead: when
capturing $originalDrivers and subsequent checks after
tenancy()->initialize($tenant), tenancy()->initialize($tenant2), and
tenancy()->end(), sort
array_keys(invade(app(BroadcastManager::class))->customCreators) (or cast to a
set) before comparing to sorted $originalDrivers plus 'testing-tenant1' (or
sorted merge) so the test verifies presence/reversion of creators without
relying on insertion order for BroadcastManager::customCreators and the
TestingBroadcaster registration.
- Around line 22-64: Extract the repeated preset array into a single shared
fixture variable and use that variable in both the setup and teardown hooks
instead of repeating the literal: define a local $presetMap (or similarly named
variable) holding the array, then assign
BroadcastingConfigBootstrapper::$mapPresets = $presetMap and later reset it to
$presetMap in afterEach; keep other resets
(BroadcastingConfigBootstrapper::$broadcaster, ::$credentialsMap and
TenancyBroadcastManager::$tenantBroadcasters) unchanged so setup/teardown cannot
drift.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7f7f6e5d-bb94-45a2-bdef-6b9e568a9933
📒 Files selected for processing (1)
tests/Bootstrappers/BroadcastingConfigBootstrapperTest.php
The order of the array items shouldn't matter.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/Bootstrappers/BroadcastingConfigBootstrapperTest.php (1)
16-47: 🛠️ Refactor suggestion | 🟠 MajorRestore the original static fixtures instead of duplicating defaults in
$cleanup.Hard-coding
BroadcastingConfigBootstrapper::$mapPresetsandTenancyBroadcastManager::$tenantBroadcastershere makes the suite drift as production defaults evolve, and these hooks no longer restore the actual pre-test baseline. Capture the current static values once and restore those originals inbeforeEach/afterEach.♻️ Suggested refactor
+$originalBroadcaster = BroadcastingConfigBootstrapper::$broadcaster; +$originalCredentialsMap = BroadcastingConfigBootstrapper::$credentialsMap; +$originalMapPresets = BroadcastingConfigBootstrapper::$mapPresets; +$originalTenantBroadcasters = TenancyBroadcastManager::$tenantBroadcasters; + -$cleanup = function () { - BroadcastingConfigBootstrapper::$broadcaster = null; - BroadcastingConfigBootstrapper::$credentialsMap = []; - BroadcastingConfigBootstrapper::$mapPresets = [ - 'pusher' => [ - 'broadcasting.connections.pusher.key' => 'pusher_key', - 'broadcasting.connections.pusher.secret' => 'pusher_secret', - 'broadcasting.connections.pusher.app_id' => 'pusher_app_id', - 'broadcasting.connections.pusher.options.cluster' => 'pusher_cluster', - ], - 'reverb' => [ - 'broadcasting.connections.reverb.key' => 'reverb_key', - 'broadcasting.connections.reverb.secret' => 'reverb_secret', - 'broadcasting.connections.reverb.app_id' => 'reverb_app_id', - 'broadcasting.connections.reverb.options.cluster' => 'reverb_cluster', - ], - 'ably' => [ - 'broadcasting.connections.ably.key' => 'ably_key', - 'broadcasting.connections.ably.public' => 'ably_public', - ], - ]; - TenancyBroadcastManager::$tenantBroadcasters = ['pusher', 'ably', 'reverb']; +$cleanup = function () use ( + &$originalBroadcaster, + &$originalCredentialsMap, + &$originalMapPresets, + &$originalTenantBroadcasters, +) { + BroadcastingConfigBootstrapper::$broadcaster = $originalBroadcaster; + BroadcastingConfigBootstrapper::$credentialsMap = $originalCredentialsMap; + BroadcastingConfigBootstrapper::$mapPresets = $originalMapPresets; + TenancyBroadcastManager::$tenantBroadcasters = $originalTenantBroadcasters; };Based on learnings: In the ArchtechX tenancy test suite, do not reset global/static state in
beforeEach/afterEachby hard-coding class default values; capture the current/original static values and restore those exact originals in the hooks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Bootstrappers/BroadcastingConfigBootstrapperTest.php` around lines 16 - 47, The test teardown currently hard-codes defaults; instead capture the original static values once and restore them in the hooks: at the top of the test file save the current BroadcastingConfigBootstrapper::$broadcaster, BroadcastingConfigBootstrapper::$credentialsMap, BroadcastingConfigBootstrapper::$mapPresets and TenancyBroadcastManager::$tenantBroadcasters into local variables, then update the beforeEach/afterEach cleanup to restore those saved originals (rather than writing literal arrays), keeping the existing Event::listen calls and using the same $cleanup closure to reassign the saved originals back to the static properties.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/Bootstrappers/BroadcastingConfigBootstrapperTest.php`:
- Around line 16-47: The test teardown currently hard-codes defaults; instead
capture the original static values once and restore them in the hooks: at the
top of the test file save the current
BroadcastingConfigBootstrapper::$broadcaster,
BroadcastingConfigBootstrapper::$credentialsMap,
BroadcastingConfigBootstrapper::$mapPresets and
TenancyBroadcastManager::$tenantBroadcasters into local variables, then update
the beforeEach/afterEach cleanup to restore those saved originals (rather than
writing literal arrays), keeping the existing Event::listen calls and using the
same $cleanup closure to reassign the saved originals back to the static
properties.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ad142858-113d-438a-82ca-82546e00b75f
📒 Files selected for processing (1)
tests/Bootstrappers/BroadcastingConfigBootstrapperTest.php
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/Bootstrappers/BroadcastingConfigBootstrapper.php (1)
62-68:⚠️ Potential issue | 🟠 MajorConstructor leaks preset mappings into later bootstrapper instances.
Line 67 rewrites the global
static::$credentialsMap. That fixes precedence for the current instance, but it also makes the class state sticky across later instantiations: if one instance usespusherand a later one switches toreverbor a custom broadcaster, the old preset keys stay in the static array andsetConfig()will keep overriding the previous connection too.Store the merged map on the instance instead of mutating the public static override.
♻️ Proposed fix
class BroadcastingConfigBootstrapper implements TenancyBootstrapper { + protected array $resolvedCredentialsMap = []; + public function __construct( protected Repository $config, protected Application $app ) { static::$broadcaster ??= $config->get('broadcasting.default'); - static::$credentialsMap = array_merge(static::$mapPresets[static::$broadcaster] ?? [], static::$credentialsMap); + $this->resolvedCredentialsMap = array_merge( + static::$mapPresets[static::$broadcaster] ?? [], + static::$credentialsMap, + ); } @@ protected function setConfig(Tenant $tenant): void { - foreach (static::$credentialsMap as $configKey => $storageKey) { + foreach ($this->resolvedCredentialsMap as $configKey => $storageKey) { $override = $tenant->$storageKey;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Bootstrappers/BroadcastingConfigBootstrapper.php` around lines 62 - 68, The constructor is mutating the class-wide static::$credentialsMap causing preset keys to leak across instances; instead compute the merged map from static::$mapPresets and static::$credentialsMap and assign it to an instance property (e.g. $this->credentialsMap) inside __construct, leaving static::$credentialsMap unchanged; then update setConfig (and any other methods referencing static::$credentialsMap) to read from $this->credentialsMap so each bootstrapper instance keeps its own merged credentials map.src/Overrides/TenancyBroadcastManager.php (1)
36-56:⚠️ Potential issue | 🟠 MajorCustom drivers still bypass channel inheritance.
Line 45 gates the entire branch on a hard-coded driver-name list. After
BroadcastingConfigBootstrappercopies centralcustomCreatorsinto this manager, a tenant default custom driver can now resolve here, but it still falls through toparent::get()with no channel transfer. If that broadcaster subclassesIlluminate\Broadcasting\Broadcasters\Broadcaster,Broadcast::auth()will still see an empty channel registry in tenant context.Please keep the “always resolve fresh” allowlist if needed, but make the channel-copy step capability-based instead of name-based.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Overrides/TenancyBroadcastManager.php` around lines 36 - 56, The current name-based gate (static::$tenantBroadcasters) prevents channel copying for custom drivers; instead, always copy channels when both the original and resolved broadcasters are capability-compatible. In get($name) still keep the allowlist behavior for "always resolve fresh" (static::$tenantBroadcasters) by resolving $newBroadcaster = $this->resolve($name) when in_array($name, static::$tenantBroadcasters) and otherwise obtain the broadcaster via parent::get($name); but after obtaining $originalBroadcaster (via $this->app->make(BroadcasterContract::class)) and $newBroadcaster, if both are instances of Illuminate\Broadcasting\Broadcasters\Broadcaster (the Broadcaster class used elsewhere) call $this->passChannelsFromOriginalBroadcaster($originalBroadcaster, $newBroadcaster) so channel inheritance is capability-based rather than driver-name-based.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/Bootstrappers/BroadcastingConfigBootstrapper.php`:
- Around line 62-68: The constructor is mutating the class-wide
static::$credentialsMap causing preset keys to leak across instances; instead
compute the merged map from static::$mapPresets and static::$credentialsMap and
assign it to an instance property (e.g. $this->credentialsMap) inside
__construct, leaving static::$credentialsMap unchanged; then update setConfig
(and any other methods referencing static::$credentialsMap) to read from
$this->credentialsMap so each bootstrapper instance keeps its own merged
credentials map.
In `@src/Overrides/TenancyBroadcastManager.php`:
- Around line 36-56: The current name-based gate (static::$tenantBroadcasters)
prevents channel copying for custom drivers; instead, always copy channels when
both the original and resolved broadcasters are capability-compatible. In
get($name) still keep the allowlist behavior for "always resolve fresh"
(static::$tenantBroadcasters) by resolving $newBroadcaster =
$this->resolve($name) when in_array($name, static::$tenantBroadcasters) and
otherwise obtain the broadcaster via parent::get($name); but after obtaining
$originalBroadcaster (via $this->app->make(BroadcasterContract::class)) and
$newBroadcaster, if both are instances of
Illuminate\Broadcasting\Broadcasters\Broadcaster (the Broadcaster class used
elsewhere) call $this->passChannelsFromOriginalBroadcaster($originalBroadcaster,
$newBroadcaster) so channel inheritance is capability-based rather than
driver-name-based.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 530b71bf-65da-4141-8bb5-9038284e58dd
📒 Files selected for processing (2)
src/Bootstrappers/BroadcastingConfigBootstrapper.phpsrc/Overrides/TenancyBroadcastManager.php
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.
Broadcasting auth route problem (
Broadcastfacade always uses centralBroadcastManager)When using
BroadcastingConfigBootstrapperfor broadcasting on private channels with multiple broadcasting apps (= each tenant has its own Pusher/Reverb/... app and credentials), the auth requests sent to the broadcasting server use the central broadcasting credentials. This is because theBroadcastfacade (used inBroadcastController::authenticatelikeBroadcast::auth($request)to authorize the channels and then retrieve the auth key that's then sent to the broadcasting server) keeps using theBroadcastManagerinstance resolved in the central context instead ofTenancyBroadcastManager. CallingwithBroadcasting()inbootstrap/app.phpresults in callingBroadcast::routes()in the central context, which resolves and stores the centralBroadcastManager(which is never cleared, and is used in tenant context).Clearing the facade's resolved
Illuminate\Contracts\Broadcasting\Factoryinstance inBroadcastingConfigBootstrapper::bootstrap()forces the facade to re-resolveFactory(=BroadcastManager) on the next use, so the nextBroadcast::auth()call usesTenantBroadcastManagerin the tenant context (and clearing the resolved instance inrevert()makes theBroadcastfacade use the centralBroadcastManageragain), and the credentials from the current config will be used for authentication.TenancyBroadcastManagerdoesn't inherit custom driver creatorsWhen registering a custom driver creator (
app(BroadcastManager::class)->extend('custom-driver', fn($app, $config) => new CustomBroadcaster(...))) in the central context (e.g. inBroadcastServiceProvider/AppServiceProvider, or in our case, in the tests), the creator won't be available in the tenant context. Calling e.g.app(BroadcastManager::class)->driver('custom-driver')in the tenant context (if the creator was registered only in the central context) would throwInvalidArgumentException("Driver [custom-driver] is not supported.").To fix that, register the original (central)
BroadcastManager's custom creators in the newTenancyBroadcastManagerinBroadcastingConfigBootstrapper::bootstrap().Central
Broadcasterinstance bound in tenant contextResolving
Illuminate\Contracts\Broadcasting\Broadcaster::classin the tenant context returns aBroadcasterinstance from the central context. Currently, this doesn't cause any issues, but it could be a problem when using DI/resolvingBroadcaster.Fixed by making
Illuminate\Contracts\Broadcasting\Broadcaster::classresolve to the driver of the currentBroadcastManagerinstance (which isTenancyBroadcastManagerin the tenant context) by usingapp->extend().But there's a catch: updating credentials in the config while in tenant context won't be reflected in the bound Broadcaster instance. To make that work (with direct config updates), we'd have to use
bind()instead ofextend(), but then, we wouldn't be able to resolve the original broadcaster inTenancyBroadcastManager::get()(so that we can pass its channels to the new broadcaster). We'd have to pass the original broadcaster toTenancyBroadcastManager, and for that, we'd have to override the manager's constructor, which doesn't seem worth, since this is a pretty niche use case and simply reinitializing tenancy fixes the problem anyway.For cases like sending a notification in response to updating tenant's broadcasting credentials in tenant context, it's recommended to reinitialize tenancy after updating the credentials.
Credential map: overriding presets (BroadcastingConfigBootstrapper)
Previously, credential mappings from
$mapPresetsoverrode mappings defined in$credentialsMap. If someone used e.g. Pusher and wanted to override some of that preset's mappings, e.g. use 'pusher_app_key' instead of 'pusher_key' by specifying 'pusher_app_key' in$credentialsMap, the preset's mapping ('pusher_key') would still be used.Fixed that by reversing the
array_merge()order inBroadcastingConfigBootstrapper::__construct().Tests
Deleted the BroadcastingTest file, moved the tests to appropriate bootstrapper test files.
Added tests for mapping tenant properties to broadcaster credentials. Also added tests for the rest of the changes mentioned above and improved the existing tests.
To-dos
TenancyBroadcastManager::$tenantBroadcasters(add it everywhere where the prop is modified to make the tests pass)Summary by CodeRabbit
New Features
Bug Fixes
Tests