moving from->to: mysql->pdo, md5->pbkdf2, procedural->oop

I'm upgrading/rewriting our online client portal and wondering if my log in page code looks okay so far. during this rewriting/learning process I am upgrading a number of things: PHP 5.2 to 5.3.5, MYSQL 4.1 to 5.5, md5() to SHA512 using pbkdf2, salt and multiple iterations, MySQL functions to PDO, HTML4 to 5. I'm learning about and trying to implement prepared statements, exceptions and OOP, thanks in large part to reading StackOverflow. Its a lot to wrap my head around and its not done yet (just starting) but I just want to make sure I am on the right track. I connect to MySQL on the webserver for log in/user management and I connect to Sybase on a fileserver for client account database. I have looked into classes but not sure I am ready to go there yet. So please tell me how the code looks, what I can do different/better, logic, flow....
thanks

```
1024 )
{
$error = 'Please enter a valid User ID and Password';
}
else
{

try
{
$mysql_select = $pdo_mysql->prepare( "SELECT userid, password, email, login_attempts FROM users WHERE userid = :userid LIMIT 1" );
$mysql_select->bindParam( ':userid', $user_id );
$mysql_select->execute();

if( $mysql_select->rowCount() == 1 )
{
$row = $mysql_select->fetch( PDO::FETCH_ASSOC );

if( $row[ 'login_attempts' ] == 5 )
{
$error = 'Your login account has been locked.';
}
else
{
if( validate_password( $password, $row[ 'password' ] ) ) //password is correct
{
//session stuff here
//header to index
}
else //password is wrong
{
$error = 'Incorrect

Since you are updating anyways, you should just update to the most recent version of PHP if you can. I believe its supposed to be faster in addition to offering a bunch of new toys to play with, but I haven't gotten IT to update me yet and I haven't gotten around to playing with it in my spare time :(

I'm not sure about any other devs, but I tend to avoid messing with the include path. Instead, I just provide the full path whenever necessary. But I'd be interested in hearing what the others think about this.

require $_SERVER[ 'DOCUMENT_ROOT' ] . 'relative/path/to/file.php';

If you are trying to learn OOP, then you are going to have to learn how to use functions first. The main problem with this script is the flow and legibility. Both would benefit quite a bit from the use of functions. Once you have everything set up in a function, then you will want to take a look at the Single Responsibility Principle to help you separate the logic into separate functions, and those functions into separate files. Then you are going to want to apply the "Don't Repeat Yourself" (DRY) Principle to them to make them more efficient. Once you have these concepts mastered, adapting that knowledge to OOP should be relatively simple. Trying to do it backwards is just going to confuse you. I'll try and explain some of the basics of the above concepts below.

Another way to check if a form has been submitted is to use the super global, $_SERVER[ 'REQUEST_METHOD' ]. Both methods are fine, but I think having a form element just to verify that POST was done with YOUR form is a security measure that is easily bypassed. The only exception might be a unique key generated for each session id with a limited shelf life, but this is getting into some more advanced stuff that I don't have much experience in.

if( $_SERVER[ 'REQUEST_METHOD' ] == 'POST' ) {

While the concept of indenting your code is good, having to indent your code over large blocks and nesting is not. This is called the Arrow Anti-Pattern. Google it to learn more, but essentially it states that heavy indentation is bad. Break early if necessary by using returns, or by reversing if/else statements so that the larger block is outside of the if/else logic and only the smaller remains then use a return in the remaining if block to prevent further parsing. For example

if( ! isset( $_POST[ 'process' ] ) ) {
    return FALSE;
}

//rest of code

Of course this concept will only get you so far without functions, so you'll have to implement some first. The same also holds true for try/catch blocks.

Something from 5.2 that is still available in the newer versions is a neat, but little-known, function called filter_input(). This function makes REGEX unnecessary by using built in filtering/sanitizing/validating. Eliminating REGEX is always a plus in my book. Take a look at the documentation for it for a full list of what it can do, but here's an example:

$user_id = filter_input( INPUT_POST, 'user_id', FILTER_SANITIZE_STRING );

Those $error strings are perfect examples of "early return" candidates. Returning in the case of an error ensures that no additional overhead is accrued when not necessary. Of course the drawback of this is that you can't register more than one error at a time, but sometimes this is more effort than it is worth. Besides, the way you have your current structure set up, that's all you would have gotten anyways.

I'm not too certain what you are doing when you say "session stuff here" and "header to index", but it looks like it might be violating DRY by repeating the first redirect. Again, this is where functions would come in handy. I feel there is going to need to be more rework here, but I can't grasp what that will be at this point. The main thing that is throwing me right now is the need for an else statement. I feel incrementing the attempt before attempting authentication might be a better tactic. I'm assuming you reset this once validated, so the increment wont make a difference then. Although, an even better tactic might be to use a session variable for this, and once that variable has been incremented to the max, then log that the account has been locked. No need to access the database so frequently for something so trivial.

Let's see... I went over the Arrow Anti-Pattern and DRY. That leaves the Single Responsibility Principle. You don't really have a good example for me to demonstrate with, but let's look at the following bit of code. This is one of the more obvious candidates for a function. Lets say the function is called reloadIndex(). That's a horrible name, but bare with me. Going by that name we can assume that the function is going to reload the index. Looking at the code we can confirm this, but it is also checking if the session variable "user_id" is set first. This is not following that principle. This function should only be concerned with reloading the index, not what is required before the index c

StackExchange Code Review Q#15750, answer score: 5