-
-
Notifications
You must be signed in to change notification settings - Fork 487
[4.x] BroadcastingConfigBootstrapper: fix central instances persisting in tenant context #1448
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
lukinovec
wants to merge
57
commits into
master
Choose a base branch
from
broadcasting-fixes
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
57 commits
Select commit
Hold shift + click to select a range
7d749eb
BroadcastingConfigBootstrapper: test mapping credentials
lukinovec 2ecc94d
BroadcastingConfigBootstrapper: test persistence of custom driver cre…
lukinovec fafd082
Fix typo in test
lukinovec c653c51
BroadcastingConfigBootstrapper: make tenant manager inherit central m…
lukinovec b1e91f1
BroadcastingConfigBootstrapper: make `Broadcaster::class` resolve to …
lukinovec 65beecf
BroadcastingConfigBootstrapper: clear the `Broadcast` facade's resolv…
lukinovec 0b860ea
Fix code style (php-cs-fixer)
github-actions[bot] c4f4451
Add assertions for the config of the bound manager's driver
lukinovec d939866
Fix custom creator assertions
lukinovec 28b6119
TenancyBroadcastManager: update docblocks
lukinovec 0fbe1bc
TenancyBroadcastManager: delete `Broadcaster` singleton binding
lukinovec b6c035c
Improve comments
lukinovec bbe2ff0
BroadcastingTest: update channel inheritance test
lukinovec b2add06
Delete BroadcastingTest
lukinovec 9e9bedc
Fix code style (php-cs-fixer)
github-actions[bot] 4b1cc9c
Improve comments
lukinovec fc45e09
Update BroadcastingConfigBootstrapperTest
lukinovec 6b99921
BroadcastingConfigBootstrapper: correct `$credentialsMap` array_merge…
lukinovec 29dd23d
BroadcastingConfigBootstrapperTest: add 'reverb' driver to datasets
lukinovec 4937a74
BroadcastingConfigBootstrapper and TenancyBroadcastManager: comments
lukinovec c831393
Update comment
lukinovec ef476c5
Polish comments
lukinovec f8528fc
Add 'reverb' to `TenancyBroadcastManager::$tenantBroadcasters`
lukinovec dc344b7
Merge branch 'master' into broadcasting-fixes
stancl 9ab0a72
Fix typo
lukinovec 4aeaa66
Remove unused `$broadcaster` parameter
lukinovec b3d5197
Correct channel prefix test comments
lukinovec a247bb0
Correct config bootstrapper test comment
lukinovec 20d494f
Reset `BroadcastingConfigBootstrapper::$broadcaster` in tests
lukinovec 7af2b8a
Improve BroadcastManager instance checks
lukinovec a3c5568
Add regression test for credentialsMap merge order
lukinovec f3652a8
Make `with()` formatting consistent
lukinovec a7acd07
Reset `BroadcastingConfigBootstrapper::$mapPresets` in tests
lukinovec 7db486d
Extract cleanup in broadcasting config test file
lukinovec d885659
Use toEqualCanonicalizing instead of toBe for custom creators
lukinovec e592b37
Merge branch 'master' into broadcasting-fixes
lukinovec ddd8c68
Improve comments
lukinovec 614344a
Add test for reverting the bound broadcaster when tenancy ends
lukinovec f20f801
Merge branch 'master' into broadcasting-fixes
lukinovec 9af1735
Explain why we swap the bound Broadcaster singleton better, remove th…
lukinovec 937c8c8
Delete the redundant BroadcastingFactory::class arg from the Broadcas…
lukinovec a19d3b5
Merge branch 'master' into broadcasting-fixes
lukinovec 24aeb23
Remove unused `$app` param and `?BroadcastManager` typehint (`singlet…
lukinovec 2a0464d
Resolve (and cache) tenant broadcasters once per tenancy initializati…
lukinovec d80fed1
Remove stale $tenantBroadcasters mention
lukinovec af14d40
Delete extra newline, fix comment inconsistency
lukinovec b8fddf8
Retrieve the re-registered channel closure to assert what tenant_chan…
lukinovec 128a1ad
Improve BroadcastingConfigBootstrapper docblock
lukinovec 9e6e0a9
Remove TenancyBroadcastManager, pass the channel auth closures direct…
lukinovec 5911619
Simplify channel-passing test
lukinovec 120cec4
Add spaces after `fn`/`function` for consistency
lukinovec 12e51d0
Test that when a tenant doesn't have the property that should be mapp…
lukinovec 0ba588d
Improve channel closure inheritance test
lukinovec e6c4c31
Test that using broadcasters that only implement the Broadcaster cont…
lukinovec 4422df2
Test configuring BroadcastingConfigBootstrapper::$broadcaster
lukinovec 1409d72
Make test name more specific
lukinovec 31d1663
Fix typo in comment
lukinovec File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -4,14 +4,19 @@ | |||||||||||||
|
|
||||||||||||||
| namespace Stancl\Tenancy\Bootstrappers; | ||||||||||||||
|
|
||||||||||||||
| use Illuminate\Broadcasting\Broadcasters\Broadcaster; | ||||||||||||||
| use Illuminate\Broadcasting\BroadcastManager; | ||||||||||||||
| use Illuminate\Config\Repository; | ||||||||||||||
| use Illuminate\Contracts\Broadcasting\Broadcaster; | ||||||||||||||
| 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; | ||||||||||||||
| use Stancl\Tenancy\Overrides\TenancyBroadcastManager; | ||||||||||||||
|
|
||||||||||||||
| /** | ||||||||||||||
| * Maps tenant credentials to the broadcasting config and rebinds BroadcastManager | ||||||||||||||
| * and Broadcaster so that broadcasters get resolved using the tenant credentials. | ||||||||||||||
| */ | ||||||||||||||
| class BroadcastingConfigBootstrapper implements TenancyBootstrapper | ||||||||||||||
| { | ||||||||||||||
| /** | ||||||||||||||
|
|
@@ -21,14 +26,16 @@ class BroadcastingConfigBootstrapper implements TenancyBootstrapper | |||||||||||||
| * [ | ||||||||||||||
| * 'config.key.name' => 'tenant_property', | ||||||||||||||
| * ] | ||||||||||||||
| * | ||||||||||||||
| * $tenant->tenant_property will be mapped to config('config.key.name') when tenancy is initialized. | ||||||||||||||
| */ | ||||||||||||||
| public static array $credentialsMap = []; | ||||||||||||||
|
|
||||||||||||||
| public static string|null $broadcaster = null; | ||||||||||||||
|
|
||||||||||||||
| protected array $originalConfig = []; | ||||||||||||||
| protected BroadcastManager|null $originalBroadcastManager = null; | ||||||||||||||
| protected Broadcaster|null $originalBroadcaster = null; | ||||||||||||||
| protected BroadcasterContract|null $originalBroadcaster = null; | ||||||||||||||
|
|
||||||||||||||
| public static array $mapPresets = [ | ||||||||||||||
| 'pusher' => [ | ||||||||||||||
|
|
@@ -54,27 +61,72 @@ public function __construct( | |||||||||||||
| protected Application $app | ||||||||||||||
| ) { | ||||||||||||||
| static::$broadcaster ??= $config->get('broadcasting.default'); | ||||||||||||||
| static::$credentialsMap = array_merge(static::$credentialsMap, static::$mapPresets[static::$broadcaster] ?? []); | ||||||||||||||
| static::$credentialsMap = array_merge(static::$mapPresets[static::$broadcaster] ?? [], static::$credentialsMap); | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| public function bootstrap(Tenant $tenant): void | ||||||||||||||
| { | ||||||||||||||
| $this->originalBroadcastManager = $this->app->make(BroadcastManager::class); | ||||||||||||||
| $this->originalBroadcaster = $this->app->make(Broadcaster::class); | ||||||||||||||
| $this->originalBroadcaster = $this->app->make(BroadcasterContract::class); | ||||||||||||||
|
|
||||||||||||||
| $this->setConfig($tenant); | ||||||||||||||
|
|
||||||||||||||
| // Make BroadcastManager resolve to a custom BroadcastManager which makes the broadcasters use the tenant credentials | ||||||||||||||
| $this->app->extend(BroadcastManager::class, function (BroadcastManager $broadcastManager) { | ||||||||||||||
| return new TenancyBroadcastManager($this->app); | ||||||||||||||
| // 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); | ||||||||||||||
|
|
||||||||||||||
| // 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; | ||||||||||||||
| }); | ||||||||||||||
|
|
||||||||||||||
| // 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. | ||||||||||||||
| $this->app->extend(BroadcasterContract::class, function (BroadcasterContract $centralBroadcaster) { | ||||||||||||||
| $tenantBroadcaster = $this->app->make(BroadcastManager::class)->connection(); | ||||||||||||||
|
|
||||||||||||||
| // The newly resolved broadcaster doesn't have any channel auth closures registered, so the | ||||||||||||||
| // closures registered in central context (e.g. in routes/channels.php) have to be passed to it | ||||||||||||||
| // manually, otherwise, Broadcast::auth() would throw a 403 for those channels. | ||||||||||||||
| // Since Laravel only ever uses the default broadcaster's channel auth closures for broadcasting auth, | ||||||||||||||
| // we only have to pass the channel closures to the default broadcaster. | ||||||||||||||
| // | ||||||||||||||
| // The $channels property and the channel() method aren't part of the Broadcaster contract -- they come | ||||||||||||||
| // from the abstract Broadcaster class, so the closures can only be copied between broadcasters extending it | ||||||||||||||
| // (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); | ||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's a |
||||||||||||||
|
|
||||||||||||||
| foreach ($centralBroadcaster->channels as $channel => $callback) { | ||||||||||||||
| $tenantBroadcaster->channel($channel, $callback, $centralBroadcaster->retrieveChannelOptions($channel)); | ||||||||||||||
| } | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| return $tenantBroadcaster; | ||||||||||||||
| }); | ||||||||||||||
|
|
||||||||||||||
| // 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(); | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| public function revert(): void | ||||||||||||||
| { | ||||||||||||||
| // Change the BroadcastManager and Broadcaster singletons back to what they were before initializing tenancy | ||||||||||||||
| $this->app->singleton(BroadcastManager::class, fn (Application $app) => $this->originalBroadcastManager); | ||||||||||||||
| $this->app->singleton(Broadcaster::class, fn (Application $app) => $this->originalBroadcaster); | ||||||||||||||
| // Revert the bound BroadcastManager and Broadcaster singletons back to their original state | ||||||||||||||
| $this->app->singleton(BroadcastManager::class, fn () => $this->originalBroadcastManager); | ||||||||||||||
| $this->app->singleton(BroadcasterContract::class, fn () => $this->originalBroadcaster); | ||||||||||||||
|
Comment on lines
+124
to
+126
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Would this be equivalent? |
||||||||||||||
|
|
||||||||||||||
| // Clear the resolved Broadcast facade instance so that it gets re-resolved as the central BroadcastManager | ||||||||||||||
| Broadcast::clearResolvedInstance(); | ||||||||||||||
|
|
||||||||||||||
| $this->unsetConfig(); | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
This file was deleted.
Oops, something went wrong.
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
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.