Untangling Spaghetti - Lesson 2.5

Tutorials on PHP, databases and other aspects of web development. Before posting a question, check in here to see whether there's a tutorial that covers your problem.

Moderator: General Moderators

Post Reply
User avatar
Celauran
Moderator
Posts: 6425
Joined: Tue Nov 09, 2010 2:39 pm
Location: Montreal, Canada

Untangling Spaghetti - Lesson 2.5

Post by Celauran »

Lesson 0 - Set up
Lesson 1 - Database abstraction
Lesson 2 - Routing and templating
Lesson 3 - Exceptions and logging

Our last lesson was beginning to run long, so I decided to wrap up at what felt like the cleanest point possible. There remain, however, some fairly glaring issues that I'd like to address before moving on. A shorter lesson, then, to cover those points and more neatly wrap up the last.

Perhaps the most obvious issue that we've yet to address -- and this is something I intentionally put into the app because I encounter it all the time -- is that we're storing a user's username to session data on login, and then using it to look up their user ID pretty much everywhere. That's a fairly ridiculous thing to do when we could store the ID from the get go and be done with it. Change our login functionality slightly,

Code: Select all

$router->post('/login', function($request, $response, $service, $app) use ($db) {
    $logged = $db->login($request->param('username'), $request->param('password'));
    if ($logged === true) {
        $_SESSION['user'] = $db->getUserID($request->param('username'));
        $response->redirect('/home')->send();
    } else {
        $service->flash('Incorrect username or password', 'error');
        $service->back();
    }
});
our catch-all route logic slightly,

Code: Select all

if (isset($_SESSION['user'])) {
    $service->current_user = $_SESSION['user'];
    $service->sidebar_data = $db->getUserSidebar($service->current_user);
    $service->am_following = $db->getUsersBeingFollowed($service->current_user);
}
and we're set. Now any route that requires login looks more like this

Code: Select all

$router->get('/home', function($request, $response, $service, $app) use ($db) {
    if (!isset($service->current_user)) {
        $response->redirect('/login')->send();
    }

    $squeaks = $db->getSqueaksForHome($service->current_user);

    $service->render($service->views_dir . 'home.php', ['squeaks' => $squeaks]);
});
and we've gotten rid of a bunch of unnecessary database calls in the process.

We talked at some point about not using superglobals, and we replaced calls to $_POST and $_GET with calls to Klein's request object. $_SESSION is still being referenced directly, though. We can loosely wrap that in a class with simple set and get functionality with very little effort and clean up the above even further.

Code: Select all

<?php

namespace Squeaker;

class Session {
    protected $started = false;
    protected $data = [];

    public function delete($key) {
        unset($this->data[$key]);
    }

    public function destroy() {
        $this->data = [];
        session_destroy();
        $this->started = false;
    }

    public function get($key) {
        return array_key_exists($key, $this->data) ? $this->data[$key] : null;
    }

    public function set($key, $value) {
        $this->data[$key] = $value;
    }

    public function start() {
        if (!$this->started) {
            session_start();
            $this->data = &$_SESSION;
            $this->started = true;
        }

        return true;
    }
}
So we create our session wrapper in app/Session.php, switch out Klein's session starting functionality with our own, and swap out direct references to the $_SESSION superglobal with our Session object.

Code: Select all

$service->startSession();
becomes

Code: Select all

$service->session = new Squeaker\Session;
$service->session->start();

Code: Select all

if (isset($_SESSION['user'])) {
    $service->current_user = $_SESSION['user'];
    $service->sidebar_data = $db->getUserSidebar($service->current_user);
    $service->am_following = $db->getUsersBeingFollowed($service->current_user);
}
becomes

Code: Select all

$current_user = $service->session->get('user');
if (isset($current_user)) {
    $service->current_user = $current_user;
    $service->sidebar_data = $db->getUserSidebar($service->current_user);
    $service->am_following = $db->getUsersBeingFollowed($service->current_user);
}
Our login function sees this

Code: Select all

$_SESSION['user'] = $db->getUserID($request->param('username'));
become this

Code: Select all

$service->session->set('user', $db->getUserID($request->param('username')));
and on logout we call $service->session->destroy() instead of session_destroy(). Really just a couple of minor changes. And of course, we need to remember to require our new class at the top of our index file. Hang on. We're getting rid of inclues and globals but we're just going to litter the code with requires? How is that different from includes? This is actually another place where namespacing our classes comes in handy; autoloading. PHP's spl_autoload_register allows us to define our own autoloading functions so we can instantiate objects without having explicitly required the class and PHP will still know where to look for them. Even better, though, Composer allows us to register namespaces to include in its autoloader, which we're already using. Let's do that. We remove our two require statements (leaving Composer's), register our namespace in our composer.json file like so

Code: Select all

"autoload": {
    "psr-4": {
        "Squeaker\\": "app/"
    }
}
run composer dump-autoload and all of our classes under the Squeaker namespace, including ones we've not yet written, will be automatically loaded when called.

That's a little improvement, a little cleaner. We've still got

Code: Select all

use ($db)
being called on nearly every route, and we're instantiating the PDO and DB objects even in cases where we're not using them. We can address both of those by registering the class on Klein's service provider and lazy loading it. Since Klein responds to all matching routes, let's create a separate catch-all route for lazy loading any services we may need.

Code: Select all

$router->respond(function($request, $response, $service, $app) {
    $app->register('DB', function() {
        $pdo = new PDO('mysql:host=' . getenv('DBHOST') . ';dbname=' . getenv('DBNAME'), getenv('DBUSER'), getenv('DBPASS'));
        $db = new Squeaker\DB($pdo);
        return $db;
    });
});
Now we can remove all the use calls, replace calls to $db with calls to $app->DB() and our functionality is preserved and our DB instance is only created when we actually need it. Beautiful. Looking through our index.php, the main logic of our application, there's very little repetition remaining.

The only obvious one is manually checking if a user is logged in on every route that requires it and redirecting to the login page if not. This one is a bit of a tougher nut to crack. If we want to abstract that away, we'd need some way to have it called automatically on all the routes that need it. Klein provides route namespacing, which could work, but all of our routes are in the same namespace and creating a new subspace for those routes, say /user/profile, /user/follow, etc is both a little confusing -- think /user/follow vs /following -- and going to cause a collision with our /user/ route anyway. We could use a different name, maybe /private, but it's not pretty. We could change our routes to use controller actions instead of callbacks, set our methods to protected, and lean on the __call magic method to check login state, but that's pretty hacky and goes against OOP principles. We could also create a run method in our controller and pass through our desired method as an argument, so $controller->run('home') rather than $controller->home(). A little better, but still feels kind of dirty. The least bad option maybe? Calling some method before the target route's controller action gets called isn't really the controller's responsibility, though. That's something more suited to the dispatcher. I suppose we could extend Klein and overwrite its dispatch handling, or we could look for a different router than does allow that kind of filtering, but this is starting to become an awful lot of work for admittedly little gain. Or we could just leave it. When you get right down to it, we're looking to remove three lines of code that gets repeated eight times. Abstraction is great. Abstraction for the sake of abstraction, less so. Something I think warrants keeping in mind when undertaking refactoring projects like this, or even when creating entirely new functionality.

Our index file is still a little on the large size and we could shunt some of that off to a config file, maybe leave the bootstrapping and move the routes themselves into a routes file, both of which could either be wrapped in classes or just included directly, and maybe that's something we'll want to revisit as we add more code to the project, but for now I think it's fine and I'll leave that decision to the reader's discretion.

I said it was going to be a short lesson this time. We've cleaned up a few of the bits I felt we left hanging last time, and this feels like a good place to wrap up the lesson. Error handling definitely merits a lesson unto itself, so we'll leave that for next time.

Download lesson
Post Reply