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

Use the hub injected in the constructor of the listeners rather than retrieving it from the SDK singleton #387

Conversation

ste93cry
Copy link
Collaborator

This PR refactorizes the event listeners to make them use the Hub injected into the constructor rather than getting it using the global functions. This eases the unit testing of those classes. Note that by default the injected hub is under the hood an instance of HubAdapter and thus it works exactly as before by proxing each call to the current hub that is retrieved on-the-fly

@ste93cry ste93cry added this to the 4.0 milestone Nov 17, 2020
@ste93cry ste93cry requested a review from Jean85 November 17, 2020 13:27
@ste93cry ste93cry force-pushed the fix/use-injected-hub-instead-of-global-functions branch 3 times, most recently from 26fb461 to acaf249 Compare November 17, 2020 20:22
@ste93cry
Copy link
Collaborator Author

@Jean85 do we really need to make the priority of the listeners configurable? I know it's a really small feature and doesn't have a big impact in the maintenance of the code, but I don't think it's useful since people can simply adjust the priority of their listeners

@Jean85
Copy link
Collaborator

Jean85 commented Nov 18, 2020

Listeners need to run after certain stuff, making them configurable makes the installation a lot easier. Requiring users to change their code to install ours is not fine IMHO.

What issues do you have with those?

@ste93cry
Copy link
Collaborator Author

ste93cry commented Nov 18, 2020

No issue at all, I was simply questioning because I never heard anyone needing this and also trying to look around GitHub using the search feature to see who does the same I didn't find many results. I'm totally fine with leaving this as-is if you think it's worth keeping the feature though, I have some doubts about the usefulness but it's just my opinion

@Jean85
Copy link
Collaborator

Jean85 commented Nov 18, 2020

The issue was requested in #49 the first time, so there's definitely a need.

@ste93cry
Copy link
Collaborator Author

Looking at that PR I think that the main issue was that the listeners at that time were all using the default priority, so people could not inject their own listeners between the Symfony ones (if there was any) and the Sentry ones. You decided to solve the issue by making the priority configurable, which is a way to solve the issue. The other way (it would had required a new major version of course to avoid breaking BC) was to explicitly set the priority. Both ways are fine, as I said I don't really have any issue with keeping things as they are now 😃

@ste93cry ste93cry force-pushed the fix/use-injected-hub-instead-of-global-functions branch 5 times, most recently from 5fa5ed8 to a0ac4d2 Compare November 23, 2020 21:34
@ste93cry ste93cry force-pushed the fix/use-injected-hub-instead-of-global-functions branch from a0ac4d2 to 88c6f75 Compare November 23, 2020 21:39
@ste93cry ste93cry marked this pull request as ready for review November 23, 2020 21:42
phpstan-baseline.neon Show resolved Hide resolved
phpstan.neon Show resolved Hide resolved
src/EventListener/ConsoleCommandListener.php Outdated Show resolved Hide resolved
src/EventListener/RequestListener.php Show resolved Hide resolved
src/EventListener/SubRequestListener.php Show resolved Hide resolved
@ste93cry ste93cry force-pushed the fix/use-injected-hub-instead-of-global-functions branch from 09b0961 to 285f3eb Compare November 24, 2020 13:19
@ste93cry ste93cry force-pushed the fix/use-injected-hub-instead-of-global-functions branch from 285f3eb to 1e23790 Compare November 24, 2020 13:22
@ste93cry ste93cry requested a review from Jean85 November 24, 2020 13:24
@Jean85 Jean85 merged commit 03335ef into getsentry:master Nov 25, 2020
@ste93cry ste93cry deleted the fix/use-injected-hub-instead-of-global-functions branch November 25, 2020 20:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants