Skip to content
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

[8.x] Add setPusher() to PusherBroadcaster::class #37033

Merged
merged 1 commit into from
Apr 19, 2021
Merged

[8.x] Add setPusher() to PusherBroadcaster::class #37033

merged 1 commit into from
Apr 19, 2021

Conversation

vdauchy
Copy link
Contributor

@vdauchy vdauchy commented Apr 18, 2021

Hi,

This PR only add a setPusher() to the PusherBroadcaster class.

The reason is the pusher SDK instance is constructed from the configs using app_id, key and secret.

When we register the channels for broadcasting during the Application boot phase this is cached inside the BroadcastManager singleton.

In an application with many pusher configurations saved in database (ex: multitenant) those configs need to be switch in a middleware for each request.

Without a setter to change the Pusher SDK instance we need to hack using reflexion or forget the driver and then re-configure it.

Thanks :-)

@driesvints
Copy link
Member

Why don't you just instantiate a new PusherBroadcaster instance?

@GrahamCampbell
Copy link
Member

Yep, this seems like an antipattern to me.

@vdauchy
Copy link
Contributor Author

vdauchy commented Apr 19, 2021

Hi,

Ok let me describe the sequence:

  • Application boot:

    • Broadcast::channel(...) : Here we register the channels we need, this cause the PusherBroadcaster singleton to be instanciated with as states: the channels defined + the Pusher SDK configured from the config files
  • Request Handling:

    • Middleware: From the request we now know the real configuration for the Pusher SDK for that tenant and so we need to update its state in the PusherBroadcaster singleton.
    • Controller: We are now in the BroadcastController@authenticate controller (setted by Broadcast::routes()) and we need the proper Pusher SDK configuration to check the channels signature, etc...

@driesvints So my first solution was indeed to forget the driver (ie PusherBroadcaster) and re-instantiate it in the middleware with the proper config, but since it has the list of channels as state I loose them.

At the end and I really see only 2 solutions:

  • Pusher switch: Pusher SDK switch in the middleware using a setter (ie this PR).
  • Driver switch: Re-instantiate with the new configs but then I need to recover all other states and reinject them to the new PusherBroadcaster (trikky because Channels can be defines in different places, etc...).

@taylorotwell taylorotwell merged commit aa1f47e into laravel:8.x Apr 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants