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

[8.0] Rework HandlesOAuthErrors trait to middleware #937

Merged
merged 1 commit into from
Jan 18, 2019
Merged

[8.0] Rework HandlesOAuthErrors trait to middleware #937

merged 1 commit into from
Jan 18, 2019

Conversation

driesvints
Copy link
Member

The benefit this gives is that it allows developers to overwrite the middleware and implement their own handling of errors if they want to. At the moment they were stuck with the implementation being a trait so the only way to overwrite the implementation was to rebind their own controllers.

With the new implementation you could bind a new middleware which extends the current one like so:

$this->app->bind(HandleOAuthErrors::class, function () {
    // return your own implementation...
});

At least it'll become way easier for developers to customize this than before. It also makes much more sense to handle the error handling in a middleware than through a trait. Given, the previous implementation was probably just an old one and just needed a refresh.

Added tests for the new middleware and removed the old one. Also removed the tests for the controllers (which aren't needed anymore now).

Sent this in to master because of the obvious breaking changes.

Fixes #289

The benefit this gives is that it allows developers to overwrite the middleware and implement their own handling of errors if they want to.

Fixes #289
@taylorotwell
Copy link
Member

taylorotwell commented Jan 18, 2019

What will the upgrade path be for existing Passport users?

@taylorotwell taylorotwell merged commit 8c73aeb into laravel:master Jan 18, 2019
@driesvints driesvints deleted the rework-error-handling branch January 18, 2019 14:24
@TimWolla
Copy link
Contributor

TimWolla commented Aug 9, 2019

@driesvints Was this PR tested from within a Laravel app? We upgraded to dev-master passport within an Laravel 5.8 application to get PKCE support and noticed that OAuth errors result in a HTTP 500 now.

The issue being that Laravel itself catches Exceptions happening in the controller within the middleware pipeline (even in master of laravel/framework): https://github.com/laravel/framework/blob/9097786caae30e9ca243874e5c807609f1f2728e/src/Illuminate/Pipeline/Pipeline.php#L133, so they don't actually reach the HandleOAuthErrors middleware.

@driesvints
Copy link
Member Author

@TimWolla hmm that's unfortunate. I don't have time myself to look at this atm so feel free to open up an issue for this.

@TimWolla
Copy link
Contributor

TimWolla commented Aug 9, 2019

@driesvints ack. I've filed #1062 to track this issue.

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