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

Add redirect route configuration too allow setting of redirect location when triggered by middleware #63

Merged
merged 16 commits into from Sep 26, 2020

Conversation

joearcher
Copy link
Contributor

I've added a config var for a route name and amended the middlewares to redirect to the configured route, defaulting to login.

Add route key to config
add route to redirect
add route to log out middleware
@antonkomarev
Copy link
Member

Thank you for the contribution @joearcher!
This is breaking changes and I'm not sure we should release major version with this changes only.
Maybe it would be better to have null as default config value and if so - make it work as it was working right now.

@antonkomarev
Copy link
Member

antonkomarev commented Sep 25, 2020

Another one point that not all apps has route named login by default and it will crash application. It might be more safe and transparent for the user if it will be redirect url, not route.

Then people might reconfigure it like this if they want to:

'redirect_url' => route('login'),

@joearcher
Copy link
Contributor Author

Both of your points make perfect sense, I will amend this to default to a null config value fall back to current behaviour and use a url for the redirect where set.

Copy link
Member

@antonkomarev antonkomarev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

StyleCI found issues in code style. Fix them to make it green, please.

config/ban.php Outdated Show resolved Hide resolved
src/Http/Middleware/ForbidBannedUser.php Outdated Show resolved Hide resolved
src/Http/Middleware/ForbidBannedUser.php Outdated Show resolved Hide resolved
src/Http/Middleware/ForbidBannedUser.php Outdated Show resolved Hide resolved
config/ban.php Outdated Show resolved Hide resolved
@antonkomarev
Copy link
Member

Thank you for polishing it up! Good job!

@antonkomarev antonkomarev merged commit 535424b into cybercog:master Sep 26, 2020
@antonkomarev
Copy link
Member

Will try to make a release today evening.

@joearcher
Copy link
Contributor Author

Happy to help! Thanks for all of your input and patience.

@antonkomarev
Copy link
Member

Laravel Ban v4.4.0 has been released.

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