PHP Developers Network

A community of PHP developers offering assistance, advice, discussion, and friendship.
 
Loading
It is currently Tue Oct 17, 2017 5:23 pm

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: 6384
Location: Montreal, Canada
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,

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 $_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.

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

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 $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

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

_________________
Supported PHP versions No longer supported versions


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 2 guests


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