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

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

Merged
merged 9 commits into from
May 27, 2024

Conversation

colq2
Copy link
Contributor

@colq2 colq2 commented May 10, 2024

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.

@taylorotwell taylorotwell requested a review from joedixon May 10, 2024 19:40
@taylorotwell taylorotwell marked this pull request as draft May 10, 2024 19:40
@joedixon
Copy link
Collaborator

@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
Copy link
Member

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

@colq2
Copy link
Contributor Author

colq2 commented May 15, 2024

@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
Copy link
Collaborator

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
Copy link
Contributor Author

colq2 commented May 17, 2024

@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 joedixon changed the title Retrieve applications from provider in pulse connections recorder [1.x] Retrieves applications from provider in Pulse connections recorder May 24, 2024
@joedixon
Copy link
Collaborator

Really nice addition, thank you!

@joedixon joedixon marked this pull request as ready for review May 24, 2024 11:13
@taylorotwell taylorotwell merged commit 513fc42 into laravel:main May 27, 2024
9 checks passed
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