[4.x] BroadcastingConfigBootstrapper: fix central instances persisting in tenant context#1448
[4.x] BroadcastingConfigBootstrapper: fix central instances persisting in tenant context#1448lukinovec wants to merge 64 commits into
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.57% 86.62% +0.04%
+ Complexity 1221 1219 -2
============================================
Files 187 186 -1
Lines 3575 3580 +5
============================================
+ Hits 3095 3101 +6
+ Misses 480 479 -1 ☔ View full report in Codecov by Harness. 🚀 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.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughBroadcasting now switches manager and broadcaster resolution during tenancy, refreshes facade bindings, applies tenant credential mappings, inherits channels, and restores central state. Expanded tests cover configuration propagation, isolation, lifecycle behavior, channel helpers, broadcaster compatibility, and state cleanup. ChangesTenant-aware broadcasting
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Tenancy
participant BroadcastingConfigBootstrapper
participant BroadcastManager
participant Broadcaster
participant BroadcastFacade
Tenancy->>BroadcastingConfigBootstrapper: initialize tenant context
BroadcastingConfigBootstrapper->>BroadcastManager: bind fresh tenant manager
BroadcastingConfigBootstrapper->>Broadcaster: bind tenant default connection
BroadcastingConfigBootstrapper->>BroadcastFacade: clear resolved instance
BroadcastFacade->>BroadcastManager: resolve tenant broadcaster
Tenancy->>BroadcastingConfigBootstrapper: end tenant context
BroadcastingConfigBootstrapper->>BroadcastFacade: restore central resolution
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
![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 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
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 `@src/Overrides/TenancyBroadcastManager.php`:
- Around line 27-42: Update TenancyBroadcastManager::resolve() to avoid
resolving BroadcasterContract through the container after the manager swap,
which can recurse via BroadcastManager::connection(); instead, preserve the
already-resolved central broadcaster by injecting or otherwise passing it
directly into the manager before resolve() runs, then use that instance in
passChannelsFromOriginalBroadcaster() while retaining the existing Broadcaster
type checks.
🪄 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: c6f806ae-4d43-4d09-b639-9d9563a4faca
📒 Files selected for processing (3)
src/Bootstrappers/BroadcastingConfigBootstrapper.phpsrc/Overrides/TenancyBroadcastManager.phptests/Bootstrappers/BroadcastingConfigBootstrapperTest.php
Use "userName" in the channel helper test's comment for consistency (the channel name uses "userName", the comment used "userId", fixed that)
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/Bootstrappers/BroadcastingConfigBootstrapper.php`:
- Around line 83-106: Wrap the BroadcastManager and Broadcaster extension
mutations plus Broadcast facade reset in a try/catch within the bootstrapper
method, and call this bootstrapper’s revert logic before rethrowing any
exception from connection() resolution. Ensure rollback also handles partial
setup before the bootstrapper is tracked. Add a regression test using a custom
driver creator that throws during connection(), asserting the configuration and
manager bindings are restored.
In `@tests/Bootstrappers/BroadcastChannelPrefixBootstrapperTest.php`:
- Around line 196-221: Update the re-registration assertions in the
tenant_channel test to retrieve the callback actually stored under the prefixed
channel name from getChannels() after the second tenant_channel() call. Use that
retrieved closure, rather than the locally assigned $tenantChannelClosure, for
all subsequent central- and tenant-context invocations while retaining the
identity assertion against $centralChannelClosure.
🪄 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: e8d31116-090a-437c-bb0d-e8304b55b593
📒 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
…nel() actually stored #1448 (comment)
…ly in the Broadcaster extend() closure Channel auth closures (registered using Broadcast::channel() e.g. in routes/channels.php) are only ever read from the default broadcaster -- both Broadcast::channel() and Broadcast::auth() go through the default broadcaster. TenancyBroadcastManager passed the closures to every broadcaster it resolved, which is pointless -- even in central context, Laravel doesn't do anything like that). Instead, bind a fresh BroadcastManager and pass the central channel closures to its default broadcaster while extending Broadcaster. So now, tenant context behaves exactly like central context, just with broadcasters resolved using the tenant credentials. Also update comments.
Remove the with() -- the test ran four times with the same TestingBroadcaster, so the only tested broadcaster was actually the testing one. Running the test just once is enough, the same things are still covered.
…ed to config, the central config values are kept
Originally, the test only asserted that the broadcasters inherited the correct channel names. Now, the test also asserts that the actual closure and the channel options are inherited too.
…ract (instead of the abstract class) don't break bootstrap() Covers the `$centralBroadcaster instanceof Broadcaster && $tenantBroadcaster instanceof Broadcaster` check in extend(BroadcasterContract::class, ...)
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 (1)
src/Bootstrappers/BroadcastingConfigBootstrapper.php (1)
124-132: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winClear the configuration snapshot after reverting.
$originalConfigsurvivesrevert(). If central broadcasting configuration changes between tenancy sessions, the nextsetConfig()keeps the stale values because of??=, andunsetConfig()later restores those old values instead of the current central configuration.Reset the snapshot after restoring the config.
🛠️ Proposed fix
$this->unsetConfig(); + $this->originalConfig = [];🤖 Prompt for 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. In `@src/Bootstrappers/BroadcastingConfigBootstrapper.php` around lines 124 - 132, Update revert() in BroadcastingConfigBootstrapper to clear the $originalConfig snapshot after restoring the central broadcasting configuration. Ensure the reset occurs before or alongside the existing unsetConfig() cleanup so the next setConfig() captures current values instead of reusing stale data.
🤖 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.
Outside diff comments:
In `@src/Bootstrappers/BroadcastingConfigBootstrapper.php`:
- Around line 124-132: Update revert() in BroadcastingConfigBootstrapper to
clear the $originalConfig snapshot after restoring the central broadcasting
configuration. Ensure the reset occurs before or alongside the existing
unsetConfig() cleanup so the next setConfig() captures current values instead of
reusing stale data.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d36e5e68-49bd-448d-b3b5-cf6cfa38cf76
📒 Files selected for processing (2)
src/Bootstrappers/BroadcastingConfigBootstrapper.phptests/Bootstrappers/BroadcastingConfigBootstrapperTest.php
| // (which all of Laravel's default broadcasters, e.g. PusherBroadcaster, do). | ||
| if ($centralBroadcaster instanceof Broadcaster && $tenantBroadcaster instanceof Broadcaster) { | ||
| // invade() because channels can't be retrieved through any of the broadcaster's public methods | ||
| $centralBroadcaster = invade($centralBroadcaster); |
There was a problem hiding this comment.
There's a getChannels() method on Broadcaster I think
There was a problem hiding this comment.
Weird, I thought there was no method like that, but looking at the Broadcaster's blame, the method was added in Laravel 10.2.0. But nevermind, good catch, I'll use getChannels() here.
For the options we still need invade() either way. Both retrieveChannelOptions() and the $channelOptions prop are protected, so getChannels() doesn't let us remove invade() entirely. We'd still need either invade($centralBroadcaster)->retrieveChannelOptions($channel) per channel, or $channelOptions = invade($centralBroadcaster)->channelOptions once.
I guess the latter could be a bit more correct because retrieveChannelOptions() does some pattern matching and returns the first matching pattern's options, so e.g. if we had orders.{x} and orders.{id} channels registered, retrieveChannelOptions('orders.{id}') would return orders.{x}'s options and we'd copy those onto the wrong channel. This couldn't really break anything in practice because when patterns overlap like that, Laravel always uses the first matching one during auth (for both the callback and the options), so the second channel's options never get read anyway. But reading $channelOptions[$channel] directly means the copy is just a 1:1 copy, no pattern matching involved.
So the code could look like this:
if ($centralBroadcaster instanceof Broadcaster && $tenantBroadcaster instanceof Broadcaster) {
// invade() because the channel options can't be retrieved through any of the broadcaster's public methods
$channelOptions = invade($centralBroadcaster)->channelOptions;
foreach ($centralBroadcaster->getChannels() as $channel => $callback) {
$tenantBroadcaster->channel($channel, $callback, $channelOptions[$channel] ?? []);
}
}Or we can just correct the comment about invade() (so that it says invade() is needed to access the channel options, not the channels) and keep using retrieveChannelOptions().
I guess I'd just change the code like I suggested in the example above. There's no issue with using $channelOptions instead of the retrieveChannelOptions() method, and it'll let us use invade() for the thing it's actually needed for.
There was a problem hiding this comment.
I see that a PHPStan error came up:
Error: Ignored error pattern #Spatie\\Invade\\Invader# (method.notFound) was not matched in reported errors.
method.notFound
That's because now, we don't do invade($thing)->method() -- we do invade($thing)->property, right (= instead of a method call on invade(), we access a property through invade())?
There was a problem hiding this comment.
It just says an ignore was not used for anything. The ignore is in phpstan.neon, there was one for method.notFound and one for property.notFound. Former is now unused. I'll just comment it out.
There was a problem hiding this comment.
I guess the latter could be a bit more correct because retrieveChannelOptions() does some pattern matching and returns the first matching pattern's options, so e.g. if we had orders.{x} and orders.{id} channels registered
Can you confirm in confident terms if there would be any real issue here.
Or we can just correct the comment about invade() (so that it says invade() is needed to access the channel options, not the channels) and keep using retrieveChannelOptions().
And then it'd be just invade() just because of the method being protected?
I'm thinking if we could address this instance similarly to my other suggestion with forgetDrivers() assuming that suggestion is usable.
Main things I'm looking at:
- facade points to the manager
- manager redirects
__callto the default driver - that's how e.g.
broadcast()is handled by the manager butchannel()is handled by the default connection/driver - what we're doing here is creating a fresh instance on the new driver and then copying over old properties
There are some issues with this approach and the alternative approach (resetting the existing object).
Current approach:
- only handles channels and channel options? There's a bunch of other state on
Broadcaster(also included in the facade docblocks) such asresolveAuthenticatedUserUsingwhich we do not copy - requires
invade()
Resetting the existing object:
- we could just keep all this various state on the object and only reset the parts that are central/tenant-specific, the parts that matter, but that would be driver specific. We'd need driver-specific logic but perhaps it could work better
For instance on Pusher: getPusher() and setPusher() are public and there's a public BroadcastManager::pusher() method.
But there may be a bigger problem with this approach which is that BroadcastManager->drivers['default driver'] and app(BroadcasterContract::class) go out of sync. The singleton instance remains bound but isn't part of the manager anymore. So we'd still need to copy that into the new BroadcastManager either with some reflection or some ugly hack that leverages how custom creators or something like that is exactly implemented.
Another separate thing: if we're using reflection anyway in this closure, is there a reason to do things as verbose as:
$channelOptions = invade($centralBroadcaster)->channelOptions;
foreach ($centralBroadcaster->getChannels() as $channel => $callback) {
$tenantBroadcaster->channel($channel, $callback, $channelOptions[$channel] ?? []);
}Instead of literally just copying the channels property, channelOptions property and all the other state that should be looked at as I mentioned here?
There was a problem hiding this comment.
Can you confirm in confident terms if there would be any real issue [with retrieveChannelOptions()] here.
If registering channels like orders.{x} and orders.{y} together is a common thing, retrieveChannelOptions would be a problem as I wrote in the previous reply. If that's not common, there should be no issue with retrieveChannelOptions (either way, channelOptions won't be a problem)
And then it'd be just invade() just because of the method being protected?
Yes
the
get/setPusher()/reset the existing object idea
Yeah, that could work for Pusher/Ably, but custom drivers would be a problem since we'd need to fall back to recreating these via the manager anyway (I think), and that'd complicate the code.
Maybe it'd make sense to implement the resetting solution since it has real benefits (preserving broadcaster state + keeping already-injected instances valid), but we still want to support custom drivers, and I'm not sure if that's worth the extra complexity.
So it's either just keep the current recreate + copy logic, or reset the existing object and keep the recreate + copy logic as a fallback for custom drivers.
But there may be a bigger problem with this approach which is that BroadcastManager->drivers['default driver'] and app(BroadcasterContract::class) go out of sync ... So we'd still need to copy that into the new BroadcastManager either with some reflection or some ugly hack...
Right, the syncing problem would be real. What you sent on Discord should fix it though:
$defaultBroadcaster = ...
Manager::forgetDrivers()
$defaultBroadcaster->setPusher
invade($manager)->drivers = [ $defaultBroadcaster ]invade($manager)->drivers = [....] would keep the singleton and the manager cache pointing at one object.
Another separate thing: if we're using reflection anyway in this closure, is there a reason to do things as verbose as ... Instead of literally just copying the channels property, channelOptions property and all the other state that should be looked at as I mentioned here?
I think the reason to do things the verbose way is to call the $broadcaster->channel(...) method so that the channels are copied "properly" since the channel() method does some extra things besides setting the channels/channelOptions properties. But that might not be relevant. You're right that we're missing authenticatedUserCallback here. There's another property, bindingRegistrar, but that's resolved lazily, so I don't think we need to copy that.
I'm testing your suggestions now (related: #1448 (comment)), and I'll let you know how the implementation could look.
There was a problem hiding this comment.
Current approach: only handles channels and channel options? There's a bunch of other state on Broadcaster (also included in the facade docblocks) such as resolveAuthenticatedUserUsing which we do not copy
Updated a test so that the auth user callback copying is tested (the test fails):
50afc49
... literally just copying the channels property, channelOptions property and all the other state that should be looked at
For now, I changed the implementation like this: fe7468a
It makes the test added in previous linked commit pass (and all the other tests pass too, so this approach works).
Looking into the "resetting the existing object" implementation more now.
There was a problem hiding this comment.
Looking into the "resetting the existing object" implementation more now
So I tried implementing this.
The full diff
diff --git a/src/Bootstrappers/BroadcastingConfigBootstrapper.php b/src/Bootstrappers/BroadcastingConfigBootstrapper.php
index 2afd61a..594605f 100644
--- a/src/Bootstrappers/BroadcastingConfigBootstrapper.php
+++ b/src/Bootstrappers/BroadcastingConfigBootstrapper.php
@@ -4,18 +4,20 @@ declare(strict_types=1);
namespace Stancl\Tenancy\Bootstrappers;
+use Illuminate\Broadcasting\Broadcasters\AblyBroadcaster;
use Illuminate\Broadcasting\Broadcasters\Broadcaster;
+use Illuminate\Broadcasting\Broadcasters\PusherBroadcaster;
use Illuminate\Broadcasting\BroadcastManager;
use Illuminate\Config\Repository;
use Illuminate\Contracts\Broadcasting\Broadcaster as BroadcasterContract;
use Illuminate\Foundation\Application;
-use Illuminate\Support\Facades\Broadcast;
use Stancl\Tenancy\Contracts\TenancyBootstrapper;
use Stancl\Tenancy\Contracts\Tenant;
/**
- * Maps tenant credentials to the broadcasting config and rebinds BroadcastManager
- * and Broadcaster so that broadcasters get resolved using the tenant credentials.
+ * Maps tenant credentials to the broadcasting config and swaps the default broadcaster's
+ * underlying client (e.g. the Pusher instance) for one built with the tenant credentials,
+ * keeping the broadcaster and manager instances intact.
*/
class BroadcastingConfigBootstrapper implements TenancyBootstrapper
{
@@ -33,8 +35,19 @@ class BroadcastingConfigBootstrapper implements TenancyBootstrapper
public static string|null $broadcaster = null;
+ /**
+ * Callbacks for making a broadcaster use the passed config, keyed by connection name.
+ * Usually that means swapping the broadcaster's client for one created using the passed
+ * config (that's how Pusher and Ably, supported out of the box, work), but it's up to the
+ * callback -- e.g. a driver storing its config in a property can just overwrite it:
+ *
+ * ['foobar' => fn (Broadcaster $broadcaster, BroadcastManager $manager, array $config) => invade($broadcaster)->config = $config]
+ *
+ * Custom drivers without a resetter fall back to the recreate + copy logic (see resetBroadcasting()).
+ */
+ public static array $broadcasterResetters = [];
+
protected array $originalConfig = [];
- protected BroadcastManager|null $originalBroadcastManager = null;
protected BroadcasterContract|null $originalBroadcaster = null;
public static array $mapPresets = [
@@ -66,44 +79,84 @@ class BroadcastingConfigBootstrapper implements TenancyBootstrapper
public function bootstrap(Tenant $tenant): void
{
- $this->originalBroadcastManager = $this->app->make(BroadcastManager::class);
$this->originalBroadcaster = $this->app->make(BroadcasterContract::class);
$this->setConfig($tenant);
- // Make BroadcastManager resolve to a fresh manager with no cached broadcasters,
- // so that its broadcasters get resolved using the updated (tenant) broadcasting
- // config and stay cached for the duration of the tenant's context.
- $this->app->extend(BroadcastManager::class, function (BroadcastManager $centralManager) {
- $tenantManager = new BroadcastManager($this->app);
+ $this->resetBroadcasting();
+ }
- // Pass the custom driver creators registered in the central context to the new manager
- // so that custom drivers work in tenant context without having to re-register the creators manually.
- foreach (invade($centralManager)->customCreators as $driver => $creator) {
- $tenantManager->extend($driver, $creator);
+ /**
+ * Make the broadcasters use the current broadcasting config.
+ *
+ * The default broadcaster's client gets swapped for a client created using the current config,
+ * while the broadcaster instance itself stays the same (its auth state stays untouched, and
+ * all the already-resolved broadcaster/manager instances will be up-to-date with the changes).
+ */
+ protected function resetBroadcasting(): void
+ {
+ $manager = $this->app->make(BroadcastManager::class);
+ $default = $this->config->get('broadcasting.default');
+
+ // Clear the manager's cached broadcasters so that the non-default broadcasters get
+ // re-resolved using the current config (custom driver creators stay registered).
+ $manager->forgetDrivers();
+
+ $broadcaster = $this->originalBroadcaster;
+
+ if ($broadcaster instanceof Broadcaster && $this->resetBroadcaster($broadcaster, $manager, $default)) {
+ // The broadcaster now uses the current config. Cache the broadcaster instance in the
+ // manager (since forgetDrivers() cleared it), so that the manager and the bound
+ // Broadcaster singleton keep using the same instance.
+ invade($manager)->drivers = [$default => $broadcaster];
+
+ $this->app->instance(BroadcasterContract::class, $broadcaster);
+ } else {
+ // If the broadcaster can't be reset (a custom driver without a registered resetter),
+ // fall back to resolving a fresh broadcaster using the current config
+ // and copying the auth state to it.
+ $freshBroadcaster = $manager->connection();
+
+ if ($broadcaster) {
+ $this->copyAuthState($broadcaster, $freshBroadcaster);
}
- return $tenantManager;
- });
+ $this->app->instance(BroadcasterContract::class, $freshBroadcaster);
+ }
+ }
+
+ /**
+ * Make the broadcaster use the given connection's current config.
+ *
+ * Different drivers store the credentials in different places, so this is driver-specific:
+ * Pusher and Ably store them in the broadcaster's client, so their client gets swapped for one
+ * created using the current config. Custom drivers get reset using the resetter registered for the connection
+ * (see $broadcasterResetters). Returns false when there's no known way to reset the broadcaster
+ * (no registered resetter and not a Pusher/Ably broadcaster).
+ */
+ protected function resetBroadcaster(Broadcaster $broadcaster, BroadcastManager $manager, string $connection): bool
+ {
+ $config = $this->config->get("broadcasting.connections.{$connection}") ?? [];
- // Swap the currently bound Broadcaster singleton (resolved earlier with the central credentials)
- // for the tenant BroadcastManager's default broadcaster, so that anything resolving the Broadcaster
- // contract gets the same tenant broadcaster that the manager uses, instead of the stale central one.
- // The closure runs immediately (the extended singleton is already resolved), and it's also what makes
- // channel auth work in tenant context -- the broadcaster resolved here gets cached as the tenant
- // manager's default driver and receives the central broadcaster's auth state (see copyAuthState()).
- $this->app->extend(BroadcasterContract::class, function (BroadcasterContract $centralBroadcaster) {
- $tenantBroadcaster = $this->app->make(BroadcastManager::class)->connection();
+ if ($resetter = static::$broadcasterResetters[$connection] ?? null) {
+ $resetter($broadcaster, $manager, $config);
- $this->copyAuthState($centralBroadcaster, $tenantBroadcaster);
+ return true;
+ }
+
+ if ($broadcaster instanceof PusherBroadcaster) {
+ $broadcaster->setPusher($manager->pusher($config));
- return $tenantBroadcaster;
- });
+ return true;
+ }
- // Extending the binding doesn't update the Broadcast facade's cached instance,
- // so clear it to make the facade re-resolve to the tenant BroadcastManager instead of the central
- // one — e.g. in the Broadcast::auth() call in BroadcastController (/broadcasting/auth).
- Broadcast::clearResolvedInstance();
+ if ($broadcaster instanceof AblyBroadcaster) {
+ $broadcaster->setAbly($manager->ably($config));
+
+ return true;
+ }
+
+ return false;
}
/**
@@ -134,14 +187,9 @@ class BroadcastingConfigBootstrapper implements TenancyBootstrapper
public function revert(): void
{
- // Revert the bound BroadcastManager and Broadcaster singletons back to their original state
- $this->app->instance(BroadcastManager::class, $this->originalBroadcastManager);
- $this->app->instance(BroadcasterContract::class, $this->originalBroadcaster);
-
- // Clear the resolved Broadcast facade instance so that it gets re-resolved as the central BroadcastManager
- Broadcast::clearResolvedInstance();
-
$this->unsetConfig();
+
+ $this->resetBroadcasting();
}
protected function setConfig(Tenant $tenant): void
diff --git a/tests/Bootstrappers/BroadcastingConfigBootstrapperTest.php b/tests/Bootstrappers/BroadcastingConfigBootstrapperTest.php
index 110d028..6796986 100644
--- a/tests/Bootstrappers/BroadcastingConfigBootstrapperTest.php
+++ b/tests/Bootstrappers/BroadcastingConfigBootstrapperTest.php
@@ -15,6 +15,7 @@ use Illuminate\Contracts\Broadcasting\Broadcaster as BroadcasterContract;
$cleanup = function () {
BroadcastingConfigBootstrapper::$broadcaster = null;
BroadcastingConfigBootstrapper::$credentialsMap = [];
+ BroadcastingConfigBootstrapper::$broadcasterResetters = [];
BroadcastingConfigBootstrapper::$mapPresets = [
'pusher' => [
'broadcasting.connections.pusher.key' => 'pusher_key',
@@ -40,6 +41,10 @@ beforeEach(function () use ($cleanup) {
Event::listen(TenancyEnded::class, RevertToCentralContext::class);
$cleanup();
+
+ BroadcastingConfigBootstrapper::$broadcasterResetters['testing'] = function ($broadcaster, $manager, array $config) {
+ $broadcaster->config = $config;
+ };
});
afterEach($cleanup);
@@ -85,6 +90,26 @@ test('ending tenancy reverts the bound broadcaster to the original instance', fu
expect($originalBroadcaster)->toBe(app(BroadcasterContract::class));
});
+test('the bound broadcaster and the manager default driver are the same instance in all contexts', function () {
+ config([
+ 'tenancy.bootstrappers' => [BroadcastingConfigBootstrapper::class],
+ 'broadcasting.default' => 'testing',
+ 'broadcasting.connections.testing.driver' => 'testing',
+ ]);
+
+ app(BroadcastManager::class)->extend('testing', fn ($app, $config) => new TestingBroadcaster('testing', $config));
+
+ expect(app(BroadcasterContract::class))->toBe(app(BroadcastManager::class)->driver());
+
+ tenancy()->initialize(Tenant::create());
+
+ expect(app(BroadcasterContract::class))->toBe(app(BroadcastManager::class)->driver());
+
+ tenancy()->end();
+
+ expect(app(BroadcasterContract::class))->toBe(app(BroadcastManager::class)->driver());
+});
+
test('BroadcastingConfigBootstrapper maps tenant properties to broadcaster credentials correctly', function (string $driver) {
config([
'broadcasting.default' => $driver,
bootstrap() and revert() both end up doing the same thing: update the config -> forgetDrivers() -> swap the default broadcaster's client on the existing broadcaster ($broadcaster->setPusher($manager->pusher($config))) -> cache that broadcaster in the manager again.
invade($manager)->drivers = [....] would keep the singleton and the manager cache pointing at one object.
Confirmed -- added a minimal test for this (assert app(BroadcasterContract::class) === app(BroadcastManager::class)->driver() in all contexts) and it passes. It's a separate test only because the existing ending tenancy reverts the bound broadcaster to the original instance test (which already checks this, but only in tenant context) fails on its first assertion with this implementation, so assertions added there wouldn't even run. If we keep the current implementation, I'd just add the missing assertions to that existing test instead of adding the new one.
... either with some reflection or some ugly hack that leverages how custom creators or something like that is exactly implemented.
Tried if this could work without invade(), using $manager->extend($default, fn () => $broadcaster) (forgetDrivers() doesn't delete the creators, so the manager keeps returning the same instance) -- the tests pass the same as with invade(), but it's wrong: $drivers are keyed by connection name, while $customCreators are keyed by driver name ($config['driver']). So for a connection named differently than its driver (e.g. connection foo with the pusher driver), the creator wouldn't get used at all, and if it got registered by the driver name instead, it would hijack all connections that use that driver (+ override a creator the user registered themselves). So invade() is correct here.
So it's either just keep the current recreate + copy logic, or reset the existing object and keep the recreate + copy logic as a fallback for custom drivers.
This turned out to be right -- custom drivers have no setPusher() equivalent, so I added a static $broadcasterResetters property where the user can register a closure that resets their driver's broadcaster (makes it use the current config), and connections without a resetter fall back to the recreate + copy logic.
(About the diff: the tests use TestingBroadcaster everywhere, and the reset path only handles PusherBroadcaster/AblyBroadcaster + connections with a registered resetter. So in the tests, I registered a resetter for the 'testing' connection to cover the reset path, and the tests using other connections -- e.g. the credential mapping test -- have no resetter, so they cover the fallback.)
Note that we use the custom 'testing' driver in the config bootstrapper tests, so I set the resetter for it using BroadcastingConfigBootstrapper::$broadcasterResetters['testing'] = ... in beforeEach. So tests that use the 'testing' driver exercise the reset path.
There's also a problem we didn't bring up before (wrote about this on Discord): preserving all the state on the single broadcaster also preserves the state we don't want to keep. Auth state (channels, channel options and user auth callbacks) registered in tenant context stays on the broadcaster after tenancy ends, and custom creators registered in tenant context persist between contexts too -- the two isolation tests fail because of this. It's fixable by saving the original state in bootstrap() and restoring it in revert(), but that's the auth state copying logic again. That said, registering these things in tenant context is pretty unlikely in real apps -- channels, creators and the user auth callback are normally registered in service providers (= in central context) -- so it is wrong, but probably wouldn't be a problem in the vast majority of apps.
Overall, 11/15 tests pass -- the two isolation tests fail (as mentioned above), and then the two instance check tests which assert that the manager/broadcaster are fresh instances. That's the opposite of what this approach does, so those two don't really tell us anything.
So the advantage of resetting the existing object is that the already-resolved/injected instances stay valid across context switches. The disadvantages:
- driver-specific reset logic
- the resetter static property solution for custom drivers
- the "recreate + copy" fallback
- saving/restoring the broadcaster's channels and the manager's custom creators, so that the ones registered in tenant context don't stay registered after tenancy ends (not in the diff -- this would have to be added to make the failing isolation tests pass)
IMO the current implementation is the better trade-off. One approach instead of two, and the context isolation (custom creator and auth state) isn't a problem since each context has its own instances.
| $this->app->extend(BroadcastManager::class, function (BroadcastManager $centralManager) { | ||
| $tenantManager = new BroadcastManager($this->app); | ||
|
|
||
| // Pass the custom driver creators registered in the central context to the new manager | ||
| // so that custom drivers work in tenant context without having to re-register the creators manually. | ||
| foreach (invade($centralManager)->customCreators as $driver => $creator) { | ||
| $tenantManager->extend($driver, $creator); | ||
| } | ||
|
|
||
| return $tenantManager; | ||
| }); |
There was a problem hiding this comment.
Is the reason for creating the instance manually like this that if we simply forgot the bound instance from the container, the custom creators that were already registered would be lost?
To confirm - when would BroadcastManager::extend() be used? Is this change simply because of how we do things in our tests or would real applications also extend the manager before tenancy bootstrap?
And what are some common use cases.
There was a problem hiding this comment.
Is the reason for creating the instance manually like this that if we simply forgot the bound instance from the container, the custom creators that were already registered would be lost?
Exactly, yeah
To confirm - when would BroadcastManager::extend() be used? Is this change simply because of how we do things in our tests or would real applications also extend the manager before tenancy bootstrap?
I made the change because when I reviewed the tests, I realized that this could be a real problem with any custom driver.
Custom drivers are registered via Broadcast::extend('foobar', ...) in a service provider's boot(), which runs before tenancy is initialized, so in a real app the creators are always on the central manager before bootstrap. Without copying them, a tenant using a custom driver as the default broadcaster would get "Driver [foobar] is not supported" in the tenant context.
And what are some common use cases
Writing your own driver, or installing a package that ships one. I see that for example, the Centrifugo package registers its driver exactly like this, in its provider's boot() (it calls extend() on an injected BroadcastManager instead of the facade, but that's the same thing since Broadcast::extend() proxies to the manager): https://github.com/denis660/laravel-centrifugo/blob/bec36ea8323fb56dff42bfc3999600abc567afaf/src/CentrifugoServiceProvider.php#L27-L29
There was a problem hiding this comment.
Is there a reason we are recreating the manager from scratch, and then copying existing stuff into the new manager, instead of trying to reset parts of the manager - if possible. The ones that contain state that becomes utdated in the tenant context.
As a brief test, I looked at what forget* methods there are in the manager and what properties exist there. I saw forgetDrivers() and that there aren't really many other properties that could hold outdated state. If I replace this whole thing with just forgetDrivers() on the bound BroadcastManager, all tests except two assertions that are coupled to this specific implementation seem to pass.
Have we tried that before and was it insufficient to make everything work? Wondering why we'd be re-instantiating managers and copying private properties to new instances and things like that in this bootstrapper when we normally just try to reset state.
Thought of this when I looked at the invade()-related review since I checked where we use invade and it's only this bootstrapper and some tests.
There was a problem hiding this comment.
As a brief test ... If I replace this whole thing with just forgetDrivers() on the bound BroadcastManager, all tests except two assertions that are coupled to this specific implementation seem to pass.
Tested this too -- these tests are coupled to the specific implementation, so yeah, these fail. Other tests pass because we don't deal with the custom creator inheritance, the BroadcastManager extend and forgetDrivers have similar primary purpose (= make the manager resolve fresh broadcasters).
(actually, I think there's also a gap in the tests -- we don't have a test for whether revert restores the central credentials after a tenant with overrides, and that one would fail with forgetDrivers() -- more on that below)
But forgetDrivers is problematic.
Have we tried that before and was it insufficient to make everything work? Wondering why we'd be re-instantiating managers and copying private properties to new instances and things like that in this bootstrapper when we normally just try to reset state.
I don't think we have tried that at all because before, we were swapping the bound broadcast manager for the custom TenancyBroadcastManager (so there was no reason to try that). And after we removed that class, it didn't occur to me to test using just forgetDrivers() instead of the extend().
forgetDrivers() keeps the custom creators (since it only clears $drivers), so custom drivers would still work without the need to copy anything. The only custom creator difference is that a creator registered inside a tenant context would leak into later contexts instead of being isolated per tenant (that's why one of the tests fails), but that's an edge case.
But the worse thing is that we have to properly revert() what bootstrap() does. If we just did forgetDrivers() on the bound manager in bootstrap(), things will work just fine in tenant context (if we ignore custom creators). Then on revert(), if we did forgetDrivers(), we'd discard the tenant broadcaster, but nothing would copy the channel closures back to the central broadcaster. To make the full forgetDrivers() implementation actually correct, we'd have to copy the channel closures onto the rebuilt central broadcaster on revert too.
And if instead we kept instance() on revert (not forgetDrivers()), the manager's cached default driver would still hold the previous (tenant) broadcaster, so /broadcasting/auth in central context would use the old tenant's key (the channel closures were copied over during bootstrap, so /broadcasting/auth wouldn't 403, but the response would be signed using the tenant's key, and the websocket server would just reject it).
So both revert solutions need copying extra state to be correct, which is why extend() (restoring the untouched central instances via instance()) probably ends up cleaner. I'll try how that implementation could look, but like I wrote, I think the extend() may be cleaner in the end -- I'll let you know.
There was a problem hiding this comment.
(Keeping all the changes local for now since this review is tied to #1448 (comment), and we'll be doing more testing/code changes)
There was a problem hiding this comment.
actually, I think there's also a gap in the tests -- we don't have a test for whether revert restores the central credentials after a tenant with overrides, and that one would fail with forgetDrivers()
Added assertions for this to the credential mapping test. They fail with the forgetDrivers() thing we tested in bootstrap(), pass with extend().
There was a problem hiding this comment.
So both revert solutions need copying extra state to be correct, which is why extend() (restoring the untouched central instances via instance()) probably ends up cleaner. I'll try how that implementation could look
Tried it, and it's like I wrote -- bootstrap() gets simpler (forgetDrivers() + the same auth state copying), but revert() has to re-resolve the central broadcaster and copy the auth state back onto it:
public function revert(): void
{
$this->unsetConfig();
// Clear the tenant broadcasters so they get re-resolved using the central config again
$this->app->make(BroadcastManager::class)->forgetDrivers();
// A freshly resolved broadcaster has no auth state, so copy it back from the original broadcaster
$centralBroadcaster = $this->app->make(BroadcastManager::class)->connection();
$this->copyAuthState($this->originalBroadcaster, $centralBroadcaster);
$this->app->instance(BroadcasterContract::class, $centralBroadcaster);
Broadcast::clearResolvedInstance();
}So the copying happens in both directions instead of once (meaning invade() stays too), the original broadcaster isn't restored on revert -- a fresh instance replaces it, so instances resolved before initializing tenancy end up stale. This all would be a bit more messy than the current implementation, so imo, switching to this won't be worth it.
(About the new assertions I mentioned in the previous comment -- with the full implementation, these assertions pass too, since revert() re-resolves the broadcaster with the central config -- only the instance check assertions fail)
Also tested the "resetting the existing object" idea from the other review, that's more interesting. I'll comment on that in a minute.
There was a problem hiding this comment.
Also tested the "resetting the existing object" idea from the other review, that's more interesting. I'll comment on that
See #1448 (comment)
Use `instance` instead of `singleton` in `revert()` (they're pretty much equivalent) Co-authored-by: Samuel Stancl <samuel@archte.ch>
…of using `invade()` for that We still need to use invade() to obtain the channelOptions.
The existing assertions only ran after a tenant without credential overrides, so a tenant broadcaster leaking into central context could hold the central key anyway and go unnoticed (works as a regression for the forgetDrivers() refactor)
…casters Currently, the test will fail because the resolvers don't get copied over to tenant broadcasters.
Instead of re-registering the central channels on the tenant broadcaster one by one, copy the properties directly. Also the authenticated user callback wasn't copied before, so user authentication (/broadcasting/user-auth) that worked in central context would 403 in tenant context if the callback was registered in central context.
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 of the tenantBroadcastManager. 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 instance (
Illuminate\Contracts\Broadcasting\Factory) inBroadcastingConfigBootstrapper::bootstrap()forces the facade to re-resolveFactory(=BroadcastManager) on the next use, so the nextBroadcast::auth()call uses the tenantBroadcastManagerin 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.Central
BroadcastManagerdoesn't pass custom driver creators to the tenant managerWhen 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 newBroadcastManagerinBroadcastingConfigBootstrapper::bootstrap().Central
Broadcasterinstance bound in tenant contextThe problem was that resolving
Illuminate\Contracts\Broadcasting\Broadcaster::classin tenant context returned the broadcaster instance from the central context (Laravel binds the contract as a singleton, resolved from the default driver's config as it was at resolution time -- before tenancy was initialized). Fixed by makingBroadcaster::classresolve to the currentBroadcastManager's default broadcaster (= the tenant broadcaster in tenant context) viaapp->extend().Since the manager caches its broadcasters (see "Broadcasters are resolved once per tenancy initialization" below),
app(Broadcaster::class)andBroadcast::driver()return the same instance in tenant context (consistent with how Laravel behaves in the central context).How
BroadcastingConfigBootstrapper::bootstrap()works (theextend()calls)Both
extend()calls run immediately duringbootstrap(), not lazily on some later resolution --extend()executes the closure right away when the extended singleton is already resolved, and both singletons are resolved at the top ofbootstrap()(where we save the central instances forrevert()). So when tenancy initializes:setConfig()maps the tenant's properties to the broadcasting config.BroadcastManagerextend swaps the bound manager for a fresh one with an empty driver cache and passes the central manager's custom driver creators to it.Broadcastercontract extend callsconnection()on the tenant manager, which resolves the default broadcaster using the updated (tenant) config and caches it as the manager's default driver. The central broadcaster's channel auth closures are then copied onto this tenant broadcaster.Broadcast::clearResolvedInstance()makes the facade re-resolve on the next call, so it returns the tenant manager instead of the stale central one.When
/broadcasting/authgets hit later,Broadcast::auth()goes through the tenant manager'sdriver(), which returns the broadcaster cached in step 3 (built with the tenant credentials, using the copied channel auth closures). This means theBroadcastercontract extend isn't just for code that resolves or injects the contract directly -- it's also what makes channel auth work in tenant context. Without it,app(Broadcaster::class)would keep returning the stale central broadcaster, and the tenant broadcaster's$channelswould stay empty, soBroadcast::auth()would throw a 403 for every channel registered in the central context.Broadcasters are resolved once per tenancy initialization,
TenancyBroadcastManageris removedOriginally,
TenancyBroadcastManagerre-resolved the broadcasters listed in its$tenantBroadcastersstatic property on every retrieval. Earlier, we considered that good since it meant direct config changes in tenant context were picked up immediately. As discussed in the review, there's probably no real use case for that, and the re-resolving actually caused a subtle bug: channels registered viaBroadcast::channel()in tenant context got silently lost on the next retrieval (e.g. during a/broadcasting/authrequest), because each re-resolution started over from a copy of the central channels -- making the auth request fail with a 403 as if the channel was never registered.Now,
TenancyBroadcastManageris removed entirely.BroadcastingConfigBootstrapperbinds a fresh, baseBroadcastManagerwith no cached broadcasters, so the broadcasters get resolved using the tenant's credentials on first use and stay cached (like in the parent manager) for the duration of the tenant's context. The central broadcaster's channel auth closures are passed to the tenant broadcaster directly in theBroadcastercontract'sextend()closure -- since Laravel only ever uses the default broadcaster's channel auth closures for broadcasting auth (bothBroadcast::channel()andBroadcast::auth()go through the default broadcaster), the closures only have to be passed to the default broadcaster.The channel auth closures are always only on the default broadcaster, even in plain Laravel. Every
Broadcast::channel()call adds an entry to the same$channelsarray on the default broadcaster -- a channel with its own authorization rules is just one of those entries, so it gets copied along with the rest. The only way to register a closure on a non-default broadcaster is callingBroadcast::driver('foobar')->channel(...)explicitly, and nothing in Laravel's auth flow ever reads those (/broadcasting/authalways authenticates using the default broadcaster).This all means that:
TenancyBroadcastManageris removed. Its$tenantBroadcastersproperty's purpose was listing the broadcasters to re-resolve (and pass the central channel auth closures to) -- custom drivers now work without the need to configure anything, and the closure copying is a few lines in the bootstrapper instead of a manager override.Broadcast::channel()in tenant context persist for the duration of that context. They don't leak into other tenants' contexts or into the central context.bootstrap(). An already-injected (= stale)Broadcasterinstance additionally needs to be obtained again after reinitialization -- reinitializing swaps the bound instance, so a previously injected instance keeps using the old credentials.NOTE: Already-connected clients stop receiving broadcasts after a credential update
Updating a tenant's credentials doesn't disconnect already-connected clients (tested with Reverb) -- they stay connected using the old key, but they stop receiving broadcasts, since broadcasts sent after the update are sent with the new credentials.
The frontend has to reconnect using the new key (e.g. by refreshing the page). Meaning, clients can't be notified about the credential change through websockets themselves -- a broadcast sent after the update already uses the new credentials, so it won't reach clients connected with the old key. So for things like notifying clients in response to the credential changes, a different mechanism is needed.
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 (including keeping the central config values when a tenant doesn't have a mapped property). Also added tests for the rest of the changes mentioned above (including a regression test for the tenant-context channel registration bug), plus tests for copying the channel options along with the auth closures, for broadcasters that only implement the
Broadcastercontract (instead of extending the abstract class), and for configuring which map preset is used via the$broadcasterproperty. Improved the existing tests.Summary by CodeRabbit
Summary of changes
Bug Fixes
Refactor
Tests
Chores