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

[6.x] Improved AuthServiceProvider::registerEventRebindHandler() #30105

Merged
merged 2 commits into from
Sep 26, 2019

Conversation

duxthefux
Copy link
Contributor

@duxthefux duxthefux commented Sep 25, 2019

  • as Auth::guard() not only checks for an existing guard but also
    initializes a new guard in case ob absence. This can lead to side-effects
    when overbinding 'events' in another service provider
  • this MR fixes initializing a guard while actually still being in register-phase of service providers. Please note that this only happens if you events if overbind with a library like https://github.com/fntneves/laravel-transactional-events. Then it leads to actually always creating a guard although it's might not needed.

Please note that this is my first MR and I'm happy to hear feedback and adjust it.

Thanks a lot!

@duxthefux duxthefux force-pushed the improve-event-rebind-handler branch 2 times, most recently from be1fc94 to 89ad1c1 Compare September 25, 2019 19:01
@driesvints driesvints changed the title Improved AuthServiceProvider::registerEventRebindHandler() [6.x] Improved AuthServiceProvider::registerEventRebindHandler() Sep 26, 2019
@driesvints
Copy link
Member

@duxthefux I fixed the build on the 6.x branch. Can you rebase? The build will pass then.

* as Auth::guard() not only checks for an existing guard but also
initializes a new guard in case ob absence. This can lead to side-effects
when overbinding 'events' in another service provider
@duxthefux
Copy link
Contributor Author

@driesvints Thx a lot! I force pushed. Looks good now!

@taylorotwell taylorotwell merged commit c109bdd into laravel:6.x Sep 26, 2019
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.

3 participants