PHP Developers Network

A community of PHP developers offering assistance, advice, discussion, and friendship.
 
Loading
It is currently Tue Jul 23, 2019 11:41 am

All times are UTC - 5 hours




Post new topic Reply to topic  [ 1 post ] 
Author Message
PostPosted: Sat Aug 22, 2015 10:01 am 
Offline
Moderator
User avatar

Joined: Tue Nov 09, 2010 3:39 pm
Posts: 6425
Location: Montreal, Canada





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,

Syntax: [ Download ] [ Hide ]
$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,

Syntax: [ Download ] [ Hide ]
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

Syntax: [ Download ] [ Hide ]
$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 $ and $ with calls to Klein's request object. $ 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.

Syntax: [ Download ] [ Hide ]
<?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 /, switch out Klein's session starting functionality with our own, and swap out direct references to the $ superglobal with our object.

Syntax: [ Download ] [ Hide ]
$service->startSession();


becomes

Syntax: [ Download ] [ Hide ]
$service->session = new Squeaker\Session;
$service->session->start();


Syntax: [ Download ] [ Hide ]
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

Syntax: [ Download ] [ Hide ]
$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

Syntax: [ Download ] [ Hide ]
$_SESSION['user'] = $db->getUserID($request->param('username'));


become this

Syntax: [ Download ] [ Hide ]
$service->session->set('user', $db->getUserID($request->param('username')));


and on logout we call $->->() instead of (). 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 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 statements (leaving Composer's), register our namespace in our file like so

Syntax: [ Download ] [ Hide ]
"autoload": {
    "psr-4": {
        "Squeaker\\": "app/"
    }
}


run 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
Syntax: [ Download ] [ Hide ]
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.

Syntax: [ Download ] [ Hide ]
$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 calls, replace calls to $ with calls to $->() and our functionality is preserved and our DB instance is only created when we actually need it. Beautiful. Looking through our , 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 //, //, etc is both a little confusing -- think // vs / -- and going to cause a collision with our // route anyway. We could use a different name, maybe /, 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 magic method to check login state, but that's pretty hacky and goes against OOP principles. We could also create a method in our controller and pass through our desired method as an argument, so $->('') rather than $->(). 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.


_________________


Top
 Profile  
 
Display posts from previous:  Sort by  
Post new topic Reply to topic  [ 1 post ] 

All times are UTC - 5 hours


Who is online

Users browsing this forum: No registered users and 1 guest


You cannot post new topics in this forum
You cannot reply to topics in this forum
You cannot edit your posts in this forum
You cannot delete your posts in this forum
You cannot post attachments in this forum

Jump to:  
Powered by phpBB® Forum Software © phpBB Group