Usage of try/catch and database transactions

Question

I am using try/catch syntax in combination with a database transaction to (hopefully) prevent partial registrations.

I am wondering if I'm on the right track, and what ways, if any, I can improve my code.

Please be advised that __construct() is passed an array of unsanitized post values that have been organized into an array after the controller has checked the CSRF tokens, and this object is constructed. Once construction is finished without error, a try catch block on add_user(). I would show some of the other code, but its a proprietary design pattern that closely emulates angular JavaScript (with far superior performance, but sacrificing readability and longer development time). Someday I'll opensource the design pattern.

Here is an excerpt from my class AddNewUser that illustrates the question:

function __construct($user)
{
    if(!is_array($user)) {
        incident('possible hack attempt','registration');
        throw new \Exception('Invalid data received');  
    }
    if($user['termsofservice'] !== 'agree') {
        incident('non ajax submission','registration');
        throw new \Exception ('Must agree to terms of service');
    }
    if($user['privacypolicy'] !== 'agree') {
        incident('non ajax submission','registration');
        throw new \Exception ('Must agree to privacy policy');
    }
    if(usernameExists($user['username'])){
        incident('possible user enumeration','registration');
        throw new \Exception('Username Taken');
    }
    if(emailExists($user['email'])) {
        incident('possible email enumeration','registration');
        throw new \Exception('Email Taken');
    }
    if(minMaxRange(3,25,$user['username'])) {
        throw new \Exception('Username must be 3 to 25 charachters in length') ;
    }
    if(minMaxRange(8,50,$user['password'])) {
        throw new \Exception('Password must be 8 to 50 charachters in length');
    }
    $this->user_name        = security($user['username']);
    $this->user_pass        = generateHash($user['password']);
    $this->user_email       = $user['email'];
    $this->user_ip          = get_ip_address();
    $this->verification     = generateActivationToken();
    $this->signupstamp      = time();
    $this->user_agent       = security($user['agent']);
    $this->user_active      = 0;
    $this->user_verified    = 0;
    $this->terms_of_service = security($user['termsofservice']);
    $this->privacy_policy   = security($user['privacypolicy']);
}

public function adduser()
{
    global $db,$cfg;
    try{
        $query = $db->query("START TRANSACTION;");
        $stmt = $db->prepare("INSERT INTO users (u_name, u_pass, u_email, u_verified, u_ip, u_active, u_verification, u_signup_stamp, u_agent) VALUES (?,?,?,?,?,?,?,?,?)");
        $stmt->bind_param('sssisisis',$this->user_name,$this->user_pass,$this->user_email,$this->user_verified,$this->user_ip,$this->user_active,$this->verification,$this->signupstamp,$this->user_agent);
        $stmt->execute();
        $stmt->close();
        $stmt = $db->prepare("INSERT INTO termsofservice (ip_addr,u_name,u_agent,answer,timestamp) VALUES (?,?,?,?,?)");
        $stmt->bind_param('ssssi',$this->user_ip,$this->user_name,$this->user_agent,$this->terms_of_service,$this->signupstamp);
        $stmt->execute();
        $stmt->close();
        $stmt = $db->prepare("INSERT INTO privacypolicy (ip_addr,u_name,u_agent,answer,timestamp) VALUES (?,?,?,?,?)");
        $stmt->bind_param('ssssi',$this->user_ip,$this->user_name,$this->user_agent,$this->privacy_policy,$this->signupstamp);
        $stmt->execute();
        $stmt->close();
        try{
            $mail = new Email('registration',$this->user_email,$this->user_name,'Important: Account Activation',$cfg['email']['no-reply']);
            $query = $db->query("COMMIT;");
        }
        catch(Exception $e){
            incident('smtp unreachable','registration');
            throw new \Exception('Unable to send activation email');
        }
        return true;
    }
    catch(Exception $e) {
        incident('sql problems','registration');
        throw new \Exception('An unknown Error Occured');
    }
}

Answer 1

Your use of try and catch looks good. It is nice to see that you're aware of exceptions and that you want to handle them appropriately. COMMIT is a nice feature here.

Database Design

Redundant data: You are saving the IP address, user agent and signup stamp in all three databases users, termsofservice and privacypolicy. Options:

1. Maintain one table only with all of the fields. After all, the additional information in the last two databases are just the values privacy_policy and terms_of_service.
2. Keep two tables: one with the user name and "normal" user information, and another table with control/meta information, e.g. if the user agreed to your privacy policy, to the terms of service or if he is verified. Since you have a lot of "control information" it might be desirable to separate them from user information like the e-mail, so that you don't have to always juggle all of the data the when you just want some basic details about someone.

When managing multiple tables it is important to avoid as much redundancy as possible. It will make your database a lot more maintainable and you can reduce the potential for errors (say, if you forget to update some information in one table). Evidently, you'll also save space.

Rollback: I really like your idea of using COMMIT only at the end if everything went well. However, consider using ROLLBACK in the case that a problem occurred, i.e. in your catch blocks. It does the opposite of COMMIT and definitely makes sure that the changes won't be written.

Conceptional

OOP: Classes usually don't model actions but rather things – nouns. I feel that it might be smarter to have a User class holding the e-mail etc. and a DatabaseHandler (or similar) which will talk to your database.

Then you could pass a User object to be registered: dbHandler->addUser(userObject);.

Identical User Names: Do you check that the same user name can't be registered multiple times? Varying capitalization should be taken into account, and a user should be able to log in with any variation of capitalization. In other words, if I join as User I want to be able to log in as user, too.

Avoid Writing to DB Unnecessarily: Is it really smart to write to the database if the user hasn't agreed to your terms of service or privacy policy? You might be better off checking that these things are OK beforehand. Then you won't need to write these details to the database and you won't have any "ghost users" lingering in your database who didn't agree to using your service, which doesn't seem of much use.

User Feedback: This is beyond the current scope, but is the user properly informed that he couldn't be registered if there was a problem with the database or with the e-mail creation?

General Programming

Return Value vs. Exceptions: You return true if the method could be executed properly; otherwise, an exception is thrown. I think here it would be best to either just stick with exceptions (if an exception is thrown, something happened, otherwise return nothing) or to stick to a routine where you return true or false (exceptions seem better here so you can differentiate more).

Handling Exceptions: Related to the point above, I'm not sure if catching an error and just throwing "Unknown error" to the invoking function is really very helpful. Ideally, you could do something sensible with the caught exception $e or at least consider its message ($e->getMessage()). (Note that some exceptions may hold a little more information than you'd want to disclose to your users, so maybe it's best to log the exception's message, rather than passing the specific exception up.)

Global Variables: Depending on the scope and seriousness of your project, you can be lax about using a handful of globals (I will probably get shot for this) but if you want a clean, expandable application, you should better shy away from using them. If you had a User and DatabaseHandler class, the latter class would already hold $dbh and would just be waiting for user data to come in. This would be clean and you would not have to worry about making various implementation details (like $dbh) available globally as they would be nicely contained in their respective classes.

Variable Names: The convention for class names and class fields is camelCase, e.g. DatabaseHandler as class or ipAddress as variable. Here again (hoping not to get yelled at) if you prefer using underscores and you're just working on a little project on your own, I think that the most important thing is consistency. (Somewhat related, we saw that multiple rows with the same data is not a good idea, but having one named u_ip and another one ip_addr definitely doesn't make it better.)

Password Handling

Yes, this deserves its own main section as it is incredibly important. You have a responsibility towards your users: don't store passwords as plain text. Ever!

It's not clear from the code if your database does that, but I'm mentioning it because it is crucial. PHP's built-in password_hash function is a good starting point, as well as a popular article on Security StackExchange about password hashing done right.

If it's for a little script, I suggest a few simple (and unsafe) ways to store passwords with some improvement in safety, but the link to Security StackExchange is crucial to read if you're planning on offering a public platform.