Turning this into a class

Question

Issue 1 I have this code below, and I keep hearing people talking about using loads of classes. Is there any merit to me putting the code below as a class (I'm already turning the data into objects) and how would I go about doing it? (I'm new to OOP PHP).

try {

    $connection = new PDO(DATA, USER, PASSWORD);

    $connection->setAttribute(PDO::ATTR_EMULATE_PREPARES, false);

    $connection->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);

} catch (PDOException $error) {

    PDOcrash($error);

}

// Scrubbing remember data
$_POST['remember'] = (int)$_POST['remember'];

// Query the database for the unique salt
$query = $connection->prepare("SELECT id, salt FROM {$_SESSION['environment']->database}.system_user WHERE username = :username LIMIT 1");

$query->execute(array(':username' => $_POST['username']));

$security = $query->fetch(PDO::FETCH_OBJ);

$query->closeCursor();

// Form the hash using sha1 alrorithm
$_POST['password'] = sha1(sha1($security->salt) . sha1($_POST['password']));

$query = $connection->prepare("SELECT id, centre, reference, first_name, last_name FROM {$_SESSION['environment']->database}.system_user WHERE username = :username AND password = :password");

$query->execute(array(':username' => $_POST['username'], ':password' => $_POST['password']));

$_SESSION['user'] = $query->fetch(PDO::FETCH_OBJ);

$query->closeCursor();

Issue 2

For this code block:

try {

    $connection = new PDO(DATA, USER, PASSWORD);

} catch (PDOException $error) {

    PDOcrash($error);

}

I am using constants to define my connection details, but they only seem to work if I define them directly on the page. I thought a constant would stay a constant through the system. If I move my define() functions to an earlier encountered page, they don't work. Does this mean the scope of a constant is a single page? I also heard that you can define connection info in some separate apache document and this is a more secure method. How do I go about doing this?

Answer 1

There is nothing wrong with not writing OOP code perse, but the benefits you will gain by using proper OOP style coding can be very useful. It will make your code easier to read, maintain, debug and test!

PHPs variables (beside a superglobal here and there) and constants are only valid per request. For what you are trying to do I would not even use a constant. Remember a constant created by define() is global. If you hear / see global anywhere you know you are probably doing OOP wrong in most cases.

What I would do in your case (if you want to go the OOP route) is create a bootstrap file where you setup the database connection. After that you can easily inject the database object into other classes / methods that need them.

Another thing please please please use a password hashing libary. This would prevent you from screwing it up. The password lib I just linked to will be implemented in PHP5.5. But before you are running that version please use that library, because your hashing is really not that secure. sha1 is not good for hashing password, because it is meant to be fast. At least use bcrypt, but again please just use the linked password library, because it is too easy to screw up the hashing of passwords.

Also: just in case you are using the MySQL PDO driver disable emulated prepared statements.

Finally to actually answer your question: no there is no benefit of simply moving that code to a class. If you do it without thinking about what you are doing or SOLID principles your class is nothing more than a fancy namespace thingy. Simply moving code into a class doesn't make it OOP by magic.

UPDATE

Bootstrap file

A bootstrap file is a file which is included with every request made. It can be used to setup things you application will need in order to function. An example of this (for most (web)applications) is a database connection. But you can also use the bootstrap file to setup some PHP settings for example like enabling error reporting. And the very useful autoloading.

In my applications the bootstrap file is always the second file which get accessed. The first file on every request is a simple index.php file in the document root which does only one thing: loading the bootstrap file. All other PHP files will be kept outside the document root. This will prevent your PHP code showing up for whatever reason. Server misconfiguration or whatever. This setup looks a bit like the following directory structure:

/src
/app
  /public
    /index.php
  /bootstrap.php

And the only thing the index.php file in the doc root contains is:

<?php

require __DIR__ . '/../bootstrap.php';

After that you may have a bootstrap file which may look something like this:

<?php

error_reporting(E_ALL);
// note that for production we would disable the displaying of errors and 
// enable logging of errors
ini_set('display_errors', 1);

require_once __DIR__ . '/../src/bootstrap.php'

// btw also don't forget to set the encoding
$dbConnection = new PDO('DSN', 'username', 'password');

// now we can simply call a class and pass the database object as an argument
$user = new User($dbConnection);
$user->changePassword('new password');

// because we still have the database object we can simply reuse it for some other class
$page = new Page($dbConnection);
echo $page->render('home');

As you can see we can just keep passing the database connection to the classes / methods that need it. This enables us to easily swap the database connection for something else to speed up our unit tests. This loose coupling also makes sure we could easily swap the database object for something else in case we need some other storage mechanism.

If you want to see most of the above in practice you can have a look at the project I am currently working on. In case you still are wondering why OOP only has benefits you should watch these series:

http://www.g-truc.net/post-0182.html

Answer 2

Converting existing code to a class is most useful when you plan on reusing the code, or are able to refactor the code to take advantage of several instances of a single class. In this example, it's impossible to tell if it would really benefit you to do that, but my hunch is that it's not.

Classes are only useful if they make your code more clear and easy to maintain. With languages like PHP where you often write scripts to handle a one off thing that is completely unlike any other code in your application, there may be no point to converting it to a class.

As for constants, yes, they're only valid for the page they're defined in. PHP's engine only inspects the pages run through it, so it doesn't know about constants you've defined in pages elsewhere when it parses and executes a page. The solution to this is related to your DB credentials question.

One option there is to define an include file that has these constants defined in them, but is then included in each page that needs them. Something like this:

page1.php

define(SOMETHING, 'value');

page2.php

require('page1.php');
echo SOMETHING;

By requiring (including, and failing if the file isn't found) page1.php, it is treated as part of the current page when being parsed and executed and so any defines contained in it will apply to later code in that page.

Answer 3

Objects

Objects, most basically, are simply collections of common functions and variables that you can package into individual units to make your life easier. Your code seems a little repetitive with regard to your database queries, and you might benefit from writing/using a wrapper class around PDO to simplify your code.

Personally, I've been using something like this for a couple years now, and it's served me well. You might find it useful as a drop-in replacement, or an example to build your own.

Credentials

The way I usually define my database credentials is in an array in a dedicated file, and immediately after setting up the connection I unset() the array. ie:

inc.db_cfg.php

<?php
$conn_info = array(
  'hostname' => 'mysql.domain.com',
  'username' => 'bob',
  'password' => '1234',
  'dbname' => 'mydb'
);

index.php

<?php
require('inc.db_cfg.php');
$uri = sprintf("mysql:host=%s;dbname=%s", $conn_info['hostname'], $conn_info['dbname']);
$this->dbh = new PDO($uri, $conn_info['username'], $conn_info['password']);
unset($conn_info, $uri);

The main reason you want to define your database credentials in variables instead of directly in the constructor is that if the connection fails and your script throws an Exception or error the stack trace/error message can potentially print out the raw line of code containing your username, password, hostname, and other bits you'd most likely want to stay private.

Also, you want to be sure that your credentials are stored in a file type that won't be output in raw text someone requests it. For example, back in the "early days" people would simply name their include files db_cfg.inc or something similar. You could have include('db_cfg.inc'); in your code with no problem, however the default configuration of Apache simply serves the file as plain text if someone were to request http://mysite.com/db_cfg.inc.

Lastly, it's not particularly required to unset() the credentials, but it doesn't hurt and satisfies my paranoia.

Password Hashing

SHA1 has been fundamentally broken for some time, you can use crypt() to access better algos like SHA256 or SHA512 or snag a bcrypt implementation from out there on the interwebs.