Laravel Middleware

Question

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);
    }
}

Answer 1

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);
    }
}

Answer 2

Well, I am not an expert with PHP, but I can point out one or two things in this code.

1. 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

2. Use a function to invoke for redirect which will check the currentRoute. Do not check this in each case/else if

3. 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.