Laravel Middleware
up vote
1
down vote
favorite
I've got a middleware that all my routes are grouped by, I'm doing a lot in 1 middleware. I was wondering if anyone could tell me how to better structure my Laravel application.
I couldn't find any other way to run checks before every page load, or even where was the best place to do it, and the best way to do it.
In this middleware I'm logging each entry to the database, checking a lot of DB related code and redirecting if something applies, and running a few other checks.
I'm just trying to overall improve it. Can anyone tell me where the weak points are or any suggestions on making this better?
class Platform
{
public function handle($request, Closure $next)
{
$agent = new Agent();
$entry = new EntryLog;
$entry->address_accessed = "$_SERVER[HTTP_HOST]$_SERVER[REQUEST_URI]";
$entry->request_ip = $request->ip();
$entry->request_device = $agent->isDesktop() ? 'Desktop' : ($agent->isMobile() ? 'Mobile' : 'Tablet');
$entry->request_system = $agent->platform() . ' ' . $agent->version($agent->platform());
$entry->request_browser = $agent->browser();
$entry->request_method = $request->method();
$entry->save();
$currentRoute = Route::current()->getName();
$dontCheck = array(
"platform.contact",
"frontend.user.account.logout"
);
if (!in_array($currentRoute, $dontCheck)) {
if (!Auth::check()) {
if (UserBan::where('ban_type', 'ip_ban')->where('ban_value', $request->ip())->whereRaw('expires_at > now()')->first() != null && $currentRoute != "platform.banned") {
return redirect()->route('platform.banned');
}
}
else {
$routeLock = RouteLock::where('route_name', $currentRoute)->first();
if ($routeLock != null && $routeLock->expires_at > time()) {
if (strlen($routeLock->required_permissions) > 0 && !Auth::user()->hasAnyPermissions($routeLock->required_permissions)) {
return redirect()->route('platform.restricted');
}
}
else if (Auth::user()->isBanned($request) && $currentRoute != "platform.banned") {
return redirect()->route('platform.banned');
}
else if (!Auth::user()->roleplayExists() && $currentRoute != "frontend.user.error") {
return redirect()->route('frontend.user.error');
}
else if (PlatformSetting::find('platform.website.frontend_maintenance_enabled')->value == 1 && $currentRoute != "platform.maintenance" && !Auth::user()->canBypassMaintenance()) {
return redirect()->route('platform.maintenance');
}
else if ($currentRoute == 'frontend.guest.register.begin' && PlatformSetting::findSetting('website.registration.enabled') == '1') {
return 'registration is currently closed.';
}
else if ($currentRoute == 'frontend.user.play') {
$permissions = PlatformSetting::findSetting('website.client.required_permissions');
if (strlen($permissions) > 0) {
if (!Auth::user()->hasAnyPermissions($permissions)) {
return new Response(view('platform.restricted'));
}
}
}
else if (Auth::user()->website_setup_finished == '0' && $currentRoute !='frontend.user.setup.step_' . Auth::user()->website_last_step) {
return redirect()->route('frontend.user.setup.step_' . Auth::user()->website_last_step);
}
else if (Auth::user()->is_locked == '1' && $currentRoute != 'frontend.user.locked' && $currentRoute != 'frontend.user.account.unlock') {
return redirect()->route('frontend.user.locked');
}
else if (Auth::user()->pin_lock == '1' && $currentRoute != 'frontend.user.pin') {
return redirect()->route('frontend.user.pin');
}
else if (PlatformSetting::find('platform.website.frontend_maintenance_enabled')->value == 1 && $currentRoute != "platform.maintenance" && !Auth::user()->canBypassMaintenance()) {
return redirect()->route('platform.maintenance');
}
else {
$platformState = PlatformSetting::find('platform.state')->value;
if (($platformState == 1 || $platformState == 2 || $platformState == 3) && !Auth::user()->hasBeta()) {
return redirect()->route('frontend.user.beta');
}
else {
$allowedPages = PlatformSetting::findSetting('website.allowed_pages');
if (strlen($allowedPages) > 0) {
if (!in_array($currentRoute, explode(',', $allowedPages))) {
return new Response(view('platform.restricted'));
}
}
}
}
}
}
return $next($request);
}
}
php laravel
edited Sep 30 '17 at 20:57
Jamal♦
30.2k11115226
asked Sep 30 '17 at 2:30
Jane Edwards
61
bumped to the homepage by Community♦ 2 days ago
This question has answers that may be good or bad; the system has marked it active so that they can be reviewed.
add a comment |
up vote
1
down vote
favorite
I've got a middleware that all my routes are grouped by, I'm doing a lot in 1 middleware. I was wondering if anyone could tell me how to better structure my Laravel application.
I couldn't find any other way to run checks before every page load, or even where was the best place to do it, and the best way to do it.
In this middleware I'm logging each entry to the database, checking a lot of DB related code and redirecting if something applies, and running a few other checks.
I'm just trying to overall improve it. Can anyone tell me where the weak points are or any suggestions on making this better?
class Platform
{
public function handle($request, Closure $next)
{
$agent = new Agent();
$entry = new EntryLog;
$entry->address_accessed = "$_SERVER[HTTP_HOST]$_SERVER[REQUEST_URI]";
$entry->request_ip = $request->ip();
$entry->request_device = $agent->isDesktop() ? 'Desktop' : ($agent->isMobile() ? 'Mobile' : 'Tablet');
$entry->request_system = $agent->platform() . ' ' . $agent->version($agent->platform());
$entry->request_browser = $agent->browser();
$entry->request_method = $request->method();
$entry->save();
$currentRoute = Route::current()->getName();
$dontCheck = array(
"platform.contact",
"frontend.user.account.logout"
);
if (!in_array($currentRoute, $dontCheck)) {
if (!Auth::check()) {
if (UserBan::where('ban_type', 'ip_ban')->where('ban_value', $request->ip())->whereRaw('expires_at > now()')->first() != null && $currentRoute != "platform.banned") {
return redirect()->route('platform.banned');
}
}
else {
$routeLock = RouteLock::where('route_name', $currentRoute)->first();
if ($routeLock != null && $routeLock->expires_at > time()) {
if (strlen($routeLock->required_permissions) > 0 && !Auth::user()->hasAnyPermissions($routeLock->required_permissions)) {
return redirect()->route('platform.restricted');
}
}
else if (Auth::user()->isBanned($request) && $currentRoute != "platform.banned") {
return redirect()->route('platform.banned');
}
else if (!Auth::user()->roleplayExists() && $currentRoute != "frontend.user.error") {
return redirect()->route('frontend.user.error');
}
else if (PlatformSetting::find('platform.website.frontend_maintenance_enabled')->value == 1 && $currentRoute != "platform.maintenance" && !Auth::user()->canBypassMaintenance()) {
return redirect()->route('platform.maintenance');
}
else if ($currentRoute == 'frontend.guest.register.begin' && PlatformSetting::findSetting('website.registration.enabled') == '1') {
return 'registration is currently closed.';
}
else if ($currentRoute == 'frontend.user.play') {
$permissions = PlatformSetting::findSetting('website.client.required_permissions');
if (strlen($permissions) > 0) {
if (!Auth::user()->hasAnyPermissions($permissions)) {
return new Response(view('platform.restricted'));
}
}
}
else if (Auth::user()->website_setup_finished == '0' && $currentRoute !='frontend.user.setup.step_' . Auth::user()->website_last_step) {
return redirect()->route('frontend.user.setup.step_' . Auth::user()->website_last_step);
}
else if (Auth::user()->is_locked == '1' && $currentRoute != 'frontend.user.locked' && $currentRoute != 'frontend.user.account.unlock') {
return redirect()->route('frontend.user.locked');
}
else if (Auth::user()->pin_lock == '1' && $currentRoute != 'frontend.user.pin') {
return redirect()->route('frontend.user.pin');
}
else if (PlatformSetting::find('platform.website.frontend_maintenance_enabled')->value == 1 && $currentRoute != "platform.maintenance" && !Auth::user()->canBypassMaintenance()) {
return redirect()->route('platform.maintenance');
}
else {
$platformState = PlatformSetting::find('platform.state')->value;
if (($platformState == 1 || $platformState == 2 || $platformState == 3) && !Auth::user()->hasBeta()) {
return redirect()->route('frontend.user.beta');
}
else {
$allowedPages = PlatformSetting::findSetting('website.allowed_pages');
if (strlen($allowedPages) > 0) {
if (!in_array($currentRoute, explode(',', $allowedPages))) {
return new Response(view('platform.restricted'));
}
}
}
}
}
}
return $next($request);
}
}
php laravel
edited Sep 30 '17 at 20:57
Jamal♦
30.2k11115226
asked Sep 30 '17 at 2:30
Jane Edwards
61
bumped to the homepage by Community♦ 2 days ago
This question has answers that may be good or bad; the system has marked it active so that they can be reviewed.
add a comment |
up vote
1
down vote
favorite
up vote
1
down vote
favorite
I've got a middleware that all my routes are grouped by, I'm doing a lot in 1 middleware. I was wondering if anyone could tell me how to better structure my Laravel application.
I couldn't find any other way to run checks before every page load, or even where was the best place to do it, and the best way to do it.
In this middleware I'm logging each entry to the database, checking a lot of DB related code and redirecting if something applies, and running a few other checks.
I'm just trying to overall improve it. Can anyone tell me where the weak points are or any suggestions on making this better?
class Platform
{
public function handle($request, Closure $next)
{
$agent = new Agent();
$entry = new EntryLog;
$entry->address_accessed = "$_SERVER[HTTP_HOST]$_SERVER[REQUEST_URI]";
$entry->request_ip = $request->ip();
$entry->request_device = $agent->isDesktop() ? 'Desktop' : ($agent->isMobile() ? 'Mobile' : 'Tablet');
$entry->request_system = $agent->platform() . ' ' . $agent->version($agent->platform());
$entry->request_browser = $agent->browser();
$entry->request_method = $request->method();
$entry->save();
$currentRoute = Route::current()->getName();
$dontCheck = array(
"platform.contact",
"frontend.user.account.logout"
);
if (!in_array($currentRoute, $dontCheck)) {
if (!Auth::check()) {
if (UserBan::where('ban_type', 'ip_ban')->where('ban_value', $request->ip())->whereRaw('expires_at > now()')->first() != null && $currentRoute != "platform.banned") {
return redirect()->route('platform.banned');
}
}
else {
$routeLock = RouteLock::where('route_name', $currentRoute)->first();
if ($routeLock != null && $routeLock->expires_at > time()) {
if (strlen($routeLock->required_permissions) > 0 && !Auth::user()->hasAnyPermissions($routeLock->required_permissions)) {
return redirect()->route('platform.restricted');
}
}
else if (Auth::user()->isBanned($request) && $currentRoute != "platform.banned") {
return redirect()->route('platform.banned');
}
else if (!Auth::user()->roleplayExists() && $currentRoute != "frontend.user.error") {
return redirect()->route('frontend.user.error');
}
else if (PlatformSetting::find('platform.website.frontend_maintenance_enabled')->value == 1 && $currentRoute != "platform.maintenance" && !Auth::user()->canBypassMaintenance()) {
return redirect()->route('platform.maintenance');
}
else if ($currentRoute == 'frontend.guest.register.begin' && PlatformSetting::findSetting('website.registration.enabled') == '1') {
return 'registration is currently closed.';
}
else if ($currentRoute == 'frontend.user.play') {
$permissions = PlatformSetting::findSetting('website.client.required_permissions');
if (strlen($permissions) > 0) {
if (!Auth::user()->hasAnyPermissions($permissions)) {
return new Response(view('platform.restricted'));
}
}
}
else if (Auth::user()->website_setup_finished == '0' && $currentRoute !='frontend.user.setup.step_' . Auth::user()->website_last_step) {
return redirect()->route('frontend.user.setup.step_' . Auth::user()->website_last_step);
}
else if (Auth::user()->is_locked == '1' && $currentRoute != 'frontend.user.locked' && $currentRoute != 'frontend.user.account.unlock') {
return redirect()->route('frontend.user.locked');
}
else if (Auth::user()->pin_lock == '1' && $currentRoute != 'frontend.user.pin') {
return redirect()->route('frontend.user.pin');
}
else if (PlatformSetting::find('platform.website.frontend_maintenance_enabled')->value == 1 && $currentRoute != "platform.maintenance" && !Auth::user()->canBypassMaintenance()) {
return redirect()->route('platform.maintenance');
}
else {
$platformState = PlatformSetting::find('platform.state')->value;
if (($platformState == 1 || $platformState == 2 || $platformState == 3) && !Auth::user()->hasBeta()) {
return redirect()->route('frontend.user.beta');
}
else {
$allowedPages = PlatformSetting::findSetting('website.allowed_pages');
if (strlen($allowedPages) > 0) {
if (!in_array($currentRoute, explode(',', $allowedPages))) {
return new Response(view('platform.restricted'));
}
}
}
}
}
}
return $next($request);
}
}
php laravel
edited Sep 30 '17 at 20:57
Jamal♦
30.2k11115226
asked Sep 30 '17 at 2:30
Jane Edwards
61
I've got a middleware that all my routes are grouped by, I'm doing a lot in 1 middleware. I was wondering if anyone could tell me how to better structure my Laravel application.
I couldn't find any other way to run checks before every page load, or even where was the best place to do it, and the best way to do it.
In this middleware I'm logging each entry to the database, checking a lot of DB related code and redirecting if something applies, and running a few other checks.
I'm just trying to overall improve it. Can anyone tell me where the weak points are or any suggestions on making this better?
class Platform
{
public function handle($request, Closure $next)
{
$agent = new Agent();
$entry = new EntryLog;
$entry->address_accessed = "$_SERVER[HTTP_HOST]$_SERVER[REQUEST_URI]";
$entry->request_ip = $request->ip();
$entry->request_device = $agent->isDesktop() ? 'Desktop' : ($agent->isMobile() ? 'Mobile' : 'Tablet');
$entry->request_system = $agent->platform() . ' ' . $agent->version($agent->platform());
$entry->request_browser = $agent->browser();
$entry->request_method = $request->method();
$entry->save();
$currentRoute = Route::current()->getName();
$dontCheck = array(
"platform.contact",
"frontend.user.account.logout"
);
if (!in_array($currentRoute, $dontCheck)) {
if (!Auth::check()) {
if (UserBan::where('ban_type', 'ip_ban')->where('ban_value', $request->ip())->whereRaw('expires_at > now()')->first() != null && $currentRoute != "platform.banned") {
return redirect()->route('platform.banned');
}
}
else {
$routeLock = RouteLock::where('route_name', $currentRoute)->first();
if ($routeLock != null && $routeLock->expires_at > time()) {
if (strlen($routeLock->required_permissions) > 0 && !Auth::user()->hasAnyPermissions($routeLock->required_permissions)) {
return redirect()->route('platform.restricted');
}
}
else if (Auth::user()->isBanned($request) && $currentRoute != "platform.banned") {
return redirect()->route('platform.banned');
}
else if (!Auth::user()->roleplayExists() && $currentRoute != "frontend.user.error") {
return redirect()->route('frontend.user.error');
}
else if (PlatformSetting::find('platform.website.frontend_maintenance_enabled')->value == 1 && $currentRoute != "platform.maintenance" && !Auth::user()->canBypassMaintenance()) {
return redirect()->route('platform.maintenance');
}
else if ($currentRoute == 'frontend.guest.register.begin' && PlatformSetting::findSetting('website.registration.enabled') == '1') {
return 'registration is currently closed.';
}
else if ($currentRoute == 'frontend.user.play') {
$permissions = PlatformSetting::findSetting('website.client.required_permissions');
if (strlen($permissions) > 0) {
if (!Auth::user()->hasAnyPermissions($permissions)) {
return new Response(view('platform.restricted'));
}
}
}
else if (Auth::user()->website_setup_finished == '0' && $currentRoute !='frontend.user.setup.step_' . Auth::user()->website_last_step) {
return redirect()->route('frontend.user.setup.step_' . Auth::user()->website_last_step);
}
else if (Auth::user()->is_locked == '1' && $currentRoute != 'frontend.user.locked' && $currentRoute != 'frontend.user.account.unlock') {
return redirect()->route('frontend.user.locked');
}
else if (Auth::user()->pin_lock == '1' && $currentRoute != 'frontend.user.pin') {
return redirect()->route('frontend.user.pin');
}
else if (PlatformSetting::find('platform.website.frontend_maintenance_enabled')->value == 1 && $currentRoute != "platform.maintenance" && !Auth::user()->canBypassMaintenance()) {
return redirect()->route('platform.maintenance');
}
else {
$platformState = PlatformSetting::find('platform.state')->value;
if (($platformState == 1 || $platformState == 2 || $platformState == 3) && !Auth::user()->hasBeta()) {
return redirect()->route('frontend.user.beta');
}
else {
$allowedPages = PlatformSetting::findSetting('website.allowed_pages');
if (strlen($allowedPages) > 0) {
if (!in_array($currentRoute, explode(',', $allowedPages))) {
return new Response(view('platform.restricted'));
}
}
}
}
}
}
return $next($request);
}
}
php laravel
php laravel
edited Sep 30 '17 at 20:57
Jamal♦
30.2k11115226
asked Sep 30 '17 at 2:30
Jane Edwards
61
edited Sep 30 '17 at 20:57
Jamal♦
30.2k11115226
asked Sep 30 '17 at 2:30
Jane Edwards
61
edited Sep 30 '17 at 20:57
Jamal♦
30.2k11115226
edited Sep 30 '17 at 20:57
Jamal♦
30.2k11115226
edited Sep 30 '17 at 20:57
Jamal♦
30.2k11115226
30.2k11115226
asked Sep 30 '17 at 2:30
Jane Edwards
61
asked Sep 30 '17 at 2:30
Jane Edwards
61
asked Sep 30 '17 at 2:30
Jane Edwards
61
61
bumped to the homepage by Community♦ 2 days ago
This question has answers that may be good or bad; the system has marked it active so that they can be reviewed.
bumped to the homepage by Community♦ 2 days ago
This question has answers that may be good or bad; the system has marked it active so that they can be reviewed.
add a comment |
add a comment |
2 Answers
2
active
oldest
votes
up vote
0
down vote
Well, I am not an expert with PHP, but I can point out one or two things in this code.
Keep the page and views names as constants in one place. Do not use strings to redirect, but use refs to these constants. This way you have to change it from one place if/when you rename a page/view
Use a function to invoke for redirect which will check the currentRoute. Do not check this in each case/else if
Collect the vars you want to use in advance. Do not ask Auth::user()-> on and on. This is totaly OK but for better readability you could -and should- spare a few nsecs.
answered Sep 30 '17 at 20:16
JohnPan
1862
add a comment |
up vote
0
down vote
You have a lot going on in one middleware. Keep in mind a middleware runs for each request so it needs to be reasonably well optimised.
I have split the logging bit out into PlatformLogger class, that seemed like a logical separation.
I have also removed a lot of the } else {, to me they just add noise and in most cases you return anyway, so the else will never get triggered.
PLEASE NOTE: The conditions below where I removed the else's may not all be the equivalent to the sample provided, it is just to show you what it would look like, you would need to check and make those changes yourself if you choose to do it that way.
Also check your if conditions, and do the cheapest test first as they might short circuit the if statement and you won't have to perform the other slower test that involve database access for example.
if (PlatformSetting::find('platform.website.frontend_maintenance_enabled')->value == 1 && $currentRoute != "platform.maintenance" && !Auth::user()->canBypassMaintenance()) {
vs
if ($currentRoute != "platform.maintenance" && PlatformSetting::find('platform.website.frontend_maintenance_enabled')->value == 1 && !Auth::user()->canBypassMaintenance()) {
Example changes
<?php
class PlatformLogger
{
public function handle($request, Closure $next) {
$agent = new Agent();
$entry = new EntryLog;
$entry->address_accessed = "$_SERVER[HTTP_HOST]$_SERVER[REQUEST_URI]";
$entry->request_ip = $request->ip();
$entry->request_device = $agent->isDesktop() ? 'Desktop' : ($agent->isMobile() ? 'Mobile' : 'Tablet');
$entry->request_system = $agent->platform() . ' ' . $agent->version($agent->platform());
$entry->request_browser = $agent->browser();
$entry->request_method = $request->method();
$entry->save();
return $next($request);
}
}
class Platform
{
public function handle($request, Closure $next)
{
$agent = new Agent();
$currentRoute = Route::current()->getName();
$dontCheck = array(
"platform.contact",
"frontend.user.account.logout"
);
if (in_array($currentRoute, $dontCheck)) {
return $next($request);
}
if (!Auth::check()) {
if (UserBan::where('ban_type', 'ip_ban')->where('ban_value', $request->ip())->whereRaw('expires_at > now()')->first() != null && $currentRoute != "platform.banned") {
return redirect()->route('platform.banned');
}
return $next($request);
}
$routeLock = RouteLock::where('route_name', $currentRoute)->first();
if ($routeLock != null && $routeLock->expires_at > time()) {
if (strlen($routeLock->required_permissions) > 0 && !Auth::user()->hasAnyPermissions($routeLock->required_permissions)) {
return redirect()->route('platform.restricted');
}
}
if (Auth::user()->isBanned($request) && $currentRoute != "platform.banned") {
return redirect()->route('platform.banned');
}
if (!Auth::user()->roleplayExists() && $currentRoute != "frontend.user.error") {
return redirect()->route('frontend.user.error');
}
if (PlatformSetting::find('platform.website.frontend_maintenance_enabled')->value == 1 && $currentRoute != "platform.maintenance" && !Auth::user()->canBypassMaintenance()) {
return redirect()->route('platform.maintenance');
}
if ($currentRoute == 'frontend.guest.register.begin' && PlatformSetting::findSetting('website.registration.enabled') == '1') {
return 'registration is currently closed.';
}
if ($currentRoute == 'frontend.user.play') {
$permissions = PlatformSetting::findSetting('website.client.required_permissions');
if (strlen($permissions) > 0) {
if (!Auth::user()->hasAnyPermissions($permissions)) {
return new Response(view('platform.restricted'));
}
}
}
if (Auth::user()->website_setup_finished == '0' && $currentRoute !='frontend.user.setup.step_' . Auth::user()->website_last_step) {
return redirect()->route('frontend.user.setup.step_' . Auth::user()->website_last_step);
}
if (Auth::user()->is_locked == '1' && $currentRoute != 'frontend.user.locked' && $currentRoute != 'frontend.user.account.unlock') {
return redirect()->route('frontend.user.locked');
}
if (Auth::user()->pin_lock == '1' && $currentRoute != 'frontend.user.pin') {
return redirect()->route('frontend.user.pin');
}
if (PlatformSetting::find('platform.website.frontend_maintenance_enabled')->value == 1 && $currentRoute != "platform.maintenance" && !Auth::user()->canBypassMaintenance()) {
return redirect()->route('platform.maintenance');
}
$platformState = PlatformSetting::find('platform.state')->value;
if (($platformState == 1 || $platformState == 2 || $platformState == 3) && !Auth::user()->hasBeta()) {
return redirect()->route('frontend.user.beta');
}
else {
$allowedPages = PlatformSetting::findSetting('website.allowed_pages');
if (strlen($allowedPages) > 0) {
if (!in_array($currentRoute, explode(',', $allowedPages))) {
return new Response(view('platform.restricted'));
}
}
}
return $next($request);
}
}
answered Oct 31 '17 at 0:38
bumperbox
1,843615
add a comment |
2 Answers
2
active
oldest
votes
2 Answers
2
active
oldest
votes
active
oldest
votes
active
oldest
votes
up vote
0
down vote
Well, I am not an expert with PHP, but I can point out one or two things in this code.
Keep the page and views names as constants in one place. Do not use strings to redirect, but use refs to these constants. This way you have to change it from one place if/when you rename a page/view
Use a function to invoke for redirect which will check the currentRoute. Do not check this in each case/else if
Collect the vars you want to use in advance. Do not ask Auth::user()-> on and on. This is totaly OK but for better readability you could -and should- spare a few nsecs.
answered Sep 30 '17 at 20:16
JohnPan
1862
add a comment |
up vote
0
down vote
Well, I am not an expert with PHP, but I can point out one or two things in this code.
Keep the page and views names as constants in one place. Do not use strings to redirect, but use refs to these constants. This way you have to change it from one place if/when you rename a page/view
Use a function to invoke for redirect which will check the currentRoute. Do not check this in each case/else if
Collect the vars you want to use in advance. Do not ask Auth::user()-> on and on. This is totaly OK but for better readability you could -and should- spare a few nsecs.
answered Sep 30 '17 at 20:16
JohnPan
1862
add a comment |
up vote
0
down vote
up vote
0
down vote
Well, I am not an expert with PHP, but I can point out one or two things in this code.
Keep the page and views names as constants in one place. Do not use strings to redirect, but use refs to these constants. This way you have to change it from one place if/when you rename a page/view
Use a function to invoke for redirect which will check the currentRoute. Do not check this in each case/else if
Collect the vars you want to use in advance. Do not ask Auth::user()-> on and on. This is totaly OK but for better readability you could -and should- spare a few nsecs.
answered Sep 30 '17 at 20:16
JohnPan
1862
Well, I am not an expert with PHP, but I can point out one or two things in this code.
Keep the page and views names as constants in one place. Do not use strings to redirect, but use refs to these constants. This way you have to change it from one place if/when you rename a page/view
Use a function to invoke for redirect which will check the currentRoute. Do not check this in each case/else if
Collect the vars you want to use in advance. Do not ask Auth::user()-> on and on. This is totaly OK but for better readability you could -and should- spare a few nsecs.
answered Sep 30 '17 at 20:16
JohnPan
1862
answered Sep 30 '17 at 20:16
JohnPan
1862
answered Sep 30 '17 at 20:16
JohnPan
1862
answered Sep 30 '17 at 20:16
JohnPan
1862
1862
add a comment |
add a comment |
up vote
0
down vote
You have a lot going on in one middleware. Keep in mind a middleware runs for each request so it needs to be reasonably well optimised.
I have split the logging bit out into PlatformLogger class, that seemed like a logical separation.
I have also removed a lot of the } else {, to me they just add noise and in most cases you return anyway, so the else will never get triggered.
PLEASE NOTE: The conditions below where I removed the else's may not all be the equivalent to the sample provided, it is just to show you what it would look like, you would need to check and make those changes yourself if you choose to do it that way.
Also check your if conditions, and do the cheapest test first as they might short circuit the if statement and you won't have to perform the other slower test that involve database access for example.
if (PlatformSetting::find('platform.website.frontend_maintenance_enabled')->value == 1 && $currentRoute != "platform.maintenance" && !Auth::user()->canBypassMaintenance()) {
vs
if ($currentRoute != "platform.maintenance" && PlatformSetting::find('platform.website.frontend_maintenance_enabled')->value == 1 && !Auth::user()->canBypassMaintenance()) {
Example changes
<?php
class PlatformLogger
{
public function handle($request, Closure $next) {
$agent = new Agent();
$entry = new EntryLog;
$entry->address_accessed = "$_SERVER[HTTP_HOST]$_SERVER[REQUEST_URI]";
$entry->request_ip = $request->ip();
$entry->request_device = $agent->isDesktop() ? 'Desktop' : ($agent->isMobile() ? 'Mobile' : 'Tablet');
$entry->request_system = $agent->platform() . ' ' . $agent->version($agent->platform());
$entry->request_browser = $agent->browser();
$entry->request_method = $request->method();
$entry->save();
return $next($request);
}
}
class Platform
{
public function handle($request, Closure $next)
{
$agent = new Agent();
$currentRoute = Route::current()->getName();
$dontCheck = array(
"platform.contact",
"frontend.user.account.logout"
);
if (in_array($currentRoute, $dontCheck)) {
return $next($request);
}
if (!Auth::check()) {
if (UserBan::where('ban_type', 'ip_ban')->where('ban_value', $request->ip())->whereRaw('expires_at > now()')->first() != null && $currentRoute != "platform.banned") {
return redirect()->route('platform.banned');
}
return $next($request);
}
$routeLock = RouteLock::where('route_name', $currentRoute)->first();
if ($routeLock != null && $routeLock->expires_at > time()) {
if (strlen($routeLock->required_permissions) > 0 && !Auth::user()->hasAnyPermissions($routeLock->required_permissions)) {
return redirect()->route('platform.restricted');
}
}
if (Auth::user()->isBanned($request) && $currentRoute != "platform.banned") {
return redirect()->route('platform.banned');
}
if (!Auth::user()->roleplayExists() && $currentRoute != "frontend.user.error") {
return redirect()->route('frontend.user.error');
}
if (PlatformSetting::find('platform.website.frontend_maintenance_enabled')->value == 1 && $currentRoute != "platform.maintenance" && !Auth::user()->canBypassMaintenance()) {
return redirect()->route('platform.maintenance');
}
if ($currentRoute == 'frontend.guest.register.begin' && PlatformSetting::findSetting('website.registration.enabled') == '1') {
return 'registration is currently closed.';
}
if ($currentRoute == 'frontend.user.play') {
$permissions = PlatformSetting::findSetting('website.client.required_permissions');
if (strlen($permissions) > 0) {
if (!Auth::user()->hasAnyPermissions($permissions)) {
return new Response(view('platform.restricted'));
}
}
}
if (Auth::user()->website_setup_finished == '0' && $currentRoute !='frontend.user.setup.step_' . Auth::user()->website_last_step) {
return redirect()->route('frontend.user.setup.step_' . Auth::user()->website_last_step);
}
if (Auth::user()->is_locked == '1' && $currentRoute != 'frontend.user.locked' && $currentRoute != 'frontend.user.account.unlock') {
return redirect()->route('frontend.user.locked');
}
if (Auth::user()->pin_lock == '1' && $currentRoute != 'frontend.user.pin') {
return redirect()->route('frontend.user.pin');
}
if (PlatformSetting::find('platform.website.frontend_maintenance_enabled')->value == 1 && $currentRoute != "platform.maintenance" && !Auth::user()->canBypassMaintenance()) {
return redirect()->route('platform.maintenance');
}
$platformState = PlatformSetting::find('platform.state')->value;
if (($platformState == 1 || $platformState == 2 || $platformState == 3) && !Auth::user()->hasBeta()) {
return redirect()->route('frontend.user.beta');
}
else {
$allowedPages = PlatformSetting::findSetting('website.allowed_pages');
if (strlen($allowedPages) > 0) {
if (!in_array($currentRoute, explode(',', $allowedPages))) {
return new Response(view('platform.restricted'));
}
}
}
return $next($request);
}
}
answered Oct 31 '17 at 0:38
bumperbox
1,843615
add a comment |
up vote
0
down vote
You have a lot going on in one middleware. Keep in mind a middleware runs for each request so it needs to be reasonably well optimised.
I have split the logging bit out into PlatformLogger class, that seemed like a logical separation.
I have also removed a lot of the } else {, to me they just add noise and in most cases you return anyway, so the else will never get triggered.
PLEASE NOTE: The conditions below where I removed the else's may not all be the equivalent to the sample provided, it is just to show you what it would look like, you would need to check and make those changes yourself if you choose to do it that way.
Also check your if conditions, and do the cheapest test first as they might short circuit the if statement and you won't have to perform the other slower test that involve database access for example.
if (PlatformSetting::find('platform.website.frontend_maintenance_enabled')->value == 1 && $currentRoute != "platform.maintenance" && !Auth::user()->canBypassMaintenance()) {
vs
if ($currentRoute != "platform.maintenance" && PlatformSetting::find('platform.website.frontend_maintenance_enabled')->value == 1 && !Auth::user()->canBypassMaintenance()) {
Example changes
<?php
class PlatformLogger
{
public function handle($request, Closure $next) {
$agent = new Agent();
$entry = new EntryLog;
$entry->address_accessed = "$_SERVER[HTTP_HOST]$_SERVER[REQUEST_URI]";
$entry->request_ip = $request->ip();
$entry->request_device = $agent->isDesktop() ? 'Desktop' : ($agent->isMobile() ? 'Mobile' : 'Tablet');
$entry->request_system = $agent->platform() . ' ' . $agent->version($agent->platform());
$entry->request_browser = $agent->browser();
$entry->request_method = $request->method();
$entry->save();
return $next($request);
}
}
class Platform
{
public function handle($request, Closure $next)
{
$agent = new Agent();
$currentRoute = Route::current()->getName();
$dontCheck = array(
"platform.contact",
"frontend.user.account.logout"
);
if (in_array($currentRoute, $dontCheck)) {
return $next($request);
}
if (!Auth::check()) {
if (UserBan::where('ban_type', 'ip_ban')->where('ban_value', $request->ip())->whereRaw('expires_at > now()')->first() != null && $currentRoute != "platform.banned") {
return redirect()->route('platform.banned');
}
return $next($request);
}
$routeLock = RouteLock::where('route_name', $currentRoute)->first();
if ($routeLock != null && $routeLock->expires_at > time()) {
if (strlen($routeLock->required_permissions) > 0 && !Auth::user()->hasAnyPermissions($routeLock->required_permissions)) {
return redirect()->route('platform.restricted');
}
}
if (Auth::user()->isBanned($request) && $currentRoute != "platform.banned") {
return redirect()->route('platform.banned');
}
if (!Auth::user()->roleplayExists() && $currentRoute != "frontend.user.error") {
return redirect()->route('frontend.user.error');
}
if (PlatformSetting::find('platform.website.frontend_maintenance_enabled')->value == 1 && $currentRoute != "platform.maintenance" && !Auth::user()->canBypassMaintenance()) {
return redirect()->route('platform.maintenance');
}
if ($currentRoute == 'frontend.guest.register.begin' && PlatformSetting::findSetting('website.registration.enabled') == '1') {
return 'registration is currently closed.';
}
if ($currentRoute == 'frontend.user.play') {
$permissions = PlatformSetting::findSetting('website.client.required_permissions');
if (strlen($permissions) > 0) {
if (!Auth::user()->hasAnyPermissions($permissions)) {
return new Response(view('platform.restricted'));
}
}
}
if (Auth::user()->website_setup_finished == '0' && $currentRoute !='frontend.user.setup.step_' . Auth::user()->website_last_step) {
return redirect()->route('frontend.user.setup.step_' . Auth::user()->website_last_step);
}
if (Auth::user()->is_locked == '1' && $currentRoute != 'frontend.user.locked' && $currentRoute != 'frontend.user.account.unlock') {
return redirect()->route('frontend.user.locked');
}
if (Auth::user()->pin_lock == '1' && $currentRoute != 'frontend.user.pin') {
return redirect()->route('frontend.user.pin');
}
if (PlatformSetting::find('platform.website.frontend_maintenance_enabled')->value == 1 && $currentRoute != "platform.maintenance" && !Auth::user()->canBypassMaintenance()) {
return redirect()->route('platform.maintenance');
}
$platformState = PlatformSetting::find('platform.state')->value;
if (($platformState == 1 || $platformState == 2 || $platformState == 3) && !Auth::user()->hasBeta()) {
return redirect()->route('frontend.user.beta');
}
else {
$allowedPages = PlatformSetting::findSetting('website.allowed_pages');
if (strlen($allowedPages) > 0) {
if (!in_array($currentRoute, explode(',', $allowedPages))) {
return new Response(view('platform.restricted'));
}
}
}
return $next($request);
}
}
answered Oct 31 '17 at 0:38
bumperbox
1,843615
add a comment |
up vote
0
down vote
up vote
0
down vote
You have a lot going on in one middleware. Keep in mind a middleware runs for each request so it needs to be reasonably well optimised.
I have split the logging bit out into PlatformLogger class, that seemed like a logical separation.
I have also removed a lot of the } else {, to me they just add noise and in most cases you return anyway, so the else will never get triggered.
PLEASE NOTE: The conditions below where I removed the else's may not all be the equivalent to the sample provided, it is just to show you what it would look like, you would need to check and make those changes yourself if you choose to do it that way.
Also check your if conditions, and do the cheapest test first as they might short circuit the if statement and you won't have to perform the other slower test that involve database access for example.
if (PlatformSetting::find('platform.website.frontend_maintenance_enabled')->value == 1 && $currentRoute != "platform.maintenance" && !Auth::user()->canBypassMaintenance()) {
vs
if ($currentRoute != "platform.maintenance" && PlatformSetting::find('platform.website.frontend_maintenance_enabled')->value == 1 && !Auth::user()->canBypassMaintenance()) {
Example changes
<?php
class PlatformLogger
{
public function handle($request, Closure $next) {
$agent = new Agent();
$entry = new EntryLog;
$entry->address_accessed = "$_SERVER[HTTP_HOST]$_SERVER[REQUEST_URI]";
$entry->request_ip = $request->ip();
$entry->request_device = $agent->isDesktop() ? 'Desktop' : ($agent->isMobile() ? 'Mobile' : 'Tablet');
$entry->request_system = $agent->platform() . ' ' . $agent->version($agent->platform());
$entry->request_browser = $agent->browser();
$entry->request_method = $request->method();
$entry->save();
return $next($request);
}
}
class Platform
{
public function handle($request, Closure $next)
{
$agent = new Agent();
$currentRoute = Route::current()->getName();
$dontCheck = array(
"platform.contact",
"frontend.user.account.logout"
);
if (in_array($currentRoute, $dontCheck)) {
return $next($request);
}
if (!Auth::check()) {
if (UserBan::where('ban_type', 'ip_ban')->where('ban_value', $request->ip())->whereRaw('expires_at > now()')->first() != null && $currentRoute != "platform.banned") {
return redirect()->route('platform.banned');
}
return $next($request);
}
$routeLock = RouteLock::where('route_name', $currentRoute)->first();
if ($routeLock != null && $routeLock->expires_at > time()) {
if (strlen($routeLock->required_permissions) > 0 && !Auth::user()->hasAnyPermissions($routeLock->required_permissions)) {
return redirect()->route('platform.restricted');
}
}
if (Auth::user()->isBanned($request) && $currentRoute != "platform.banned") {
return redirect()->route('platform.banned');
}
if (!Auth::user()->roleplayExists() && $currentRoute != "frontend.user.error") {
return redirect()->route('frontend.user.error');
}
if (PlatformSetting::find('platform.website.frontend_maintenance_enabled')->value == 1 && $currentRoute != "platform.maintenance" && !Auth::user()->canBypassMaintenance()) {
return redirect()->route('platform.maintenance');
}
if ($currentRoute == 'frontend.guest.register.begin' && PlatformSetting::findSetting('website.registration.enabled') == '1') {
return 'registration is currently closed.';
}
if ($currentRoute == 'frontend.user.play') {
$permissions = PlatformSetting::findSetting('website.client.required_permissions');
if (strlen($permissions) > 0) {
if (!Auth::user()->hasAnyPermissions($permissions)) {
return new Response(view('platform.restricted'));
}
}
}
if (Auth::user()->website_setup_finished == '0' && $currentRoute !='frontend.user.setup.step_' . Auth::user()->website_last_step) {
return redirect()->route('frontend.user.setup.step_' . Auth::user()->website_last_step);
}
if (Auth::user()->is_locked == '1' && $currentRoute != 'frontend.user.locked' && $currentRoute != 'frontend.user.account.unlock') {
return redirect()->route('frontend.user.locked');
}
if (Auth::user()->pin_lock == '1' && $currentRoute != 'frontend.user.pin') {
return redirect()->route('frontend.user.pin');
}
if (PlatformSetting::find('platform.website.frontend_maintenance_enabled')->value == 1 && $currentRoute != "platform.maintenance" && !Auth::user()->canBypassMaintenance()) {
return redirect()->route('platform.maintenance');
}
$platformState = PlatformSetting::find('platform.state')->value;
if (($platformState == 1 || $platformState == 2 || $platformState == 3) && !Auth::user()->hasBeta()) {
return redirect()->route('frontend.user.beta');
}
else {
$allowedPages = PlatformSetting::findSetting('website.allowed_pages');
if (strlen($allowedPages) > 0) {
if (!in_array($currentRoute, explode(',', $allowedPages))) {
return new Response(view('platform.restricted'));
}
}
}
return $next($request);
}
}
answered Oct 31 '17 at 0:38
bumperbox
1,843615
You have a lot going on in one middleware. Keep in mind a middleware runs for each request so it needs to be reasonably well optimised.
I have split the logging bit out into PlatformLogger class, that seemed like a logical separation.
I have also removed a lot of the } else {, to me they just add noise and in most cases you return anyway, so the else will never get triggered.
PLEASE NOTE: The conditions below where I removed the else's may not all be the equivalent to the sample provided, it is just to show you what it would look like, you would need to check and make those changes yourself if you choose to do it that way.
Also check your if conditions, and do the cheapest test first as they might short circuit the if statement and you won't have to perform the other slower test that involve database access for example.
if (PlatformSetting::find('platform.website.frontend_maintenance_enabled')->value == 1 && $currentRoute != "platform.maintenance" && !Auth::user()->canBypassMaintenance()) {
vs
if ($currentRoute != "platform.maintenance" && PlatformSetting::find('platform.website.frontend_maintenance_enabled')->value == 1 && !Auth::user()->canBypassMaintenance()) {
Example changes
<?php
class PlatformLogger
{
public function handle($request, Closure $next) {
$agent = new Agent();
$entry = new EntryLog;
$entry->address_accessed = "$_SERVER[HTTP_HOST]$_SERVER[REQUEST_URI]";
$entry->request_ip = $request->ip();
$entry->request_device = $agent->isDesktop() ? 'Desktop' : ($agent->isMobile() ? 'Mobile' : 'Tablet');
$entry->request_system = $agent->platform() . ' ' . $agent->version($agent->platform());
$entry->request_browser = $agent->browser();
$entry->request_method = $request->method();
$entry->save();
return $next($request);
}
}
class Platform
{
public function handle($request, Closure $next)
{
$agent = new Agent();
$currentRoute = Route::current()->getName();
$dontCheck = array(
"platform.contact",
"frontend.user.account.logout"
);
if (in_array($currentRoute, $dontCheck)) {
return $next($request);
}
if (!Auth::check()) {
if (UserBan::where('ban_type', 'ip_ban')->where('ban_value', $request->ip())->whereRaw('expires_at > now()')->first() != null && $currentRoute != "platform.banned") {
return redirect()->route('platform.banned');
}
return $next($request);
}
$routeLock = RouteLock::where('route_name', $currentRoute)->first();
if ($routeLock != null && $routeLock->expires_at > time()) {
if (strlen($routeLock->required_permissions) > 0 && !Auth::user()->hasAnyPermissions($routeLock->required_permissions)) {
return redirect()->route('platform.restricted');
}
}
if (Auth::user()->isBanned($request) && $currentRoute != "platform.banned") {
return redirect()->route('platform.banned');
}
if (!Auth::user()->roleplayExists() && $currentRoute != "frontend.user.error") {
return redirect()->route('frontend.user.error');
}
if (PlatformSetting::find('platform.website.frontend_maintenance_enabled')->value == 1 && $currentRoute != "platform.maintenance" && !Auth::user()->canBypassMaintenance()) {
return redirect()->route('platform.maintenance');
}
if ($currentRoute == 'frontend.guest.register.begin' && PlatformSetting::findSetting('website.registration.enabled') == '1') {
return 'registration is currently closed.';
}
if ($currentRoute == 'frontend.user.play') {
$permissions = PlatformSetting::findSetting('website.client.required_permissions');
if (strlen($permissions) > 0) {
if (!Auth::user()->hasAnyPermissions($permissions)) {
return new Response(view('platform.restricted'));
}
}
}
if (Auth::user()->website_setup_finished == '0' && $currentRoute !='frontend.user.setup.step_' . Auth::user()->website_last_step) {
return redirect()->route('frontend.user.setup.step_' . Auth::user()->website_last_step);
}
if (Auth::user()->is_locked == '1' && $currentRoute != 'frontend.user.locked' && $currentRoute != 'frontend.user.account.unlock') {
return redirect()->route('frontend.user.locked');
}
if (Auth::user()->pin_lock == '1' && $currentRoute != 'frontend.user.pin') {
return redirect()->route('frontend.user.pin');
}
if (PlatformSetting::find('platform.website.frontend_maintenance_enabled')->value == 1 && $currentRoute != "platform.maintenance" && !Auth::user()->canBypassMaintenance()) {
return redirect()->route('platform.maintenance');
}
$platformState = PlatformSetting::find('platform.state')->value;
if (($platformState == 1 || $platformState == 2 || $platformState == 3) && !Auth::user()->hasBeta()) {
return redirect()->route('frontend.user.beta');
}
else {
$allowedPages = PlatformSetting::findSetting('website.allowed_pages');
if (strlen($allowedPages) > 0) {
if (!in_array($currentRoute, explode(',', $allowedPages))) {
return new Response(view('platform.restricted'));
}
}
}
return $next($request);
}
}
answered Oct 31 '17 at 0:38
bumperbox
1,843615
answered Oct 31 '17 at 0:38
bumperbox
1,843615
answered Oct 31 '17 at 0:38
bumperbox
1,843615
answered Oct 31 '17 at 0:38
bumperbox
1,843615
1,843615
add a comment |
add a comment |
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
StackExchange.ready(
function () {
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f176845%2flaravel-middleware%23new-answer', 'question_page');
}
);
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
