search

laravel / reverb

Laravel Reverb provides a real-time WebSocket communication backend for Laravel applications.
https://reverb.laravel.com
MIT License
1.07k stars 82 forks source link

[1.x] Retrieves applications from provider in Pulse connections recorder #194

Closed colq2 closed 4 months ago

colq2 commented 4 months ago

This PR updates how the connections recorder for Pulse retrieves applications. It now retrieves them from the registered ApplicationProvider. This extends the Application constructor with the options array.

Currently, the connections recorder does not work with providers other than the config provider. Another potential provider could be a database provider.

Let me know if you are willing to accept this kind of PR. I am happy to add test cases for this or update the PR.

joedixon commented 4 months ago

@colq2 I think this makes sense so would be happy to accept a PR here, but looks like tests are failing at the moment.

Also, just wanted to confirm adding the options array should be addressed regardless of this PR? You just happen to be resolving that issue at the same time?

driesvints commented 4 months ago

Tests fail here. Make sure to fix them before marking as ready.

colq2 commented 4 months ago

@joedixon thanks, I will address the failing tests and update the PR.

The options array is mandatory for this to work. Without the options property there is no way to get the options for the pusher client. Definitely not trying to squeeze two different things into same pr.

joedixon commented 4 months ago

Gotcha! I actually didn't mean to suggest you were squeezing two for one. It just seemed to me from reading your PR that the options array is missing from the current "config only" implementation.

colq2 commented 4 months ago

@joedixon no worries, I have to work on my description next time 🤓

Fixing the failing tests was pretty simple, just didn't have the time to resolve it earlier. I added a simple test, which I am not sure about. Feels like it tests too much and not enough. What do you think should be tested here?

joedixon commented 4 months ago

Really nice addition, thank you!