Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I am new to PHP so this is largely compiled from examples around the web. It seems to work fine. Are there any ways in which it can be improved upon? Is it secure?

<?php
    mysql_connect("localhost", "myusername", "mypassword");
    mysql_select_db("companyDB");

    function user_login($username, $password)

{
    ob_start();

    $username = mysql_real_escape_string($username);
    $password = md5($password);

    $sql = "select * from users where username='$username' and password ='$password' LIMIT 1";
    $result = mysql_query($sql) or die('Please change your mysql query !');

    if(mysql_num_rows($result) > 0)
    {       
        $_SESSION['username'] = $username;
        $_SESSION['loggedin'] = true;

        $row = mysql_fetch_row($result);
        $access_level = $row[4];
        $_SESSION['accessLevel'] = $access_level;

        while (ob_get_status()) 
        {
            ob_end_clean();
        }

        header("Location: /index.php");

    }
    else
    {

    }
}
?>
share|improve this question
add comment

1 Answer

up vote 3 down vote accepted

Well, mysql() is being deprecated, so be aware of that. mysqli() and pdo() are the new standards. I'd say continue learning on the old one, everything you learn can be translated to the new.

When connecting to a database, it is best to give that connection a name, such as $conn so that it can be referenced later. This is so that you can specify which connection you wish to use when selecting databases, or if you need to check if the connection is successful. Same for selecting the database. So...

$conn = mysql_connect("localhost", "myusername", "mypassword");
if( ! $conn ) {
    //handle error and exit script...
}

$companyDB = mysql_select_db("companyDB", $conn);
if( ! $companyDB ) {
    //handle error and exit script...
}

You've started output buffering (ob_start()), but not done anything with it except clear it. Usually you'll want to examine the output for errors, or dump it directly onto the page before clearing it. But for a "first" script this is fine. Just be aware that there is more to it than just starting and clearing.

Try to avoid or die() short circuiting. It works but was not intended for this purpose. die() shouldn't even be in live environments, its meant as a debugging tool. Instead you should handle any errors more elegantly, such as displaying what happened, logging errors, etc...

Be careful of magic numbers. You've told us that $row[4] is the access level, but why? What if it changes? This should probably be grabbed with a more specific SQL statement. I'm not much of SQL guy, so I can't help you out here, but I imagine there should be a better way.

Location headers should be terminated with exit afterwards to prevent the rest of the script from running.

If you don't have anything to put in an else block, then there is no need to declare one.

You also need to close connections to your mysql using mysql_close().

share|improve this answer
1  
Thanks for the detailed answer! It really helps. –  kmcgrady Jul 2 '12 at 20:02
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.