"Wifi Here on a Blackboard" by "Jem Stone" on Flickr

Free Wi-Fi does not need to be password-less!

Recently a friend of mine forwarded an email to me about a Wi-fi service he wanted to use from a firm, but he raised some technical questions with them which they seemed to completely misunderstand!

So, let’s talk about the misconceptions of Wi-fi passwords.

Many people assume that when you log into a system, it means that system is secure. For example, logging into a website makes sure that your data is secure and protected, right? Not necessarily – the password you entered could be on a web page that is not secured by TLS, or perhaps the web server doesn’t properly transfer it’s contents to a database. Maybe the website was badly written, and means it’s vulnerable to one of a handful of common attacks (with fun names like “Cross Site Scripting” or “SQL Injection Attacks”)…

People also assume the same thing about Wi-fi. You reached a log in page, so it must be secure, right? It depends. If you didn’t put in a password to access the Wi-fi in the first place (like in the image of the Windows 10 screen, or on my KDE Desktop) then you’re probably using Unsecured Wi-fi.

An example of a secured Wi-fi sign-in box on Windows 10
The same Wi-fi sign in box on KDE Neon

People like to compare network traffic to “sending things through the post”, notablycomparing E-Mail to “sending a postcard”, versus PGP encrypted E-Mail being compared to “sending a sealed letter”. Unencrypted Wi-fi is like using CB. Anyone who can hear your signal can understand what you are saying… but if you visit a website which uses HTTPS, then it’s like listening to someone saying random numbers over the radio.

And, if you’re using Unencrypted Wi-fi, it’s also possible for an attacker to see what website you visited, because the request for the address to reach on the Internet (e.g. “Google.com” = 172.217.23.14) is sent in the clear. Also because of the way that DNS works (that name to address matching thing) means that if someone knows you’re visiting a “site of interest” (like, perhaps a bank website), they can reply *before* the real DNS server, and tell you that the server on their machine is actually your bank’s website.

So, many of these things can be protected against by using a simple method, that many people who provide Wi-fi don’t do.

Turn on WPA2 (the authentication bit). Even if *everyone* uses the same password (which they’d have to for WPA2), the fact you’re logging into the Access Point means it creates a unique shared secret for your session.

“But hang on”, I hear the guy at the back cry, “you used the same password – how does that work?”

OK, so this is where the fun stuff starts. The password is just part of how you negotiate to get on to the network. There’s a complex beast of a method that explains how get a shared unique secret when you’re passing stuff around “in the clear”, and so as a result, when you first connect to that Wi-fi access point, and you hand over your password, it “Authorises” you on to the network, but then hands you over to the encryption part, where you generate a key and then use that to talk to each other. The encryption is the bit like “HTTPS”, where you make it so that people can’t see what you’re looking at.

“I got told that if everyone used the same password” said a hipster in the front row, “I wouldn’t be able to tell them apart.” Aha, not true. You can have a separate passphrase to access the Wi-fi from the Login page, after all, you’ve got to make sure that people aren’t breaking the rules (which they *TOTALLY* read, before clicking “I agree, just get me on the damn Wi-fi already”) by using your network.

“OK”, says the lady over on the right, “but when I connected to the Wi-fi, they asked me to log in using Facebook – that’s secure, right?”

Um, no. Well, maybe. See, if they gave you a WPA2 password to log into the Wi-fi, and then the first thing you got to was that login screen, then yep, it’s all good! {*} You can browse with (relative) impunity. But if they didn’t… well, not only are they asking you to shout your secrets on the radio, but if you’re really unlucky, the page asking you to log into Facebook might *also* not actually be Facebook, but another website that just looks like Facebook… after all, I’m sure that page you went to complained that it wasn’t Google or Facebook when you tried to open it…

{*} Except for the fact they’re asking you to tell them not only who you are, but who you’re also friends with, where you went to school, what your hobbies are, what groups you’re in, your date of birth and so on.

But anyway. I understand why those login screens are there. They’re asserting that not only do you understand that you mustn’t use their network for bad things, but that if the police come and ask them who used their network to do something naughty, they can say “He said his name was ‘Bob Smith’ and his email address was ‘bob@example.com’, Officer”…

It also means that the “free” service they provide to you, usually at some great expense (*eye roll*) can get them some return on investment (like, they just got your totally-real-and-not-at-all-made-up-email-address… honest, and they also know what websites you visited while you were there, which they can sell on).

So… What to do the next time you “need” Wi-fi, and there’s a free service there? Always use a VPN when you’re not using a network you trust. If the Wi-fi isn’t using WPA2 encryption (even something as simple as “Buy a drink first” is a great passphrase to use!) point them to this page, and tell them it’s virtually pain free (as long as the passphrase is easy to remember, easy to type and doesn’t have too many weird symbols in) and makes their service more safe and secure for their customers…

Featured image is “Wifi Here on a Blackboard” by “Jem Stone” on Flickr and is released under a CC-BY license.

A quick note on autoloaders for PHP

Over the past few days, as you may have noticed, I’ve been experimenting with PHPUnit, and writing up notes on what I’ve learned. Here’s a biggie, but it’s such a small actual change, I didn’t want to miss it.

So, when you have your autoloader written, you’ll have a function like this (probably):

<?php
function __autoload($classname)
{
    if (file_exists(dirname(__FILE__) . '/classes/' . $classname . '.php')) {
        require_once dirname(__FILE__) . '/classes/' . $classname . '.php';
    }
}

Load this from your test, or in a bootstrap file (more to come on that particular subject, I think!), like this:

<?php
require_once dirname(__FILE__) . '/../autoloader.php';
class SomeClassTest extends ........

And you’ll probably notice the autoloader doesn’t do anything… but why is this? Because PHPUnit has it’s own autoloader, and you need to chain our autoloader to the end. So, in your autoloader file, add this line to the end:

<?php
function __autoload($classname)
{
    if (file_exists(dirname(__FILE__) . '/classes/' . $classname . '.php')) {
        require_once dirname(__FILE__) . '/classes/' . $classname . '.php';
    }
}

spl_autoload_register('__autoload');

And it all should just work, which is nice :)

A quick word on salting your hashes.

If you don’t know what hashing is in relation to coding, the long version is here: Cryptographic Hash Function but the short version is that it performs a mathermatical formula to components of the file, string or data, and returns a much shorter number with a slim chance of “collisions”.

I don’t know whether it’s immediately clear to anyone else, but I used to think this was a good idea.

<?php
$password = sha1($_POST['password']);

Then I went to a PHPNW session, and asked someone to take a look at my code, and got a thorough drubbing for not adding a cryptographic salt (wikipedia).

For those who don’t know, a salt is a set of characters you add before or after the password (or both!) to make it so that a simple “rainbow table analysis” doesn’t work (essentially a brute-force attack against the authentication data by hashing lots and lots of strings looking for another hash which matches the stored hash). In order to make it possible to actually authenticate with that string again in the future, the string should be easily repeatable, and a way to do that is to use other data that’s already in the user record.

For example, this is a simple salt:

<?php
$password = sha1('salt' . $_POST['password']);

I read in the April 2012 edition of 2600 magazine something that I should have been doing with my hashes all along. How’s this for more secure code?

<?php
$site_salt = 'pepper';
$SQL = "SELECT intUserID FROM users WHERE strUsername = ?";
$DB = new PDO($dsn);
$query = $DB->prepare($SQL);
$query->execute(array(strtolower($_POST['username'])));
$userid = $query->fetch();
if ($userid == false) {
  return false;
}
$prefix = '';
$suffix = '';
if ($userid % 2 == 0) {
  $prefix = $site_salt;
} else {
  $suffix = $site_salt;
}
if ($userid % 3 == 0) {
  $prefix .= strtolower($_POST['username']);
} else {
  $suffix .= strtolower($_POST['username']);
}
if ($userid % 4 == 0) {
  $prefix = strrev($prefix);
}
if ($userid % 5 == 0) {
  $suffix = strrev($suffix);
}
$hashedPassword = sha1($prefix . $_POST['password'] . $suffix);

So, this gives you an easily repeatable string, that’s relatively hard to calculate without easy access to the source code :)

Getting started with Unit Testing for PHP

Unit testing seems like a bit of a dark art when you’re first introduced to it. “Create this new file. Tell it what is supposed to be the result when you run a test, and it’ll tell you if you’re right nor not.”

Let’s start with a pseudocode example:

test->assertTrue(1+1 = 2); // Test returns true, huzzah!
test->assertFalse(1+1 = 3); // Test returns false. Those integers must not have been large enough

I want to use PHPUnit, and for me the easiest way to get this and the rest of the tools I’ll be referring to in this collection of posts is to install “The PHP Quality Assurance Toolchain“. On my Ubuntu install, this was done as follows:

sudo pear upgrade PEAR
sudo pear config-set auto_discover 1
sudo pear install --all-deps pear.phpqatools.org/phpqatools

Now we’ve got the tools in place, let’s set up the directory structure.

/
+ -- Classes
|    + -- Config.php
+ -- Tests
     + -- ConfigTest.php

In here, you see we’ve created two files, one contains the class we want to use, and the other contains the tests we will be running.

So, let’s slap on the veneer of coating that these two files need to be valid to test.

/Classes/Config.php

<?php
class Config
{
}

/Tests/Config.php

<?php

include dirname(__FILE__) . '/../Classes/Config.php';

class ConfigTest extends PHPUnit_Framework_TestCase
{
}

So, just to summarise, here we have two, essentially empty classes.

Let’s put some code into the test file.

<?php

include dirname(__FILE__) . '/../Classes/Config.php';

class ConfigTest extends PHPUnit_Framework_TestCase
{
  public function testCreateObject()
  {
    $config = new Config();
    $this->assertTrue(is_object($config));
  }
}

We can now run this test from the command line as follows:

phpunit Tests/ConfigTest.php

phpunit Tests/01_ConfigTest.php
PHPUnit 3.6.10 by Sebastian Bergmann.

.

Time: 1 second, Memory: 3.00Mb

OK (1 test, 1 assertion)

That was nice and straightforward!

Let’s add some more code!

In ConfigTest, let’s tell it to load some configuration, using a config file.

<?php

include dirname(__FILE__) . '/../Classes/Config.php';

class ConfigTest extends PHPUnit_Framework_TestCase
{
  public function testCreateObject()
  {
    $config = new Config();
    $this->assertTrue(is_object($config));
  }

  public function testLoadConfig()
  {
    $config = new Config();
    $config->load();
  }
}

And now when we run it?

PHP Fatal error:  Call to undefined method Config::load() in /var/www/PhpBetterPractices/Tests/ConfigTest.php on line 16

Ah, perhaps we need to write some code into /Classes/Config.php

<?php
class Config
{
  public function load()
  {
    include dirname(__FILE__) . '/../Config/default_config.php';
  }
}

But, running this, again, we get an error message!

PHPUnit 3.6.10 by Sebastian Bergmann.

.E

Time: 0 seconds, Memory: 3.00Mb

There was 1 error:

1) ConfigTest::testLoadConfig
include(/var/www/PhpBetterPractices/Config/default_config.php): failed to open stream: No such file or directory

/var/www/PhpBetterPractices/Classes/Config.php:7
/var/www/PhpBetterPractices/Classes/Config.php:7
/var/www/PhpBetterPractices/Tests/ConfigTest.php:16

FAILURES!
Tests: 2, Assertions: 1, Errors: 1.

So, we actually need to check that the file exists first, perhaps we should throw an error if it doesn’t? We could also pass the name of the config file to pass to the script, which would let us test more and different configuration options, should we need them.

class Config
{
    public function load($file = null)
    {
        if ($file == null) {
            $file = 'default.config.php';
        }

        $filename = dirname(__FILE__) . '/../Config/' . $file;

        if (file_exists($filename)) {
            include $filename;
        } else {
            throw new InvalidArgumentException("File not found");
        }
    }
}

So, here’s the new UnitTest code:

class ConfigTest extends PHPUnit_Framework_TestCase
{
    public function testCreateObject()
    {
        $config = new Config();
        $this->assertTrue(is_object($config));
    }

    public function testLoadConfig()
    {
        $config = new Config();
        $config->load();
    }

    /**
     * @expectedException InvalidArgumentException
     */
    public function testFailLoadingConfig()
    {
        $config = new Config();
        @$config->load('A file which does not exist');
    }
}

This assumes the file /Config/default.config.php exists, albeit as an empty file.

So, let’s run those tests and see what happens?

PHPUnit 3.6.10 by Sebastian Bergmann.

...

Time: 0 seconds, Memory: 3.25Mb

OK (3 tests, 2 assertions)

Huzzah! That’s looking good. Notice that to handle a test of something which should throw an exception, you can either wrapper the function in a try/catch loop and, in the try side of the loop, have $this->assertTrue(false) to prevent false positives and in the catch side, do your $this->assertBlah() on the exception. Alternatively, (and much more simplely), use a documentation notation of @expectedException NameOfException and then prefix the function you are testing with the @ symbol. This is how I did it with the test “testFailLoadingConfig()”.

This obviously doesn’t handle setting and getting configuration values, so let’s add those.

Here’s the additions to the Config.php file:

    public function set($key = null, $value = null)
    {
        if ($key == null) {
            throw new BadFunctionCallException("Key not set");
        }
        if ($value == null) {
            unset ($this->arrValues[$key]);
            return true;
        } else {
            $this->arrValues[$key] = $value;
            return true;
        }
    }

    public function get($key = null)
    {
        if ($key == null) {
            throw new BadFunctionCallException("Key not set");
        }
        if (isset($this->arrValues[$key])) {
            return $this->arrValues[$key];
        } else {
            return null;
        }
    }

And the default.config.php file:

<?php
$this->set('demo', true);

And lastly, the changes to the ConfigTest.php file:

    public function testLoadConfig()
    {
        $config = new Config();
        $this->assertTrue(is_object($config));
        $config->load('default.config.php');
        $this->assertTrue($config->get('demo'));
    }

    /**
     * @expectedException BadFunctionCallException
     */
    public function testFailSettingValue()
    {
        $config = new Config();
        @$config->set();
    }

    /**
     * @expectedException BadFunctionCallException
     */
    public function testFailGettingValue()
    {
        $config = new Config();
        @$config->get();
    }

We’ve not actually finished testing this yet. Not sure how I can tell?

phpunit --coverage-text Tests/ConfigTest.php
PHPUnit 3.6.10 by Sebastian Bergmann.

....

Time: 0 seconds, Memory: 3.75Mb

OK (4 tests, 5 assertions)

Generating textual code coverage report, this may take a moment.

Code Coverage Report
  2012-05-08 18:54:16

 Summary:
  Classes: 0.00% (0/1)
  Methods: 0.00% (0/3)
  Lines:   76.19% (16/21)

@Config::Config
  Methods: 100.00% ( 3/ 3)   Lines:  76.19% ( 16/ 21)

Notice that there are 5 lines outstanding – probably around the unsetting values and using default values. If you use an IDE (like NetBeans) you can actually get the editor to show you, using coloured lines, exactly which lines you’ve not yet tested! Nice.

So, the last thing to talk about is Containers and Dependency Injection. We’ve already started with the Dependency Injection here – that $config->load(‘filename’); function handles loading config files, or you could just bypass that with $config->set(‘key’, ‘value); but once you get past a file or two, you might just end up with a lot of redundant re-loading of config files, or worse, lots of database connections open.

So, this is where Containers come in (something I horrifically failed to understand before).

Here’s a container:

class ConfigContainer
{
  protected static $config = null;

  public static function Load()
  {
    if (self::$config == null) {
      self::$config = new Config();
      self::$config->load();
    }
    return self::$Config;
  }
}

It’s purpose (in this case) is to load the config class, including any dependencies that you may need for that class, and then return that class to you. You could conceivably create a Database container, or a Request container or a User container with very little extra work, and with a few short calls, have a single function for each of your regular and routine sources of processing data, but without preventing you from being able to easily and repeatably test that data – by not going through the container.

Of course, there’s nothing to stop you just having these created in a registry class, or store them in a global from the get-go, but, I am calling these “Better Practices” after all, and these are considered to be not-so-good-practices.

Just as a note, code from this section can be seen at GitHub, if you want to use them at all.

Update 2012-05-11: Added detail to the try/catch exception catching as per frimkron’s comment. Thanks!

php-PDO-MySQL versus php-mysql

PHP and MySQL was the duo that took dynamic web development from /cgi/formmail.cgi to… well, where we are today for me. My first proper experience of working with a web application, as opposed to a static web site with a few perl scripts bolted on was PHPNuke, and I’ve not looked back since.

Almost the first lines of PHP you learn to code are

<?php
mysql_connect("localhost", "root", "");
mysql_use_db("mysite");

Followed closely by mysql_query(), then mysql_fetch_array(), mysl_num_rows(), and if the rest of us are lucky, mysql_real_escape_string().

So you see, for me, it’s really weird to find that even though this is what we’re teaching everyone to use, this weird incantation where we have to wrap every query up in a cotton wool ball of SQL injection protection… there’s an easier way where you don’t have to wrapper everything, which, even better, is more efficient at your SQL server and, if you’ve made the transition to using Classes in your code, will give you an object per-result rather than an array.

Let’s look at some examples of code, and see how we would transition things.

Here’s some rather unoriginal code to get the contents of a user table.

<?php
$db = mysql_connect("localhost", "root", "");
mysql_use_db("mysite");
$sql = "select userid, realname, lastloggedin from users "
     . "where username = '" . mysql_real_escape_string($_POST['username'])
     . "' and password = '" . mysql_real_escape_string($_POST['password']) . "'";
$query = mysql_query($sql);
$data = false;
if (mysql_num_rows($query) == 1) {
  $data = mysql_fetch_array($query);
  echo "Hello {$data['realname']}, your userid is {$data['userid']} and "
     . "you last logged in at " . date("H:i:s", strtotime($data['lastloggedin']))
     . " on " . date("d-M-Y" , strtotime($data['lastloggedin']));
}

There, now there’s not much wrong with that (ignoring the security hole of having your passwords in cleartext) is there? All pretty readable, right? Well… what happens if your database is down, how do you handle that in some kind of sensible way? You need to wrap some code around the mysql_connect to show it actually connected. What about if someone dropped the database by mistake, or you connected to the wrong database server and that table wasn’t there, now you’re wrappering the use_db line too. Did you make a typo in the SQL? Are you sure you didn’t miss a mysql_real_escape_string in there somewhere…

Here’s the same thing in PDO, protected against all (again, except the cleartext password) of those things.

<?php
try {
  $db = new PDO("mysql:host=localhost;dbname=mysite", "root", "");
  $db->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
  $sql = "select userid, realname, lastloggedin from users where username = ? and password = ?";
  $query = $db->prepare($sql);
  $query->execute(array($_POST['username'], $_POST['password']));
  $data = $query->fetch();
  if ($data != false) {
    echo "Hello {$data['realname']}, your userid is {$data['userid']} and "
       . "you last logged in at " . date("H:i:s", strtotime($data['lastloggedin']))
       . " on " . date("d-M-Y" , strtotime($data['lastloggedin']));
  }
} catch (PDOException $e) {
  error_log("User unable to login: " . $e->getMessage());
}

So, let’s look at what transforms this into something much more interesting: $query->fetchObject();

<?php
try {
  $db = new PDO("mysql:host=localhost;dbname=mysite", "root", "");
  $db->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
  $sql = "select userid, realname, lastloggedin, username, password " 
       . "from users where username = ? and password = ?";
  $query = $db->prepare($sql);
  $query->execute(array($_POST['username'], $_POST['password']));
  $data = $query->fetchObject('user');
  if ($data != false) {
    echo $data;
  }
} catch (PDOException $e) {
  error_log("User unable to login: " . $e->getMessage());
}

class user
{
  // Columns from the database
  protected $userid = null;
  protected $realname = null;
  protected $lastloggedin = null;
  protected $username = null;
  protected $password = null;
  // Processed Data
  protected $transformed_lastloggedin = null

  public function __construct()
  {
    if ($this->lastloggedin != null) {
      $this->transformed_lastloggedin = date("H:i:s", strtotime($this->lastloggedin)) 
                             . " on " . date("d-M-Y" , strtotime($this->lastloggedin);
    }
  }

  public function toString()
  {
    echo "Hello {$this->realname}, your userid is {$this->userid} and " 
       . "you last logged in at {$this->transformed_lastloggedin}";
  }
}

I’m starting to stretch the analogy here a little, but the important things here are:

  1. The __construct() function of the class is run after the data is inserted into it, meaning you can post-process any data you receive from the database without needing any new functions. Arguably, you could make the construct function accept the same values you’d receive from the database, and process those in the same way.
  2. The toString() function means you can mangle the content you want to output, without having to apparently run another function on the class you’ve just created.
  3. By using classes you get to do wonderful things like Unit Testing (this is a topic to follow), have automatic code documentation (err… to follow), reduce code duplication by extending classes and check on that, using “Copy and Paste Detection”.

PHP Better Practices – an introduction

I think it would be fair to say that I’m no PHP expert. I think it would also be fair to say that my PHP is far from perfect… or excellent. Hell, sometimes even saying it’s good can be a stretch!

But, it would be fair to say that I’m learning how to do things better than I was doing them before, and so, I’m going to start jotting some of these things down in this new category.

Don’t consider this the best way to do things – just think, if you’ve found a post in this series and you’re wondering whether it’s the right thing to do, well, I’ve been there too, and that’s why I wrote about it!

Dependency Hell – and it’s not my package manager to blame

I’m writing a web service for conferences. I’ve been writing it, on and off for 3 years, and I think it would be fair to say the coding reflects my learning over those three years. The script is written in PHP. It has a MySQL backend. It has undergone a lot of changes.

In the beginning, was a concept. The concept was to have a digital timetable. One where you could register your interest in a talk, and the busiest talks got the biggest rooms. It should use any tech the user had to use the system including SMS. It would not expect you to have a user name and password combo (heaven forbid!), but would use OpenID.

The concept was implemented and demo’d at an event 2 years ago, and a friend [1] asked where the API was. “API?” I replied, feeling somewhat foolish, “It doesn’t have an API“.

[1] Lorna Jane Bowman (neé Mitchell aka @lornajane)

I realised the foolishness of my coding when discussing this issue [2]. “It’s dead simple, just create another view which returns just JSON” said someone. “View? I don’t know what you mean“… “Model, View, Controller? MVC – it’s a pretty common pattern”. “Oh no” I replied, “my project has inline PHP. It just seemed simpler that way.” “Well, how about you add a toString() function to your classes, and use that to render the output?” “Classes. Another thing my project doesn’t have. Sorry. Can’t do that either.

[2] With Lorna again, and adding Katherine Reeve (aka @BinaryKitten)

Did you ever get that slightly sinking feeling that maybe you’re making a fool of yourself?

“Well, there are lots of benefits to Classes in PHP, not least of which that you can use PHP CodeSniffer to enforce your coding standards” I start to laugh a little at this point “and you can use PHP Documenter to create all your developer documentation” which shut me right back up again “you can use PDO to create your objects from your database results” wow… mind blown – I’ve jumped on this little trick of a pony… “and of course, you can’t really do unit testing without classes”. Uh oh. “What’s unit testing?” “It’s a set of automated tests you can run against your code every time you commit to your version control software” whew! I’m using that at least! “to make sure that the code you’re committing hasn’t broken anything”.

Fast forward to this week, and I asked on Facebook whether anyone could teach me how to do Unit Testing. See, I’ve managed to cobble together my own MVC – I have classes called “Object_” which are the Models, the controller is my routing script – otherwise known as index.php, and then the Views are templates in Smarty, or just a json_encode((array) $object) [3] for my API. I have my own set of tests – not unit tests, it’s a script called “Test.sh” which runs PHP against the file (to make sure that the pages don’t have brackets missing or similar), then runs PHP Code Sniffer against it, and finally, if all the files are OK, it then runs PHPDoc against the whole mass of it.

[3] As the Apple iPhone/iPad adverts say – some sequences shortened

So, one of the books suggested to me, again by the lovely Lorna Jane, was The Grumpy Programmer’s Guide To Building Testable PHP Applications which mentioned that it’s much easier to do unit testing if you set up dependency injection. Note, I’m still not yet doing Unit testing here.

Woah. What the hell is Dependency Injection? Well, fortunately, there were code examples in that book. Oh boy, were there code examples. So let’s look through this idea. Instead of writing

$stuff = new MyStuff();

class MyStuff()
{
    function construct()
    {
        $this->db = mysql_connect('localhost', 'username', 'password');
        mysql_use_database('production', $this->db);
    }
}

You could instead write this:

$db = mysql_connect('localhost', 'username', 'password');
mysql_use_database('production', $this->db);
$stuff = new MyStuff($db);

class MyStuff()
{
    function construct($db = null)
    {
        $this->db = $db;
    }
}

So, this now means that in your testing framework, you could pass it a non-production database, or a testing database, or, well, anything that relies on something outside your script.

I did a bit of digging around to find some other examples of dependency injection code that might be a bit easier to use, which is to say, something so I don’t need to amend all my constructor functions.

I found this slideshare from a talk at PHP Barcelona about dependency injection which says you can do dependency injection like this:

$thing = new MyClass($dependency);

OR

$thing = new MyClass();
 $thing->setDependency($dependency);

OR

$thing = new MyClass();
 $thing->dependency = $dependency;

but somewhat weirdly, it also says that you can create a container class which holds your dependencies, and refer to that later – and that this isn’t a singleton. Sadly, I didn’t understand all that code fully (and have gone on to file a bug for the PHP documentation for the functions I didn’t understand to help people who follow behind with it!), but (and I’ve copied this verbatim from the slideshare) essentially, it looks like this:

class Container { protected $values = array(); function __set($id,$value) {  $this->values[$id] = $value;  }  function __get($id) {  if (!isset($this->values[$id])) {  throw new InvalidArgumentException(sprintf('Value "%s" is not defined.', $id));  }  if (is_callable($this->values[$id])) {  return $this->values[$id]($this);  } else { return $this->values[$id];  }  }  function asShared ($callable) {  return function($c) use ($callable) {  static $object; if (is_null($object)) {  $object=$callable($c);  }  return $object; }; } } $container = new Container(); $container->session_name='SESSION_ID'; $container->storage_class='SessionStorage'; $container->user = $container->asShared( function($c) {  return new User($c->storage);  } ); $container->storage = $container->asShared(  function($c) {  return new $c->storage_class($c->session_name); } );

Now, I have to be honest, this confuses the hell out of me. How on earth do I use this in my code? I’ve been doing this in my code thus far:

class Object_User{
  protected $intUserID    = null; // Obtained by the getCurrentUser() function
  protected $strUsername  = null;
  protected $hashPassword = null; // More of these, depending on the SQL

  function getCurrentUser() { // Called as $user = Base_User::getCurrentUser();
 $objCache  = Base_Cache::getHandler(); // A singleton to "cache" any data we've pulled to save getting it repeatedly
    if (
          isset($objCache->arrCache['Object_User']['current'])
          && $objCache->arrCache['Object_User']['current'] != false
        ) {
      return $objCache->arrCache['Object_User']['current'];
    }
    $arrRequest  = Base_Request::getRequest(); // Returns array of parsed request data
    $objDatabase = Base_Database::getConnection(); // Returns a PDO object

    $sql = "SELECT * FROM users WHERE strUsername = ? and hashPassword = ?";
    $query = $db->prepare($sql);
    $query->execute(array($request['username'], $request['password'])); $result = $query->fetchObject('Object_User');
    if ($result != false) {
      $objCache->arrCache['Object_User']['id'][$result->getVal('intUserID')] = $result;
      $objCache->arrCache['Object_User']['current'] = $result;
    }
    return $result;
  }

  function getVal($key) { // Return protected variables
    if (!isset($this->$key)) {
      return false;
    }
    return $this->$key;
  }
}

I know that singleton methods are considered “Bad” because they’re (apparently) difficult to unit test, but I would have thought that it would have been pretty straightforward to create a singleton class which holds all the dependencies (see following)

class Base_Dependencies
{
  protected static $handler = null; protected $arrDependencies = array();   protected function GetHandler() {
    if (self::$handler == null) { self::$handler = new self(); }
    return self::$handler;
  }

  function set($key, $dependency) {
    $handler = self::GetHandler();
    $handler->arrDependencies[$key] = $dependency;
  }

  function get($key) {
    $handler = self::GetHandler();
    if (isset($handler->arrDependencies[$key])) {
      return $handler->arrDependencies[$key];
    } else {
      return false;
    }
  }

  function unset($key) { // Only used for Unit Testing, I would imagine
    $handler = self::GetHandler();
    if (isset($handler->arrDependencies[$key])) {
      unset($handler->arrDependencies[$key]);
    }
  }
}

Doing it this way means I can, from, for example, my database class, which is currently a singleton, say instead:

function GetConnection() {
  $db = Base_Dependencies::get("Database");
  if ($db != false) {
    return $db;
  }
  $config = Base_Config::getAllConfig();
  $db = new PDO($config['DSN'], $config['DB_User'], $config['DB_Pass']);
  Base_Dependencies::set("Database", $db);
  return $db;
}

Is this wrong? Is this just not best practice? Given the above, how can I fix my dependencies in such a way that the poor schmuck who wants to commit a patch can figure out what they’re sending? Or do I just need to fix how my head works with this stuff? If it’s the latter, can someone provide some samples of what I’m doing wrong?

Thanks for reading this mammoth post!

Getting my head around coding Object Orientated PHP

I’ve been writing two open source projects over the last couple of years. My code has never really been particularly great, but I’ve been trying to learn how to improve my code and over the last few months, I’ve been really trying to polish up my coding skills.

A few months back, I attended a series of fantastic sessions at PHPNW about using Unit Testing, PHP CodeSniffer and phpDocumentor, and how these can be incorporated into Object Orientated code (or in fact, requiring Object Orientated code to implement them).

So, I went back into my main projects and started to look at how I could fix the code to start adopting these tools.

So, the first thing I needed to do was to start thinking about the structure. CCHits.net (which is the “big project” I’ve been working on recently) has several chunks of data, and these are:

User
Track
Artist
Show
ShowTrack
Vote
Chart

Each of these have been broken down into three “things” – The Object itself, a “Broker” which finds all the relevant objects, and a class to create new items, so let’s start with a user class. We’ll define a few properties in the initial class creation.

class UserObject
{
    protected $intUserID = 0;
    protected $strOpenID = "";
    protected $strCookieID = "";
    protected $sha1Pass = "";
    protected $isAuthorized = 0;
    protected $isUploader = 0;
    protected $isAdmin = 0;
    protected $datLastSeen = "";
}

By setting these as protected, it stops me from directly setting or accessing these variables from outside of the class – instead I want to do it from a function, so let’s add those in (I’ll just do one – assume these will be copied on to the rest of the values).

class UserObject
{
    protected $strOpenID = "";

    function set_strOpenID($strOpenID = "") {
        if ($strOpenID != $this->strOpenID) {
            $this->strOpenID = $strOpenID;
        }
    }

    function get_strOpenID() {
        return $this->strOpenID;
    }
}

In the set_ functions, we already do a little bit of error checking here (is the value already set to this?) but we could add other things like, on the integer items, is it actually an integer, with the boolean values ($is[SOMETHING]), make sure it’s set to 1 or 0 (or true/false).

Now, let’s add some documentation to this:

/**
 * CCHits.net is a website designed to promote Creative Commons Music,
 * the artists who produce it and anyone or anywhere that plays it.
 * These files are used to generate the site.
 *
 * PHP version 5
 *
 * @category Default
 * @package  CCHitsClass
 * @author   Jon Spriggs 
 * @license  http://www.gnu.org/licenses/agpl.html AGPLv3
 * @link     http://cchits.net Actual web service
 * @link     http://code.cchits.net Developers Web Site
 * @link     http://gitorious.net/cchits-net Version Control Service
 */
/**
 * This class deals with user objects
 *
 * @category Default
 * @package  Objects
 * @author   Jon Spriggs 
 * @license  http://www.gnu.org/licenses/agpl.html AGPLv3
 * @link     http://cchits.net Actual web service
 * @link     http://code.cchits.net Developers Web Site
 * @link     http://gitorious.net/cchits-net Version Control Service
 */
class UserObject
{
    /**
     * This is the Setter function for the strOpenID value
     *
     * @param string $strOpenID The value to set
     *
     * @return void There is no response from this function
     */
    function set_strOpenID($strOpenID = "") {
        // Do stuff
    }
}

So, let’s make it do stuff with the database. Firstly, we need to set up a connection to the database. I’ll be using PDO (PHP Database Object, I think) to set up the connection (I’ll show why in a minute).

So, here’s another class – Database, this time it’s using a singleton factory (which is to say, while it may exist many times in the code, it’ll only ever have one connection open at once) – note it’s got hard-coded authentication details here – this isn’t how it actually is in my code, but this way it’s a bit more understandable!

class Database
{
    protected static $handler = null;
    protected $db = null;

    /**
     * This function creates or returns an instance of this class.
     *
     * @return object $handler The Handler object
     */
    private static function getHandler()
    {
        if (self::$handler == null) {
            self::$handler = new self();
        }
        return self::$handler;
    }

    /**
     * This creates or returns the database object - depending on RO/RW requirements.
     *
     * @return object A PDO instance for the query.
     */
    public function getConnection()
    {
        $self = self::getHandler();
        try {
            $self->rw_db = new PDO('mysql:host=localhost;dbname=cchits', 'cchits', 'cchits', array(PDO::MYSQL_ATTR_INIT_COMMAND => "SET NAMES utf8"));
            return $self->rw_db;
        } catch (Exception $e) {
            echo "Error connecting: " . $e->getMessage();
            die();
        }
    }
}

So, now let’s create yet another class, called GenericObject. This will be re-used in all of the “Object” classes to perform our main database calls. Try to move as much code up to your highest level objects – so you could have a “validateBoolean($value)” function that is used any time you set or get a boolean value here… we even mentioned something like this above!

class GenericObject
{
    protected $arrDBItems = array();
    protected $strDBTable = "";
    protected $strDBKeyCol = "";
    protected $arrChanges = array();

    /**
     * Commit any changes to the database
     *
     * @return boolean Status of the write action
     */
    function write()
    {
        if (count($this->arrChanges) > 0) {
            $sql = '';
            $strDBKeyCol = $this->strDBKeyCol;
            $values[$strDBKeyCol] = $this->$strDBKeyCol;
            $values = array();
            foreach ($this->arrChanges as $change) {
                if ($sql != '') {
                    $sql .= ", ";
                }
                if (isset($this->arrDBItems[$change])) {
                    $sql .= "$change = :$change";
                    $values[$change] = $this->$change;
                }
            }
            $full_sql = "UPDATE {$this->strDBTable} SET $sql WHERE {$this->strDBKeyCol} = :{$this->strDBKeyCol}";
            try {
                $db = Database::getConnection();
                $query = $db->prepare($full_sql);
                $query->execute($values);
                return true;
            } catch(Exception $e) {
                return false;
            }
        }
    }

    /**
     * Create the object
     *
     * @return boolean status of the create operation
     */
    protected function create()
    {
        $keys = '';
        $key_place = '';
        foreach ($this->arrDBItems as $field_name=>$dummy) {
            if ($keys != '') {
                $keys .= ', ';
                $key_place .= ', ';
            }
            $keys .= $field_name;
            $key_place .= ":$field_name";
            $values[$field_name] = $this->$field_name;
        }
        $full_sql = "INSERT INTO {$this->strDBTable} ($keys) VALUES ($key_place)";
        try {
            $db = Database::getConnection();
            $query = $db->prepare($full_sql);
            $query->execute($values);
            if ($this->strDBKeyCol != '') {
                $key = $this->strDBKeyCol;
                $this->$key = $query->lastInsertId();
            }
            return true;
        } catch(Exception $e) {
            return false;
        }
    }

    /**
     * Return an array of the collected or created data.
     *
     * @return array A mixed array of these items
     */
    function getSelf()
    {
        if ($this->strDBKeyCol != '') {
            $key = $this->strDBKeyCol;
            $return[$key] = $this->$key;
        }
        foreach ($this->arrDBItems as $key=>$dummy) {
            $return[$key] = $this->$key;
        }
        return $return;
    }
}

Any protected functions or variables are only accessible to the class itself or classes which have been extended from it (referred to as a child class). We need to make some changes to our UserObject to make use of these new functions:

class UserObject extends GenericObject
{
    // Inherited Properties
    protected $arrDBItems = array(
        'strOpenID'=>true,
        'strCookieID'=>true,
        'sha1Pass'=>true,
        'isAuthorized'=>true,
        'isUploader'=>true,
        'isAdmin'=>true,
        'datLastSeen'=>true
    );
    protected $strDBTable = "users";
    protected $strDBKeyCol = "intUserID";
    // Local Properties
    protected $intUserID = 0;
    protected $strOpenID = "";
    protected $strCookieID = "";
    protected $sha1Pass = "";
    protected $isAuthorized = 0;
    protected $isUploader = 0;
    protected $isAdmin = 0;
    protected $datLastSeen = "";

    function set_strOpenID($strOpenID = "") {
        if ($this->strOpenID != $strOpenID) {
            $this->strOpenID = $strOpenID;
            $this->arrChanges[] = 'strOpenID';
        }
    }
}

Notice I’ve stripped the “phpDoc” style comments from this re-iteration to save some space! In the real code, they still exist.

Now we have an object we can work with, let’s extend it further. It’s probably not best practice, but I find it much more convenient to create new objects by extending the UserObject into a new class called NewUserObject. Notice once we’ve set our database items, we run the create(); function, which was previously defined in the GenericObject class.

class NewUserObject extends UserObject
{
    public function __construct($data = "")
    {
        if (strpos($data, "http://") !== false or strpos($data, "https://") !== false) {
            $this->set_strOpenID($data);
        } elseif ($data != "") {
            $this->set_sha1Pass($data);
        } else {
            if (isset($_SERVER['HTTP_X_FORWARDED_FOR'])) {
                $cookie_string = $_SERVER['HTTP_X_FORWARDED_FOR'];
            } else {
                $cookie_string = $_SERVER['REMOTE_ADDR'];
            }
            $cookie_string .= $_SERVER['HTTP_USER_AGENT'];
            $cookie_string .= $_SERVER['HTTP_ACCEPT_LANGUAGE'];
            $cookie_string .= $_SERVER['HTTP_ACCEPT_ENCODING'];
            $cookie_string .= $_SERVER['HTTP_ACCEPT_CHARSET'];
            $this->set_strCookieID(sha1sum($cookie_string));
        }
        $this->datLastSeen = date("Y-m-d H:i:s");
        $_SESSION['cookie'] = sha1($cookie_string);
        return $this->create();
    }
}

Using the code $user = new NewUserObject($login_string); we can create a new user, but how about retrieving it.

This is where the reason I’m loving PDO comes into play. See, before PDO, when you did a database request, you might have had something like this:

$db = mysql_connect("localhost", "root", "");
if ($db == false) {
    die ("Failed to connect to the Database Server");
}
if (! mysql_select_db("database")) {
    die ("Failed to select the database");
}
$intUserID = mysql_real_escape_string($_GET['intUserID']);
$sql = "SELECT * FROM users WHERE intUserID = '$intUserID' LIMIT 1";
$qry = mysql_query($sql);
if (mysql_errno() > 0) {
    echo "Failed to make Database Call: " . mysql_error();
} else {
    if (mysql_num_rows($qry) == 0) {
        echo "Failed to retrieve record 1";
    } else {
        $row = mysql_fetch_array($qry);
    }
}

Now, let’s assume you’re looking for a few users? You need to do more of those mysql_real_escape_strings and mysql_num_rows() and mysql_fetch_array()s to get your data out – it’s far from clean and clear code.

How about this instead?

try {
    $db = Database::getConnection();
    $sql = "SELECT * FROM users WHERE intUserID = ? LIMIT 1";
    $query = $db->prepare($sql);
    $query->execute(array($_GET['intUserID']));
    $row = $query->fetch();
} catch(Exception $e) {
    echo $e;
    $row = false;
}

The most complicated bit here is that you’re having to prepare your SQL query and then tell it what to get. The reason we do that is that PDO will automatically ensure that anything being passed to it using the ? is sanitized before passing it into the query. If you look back at the GenericObject class we created earlier, it uses something like this there too, except there (if you work out what it’s doing) it prepares something like INSERT INTO users (intUserID) VALUES (:intUserID); and then executes it with the values like this: array(‘:intUserID’=>1) and with this you can have some very complex statements. Other code you might spot while mooching through CCHits (if you decide to) looks like this:

$sql = "UPDATE IGNORE votes SET intTrackID = ? WHERE intTrackID = ?; DELETE FROM votes WHERE intTrackID = ?";
$query = $db->prepare($sql);
$query->execute(array($intNewTrackID, $intOldTrackID, $intOldTrackID));

By wrapping it all up in a try/catch block, you can get your error dumped in one place, including a stack trace (showing where the issue turned up). You don’t need to check for mysql_num_rows – if the row doesn’t exist, it’ll just return a false. Sweet.

Where it gets REALLY nice, is that if you swap $row = $query->fetch() with $object = $query->fetchObject(‘UserObject’); and it’ll create the object for you!

So, now, with the function getUser (visible at https://gitorious.org/cchits-net/website-rewrite/blobs/master/CLASSES/class_UserBroker.php) it’ll try to return a UserObject based on whether they’re using OpenID, Username/Password or just browsing with a cookie… and if it can’t, it’ll create you a new user (based on the above criteria) and then return you that UserObject.

The last thing to add, is that I wrapped up all the nice phpDocumentor, PHP CodeSniffer functions, plus wrote a script to check for missing or incorrectly labelled functions across a suite of classes. These sit in https://gitorious.org/cchits-net/website-rewrite/trees/master/TESTS if you want to take a look around :)

EDIT 2011-08-25: Correcting some errors in the code, and to adjust formatting slightly.
EDIT 2012-05-05: Changed category, removed the duplicated title in the top line, removed some whitespace.