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

[5.7] Do not pass the guard instance to the authentication events #25568

Merged
merged 1 commit into from
Sep 11, 2018
Merged

[5.7] Do not pass the guard instance to the authentication events #25568

merged 1 commit into from
Sep 11, 2018

Conversation

rodrigopedra
Copy link
Contributor

PR #25261 breaks usage of queued listeners on authentication events as it passes the Guard instance which can contain closure based inner properties thus is not serializable

This PR changes the behavior to pass only the guard name to the events constructors so:

  1. The intended feature fro PR [5.7] Add guard to authentication events #25261 is still present
  2. Any queued listeners can be serialized

I am working in the tests right now to prevent future issues

@rodrigopedra rodrigopedra changed the title [5.7] [WIP] Do not pass the guard instance to the authentication events [5.7] Do not pass the guard instance to the authentication events Sep 10, 2018
@rodrigopedra
Copy link
Contributor Author

Changed the tests to ensure the guards are passed as string

@deleugpn
Copy link
Contributor

Although it feels like a breaking change, this is more of a feature fix when queuing.
I will try to work on an integration test that makes sure to never allow closure on queueable objects.

@taylorotwell taylorotwell merged commit e5216cd into laravel:5.7 Sep 11, 2018
nhobi added a commit to nhobi/laravel-aws-cognito-auth that referenced this pull request Dec 19, 2018
@frigginglorious
Copy link

Is there a reason why this was left out of the Upgrade guide at https://laravel.com/docs/5.7/upgrade ?

@deleugpn
Copy link
Contributor

If memory serves me right this was introduced at the end of 5.6 and reverted at the first week of 5.7 or so. I guess that's why it didn't make the list because it was a revert of a barely released feature.

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