PHP/MySQL login error

[mexabet]

My login script is displaying the following error message:

Warning: Illegal string offset 'hash' on line 30.

Here is the code that generates the error:

<?php
    // configuration
    require("../../includes/config.php"); 

    // if form was submitted
    if ($_SERVER["REQUEST_METHOD"] == "POST")
    {
        // validate submission
        if (empty($_POST["username"]))
        {
            adminapologize("You must provide your username.");
        }
        else if (empty($_POST["password"]))
        {
           adminapologize("You must provide your password.");
        }

        $username = $_POST["username"];

        // query database for user
        $rows = "SELECT * FROM admin WHERE username = '$username'";

        // if we found user, check password
        if (count($rows) == 1)
        {
            // first (and only) row
            $row = $rows[0];

            // compare hash of user's input against hash that's in database
            if (crypt($_POST["password"], $row["hash"]) == $row["hash"])
            {
                // remember that user's now logged in by storing user's ID in session
                $_SESSION["admin_id"] = $row["admin_id"];

                // redirect to admin home
                redirect("index.php");
            }
        }

        // else apologize
        adminapologize("Invalid username and/or password.");
    }
    else
    {
        // else render form
        adminrender("login_form.php", ["title" => "Admin Log In"]);
    }

?>

The error seems to be coming from this line:

if (crypt($_POST["password"], $row["hash"]) == $row["hash"])

[mexabet]

Please, be advised that I'm using PDO in my functions.php file to connect, query, prepare, and execute.

[cereal]

Hi,

You are not executing the query:

$rows = "SELECT * FROM admin WHERE username = '$username'";

Where is the execute()?

Besides, the IF statement will fail because the crypt() function will generate a different hash each time you run it. So change line 30 to:

if (password_verify($_POST["password"], $row["hash"]))

It's better to use password_hash() instead of crypt(), it will generate strong hashes. More info here:

• http://php.net/password-hash
• http://php.net/password-verify
• http://php.net/crypt

[mexabet]

@cereal,
execute() is in the functions.php file:

function query($sql, $parameters = null)

    {

    static $pdo; // define the var as static so that it will persist between function calls



    try

    {

        // if no db connection, make one

        if (!isset($pdo))

        {

            // connect to database



            // you should set the character encoding for the connection



            $pdo = new PDO("mysql:dbname=" . DB_NAME . ";host=" . DB_SERVER, DB_USERNAME, DB_PASSWORD);

            $pdo->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION); // set the error mode to exceptions

            $pdo->setAttribute(PDO::ATTR_EMULATE_PREPARES,false); // turn emulated prepares off

            $pdo->setAttribute(PDO::ATTR_DEFAULT_FETCH_MODE,PDO::FETCH_ASSOC); // set default fetch mode to assoc so that you don't have to explicitly list the fetch mode every place

        }



        if(empty($parameters)){

            // no bound inputs

            $stmt = $pdo->query($sql);

        } else {

            // has bound inputs

            $stmt = $pdo->prepare($sql);



             // you should use explicit bindValue() statements to bind any inputs, instead of supplying them as a parameter to the ->execute() method. the comments posted in your thread lists the reasons why.



            $stmt->execute($parameters);

        }



    }

    catch (Exception $e)

    {

        // all errors with the connection, query, prepare, and execute will be handled here



        // you should also use the line, file, and backtrace information to produce a detailed error message

        // if the error is due to a query, you should also include the $sql statement as part of the error message

        // if $pdo ($handle in your code) is set, it means that the connection was successful and the error is due to a query. you can use this to include the $sql in the error message.



        trigger_error($e->getMessage(), E_USER_ERROR);

        //exit; // note: E_USER_ERROR causes an exit, so you don't need an exit; here.

    }



    return $stmt; // if the query ran without any errors, return the pdo statement object to the calling code

}

I modified line 30 with password_verify, like you suggested, but the error persists. I'm learning how to implement the password_hash.

[cereal]

I was talking about these lines:

// query database for user
$rows = "SELECT * FROM admin WHERE username = '$username'";
// if we found user, check password
if (count($rows) == 1)
{
    // first (and only) row
        $row = $rows[0];

In this case $rows is not a result set, it's only a string and if you do $row = $rows[0]; the contents of $row will be S, i.e. the first letter of the string. You have to submit the query to your function and return the result set, after that you can execute the rest of the code.

[mexabet]

@cereal, thanks for your insight.

I honestly don't know how to submit the query to the function and return the result set and then execute the remaining code. However, if you can tell me the PDO equivalent of the following code, I'll try to make the script to work:

$row = mysql_fetch_array($rows);

Thanks in advance.

[cereal]

If the functions.php file is included by the config.php file then try:

$rows = query($rows);

It should work and return an associative array.

[mexabet]

@cereal, thanks once again for your insight.

This is what I tried, but it still generates "illegal string offset 'username' error message:

$rows = "SELECT * FROM admin WHERE username = '$username'";

        $row = query($rows);

        // if we found user, check password
        if (count($rows) == 1)
        {
            // first (and only) row
            $row = $rows[0];

            // compare hash of user's input against hash that's in database
            if ($_POST["username"] == $row["username"] && crypt($_POST["password"], $row["hash"]) == $row["hash"])

[mexabet]

@cereal, I forgot to say that functions.php is included in config.php.

[cereal]

It was $rows not $row, the variable was overwriting itself, anyway to avoid confusion do:

$sql  = "SELECT * FROM admin WHERE username = '$username'";
$rows = query($sql);

if(is_array($rows) && count($rows) == 1)
{
    $row = $rows[0];

[mexabet]

@cereal,
The "illegal string offset" error message has stopped displaying. But down the line I'm getting another type of error, "Invalid username and/or password", when I know the supplied credentials are correct.

Here is the full code of the login.php:

<?php

    // configuration
    require("../../includes/config.php"); 

    // if form was submitted
    if ($_SERVER["REQUEST_METHOD"] == "POST")
    {
        // validate submission
        if (empty($_POST["username"]))
        {
            adminapologize("You must provide your username.");
        }
        else if (empty($_POST["password"]))
        {
           adminapologize("You must provide your password.");
        }

        $username = $_POST["username"];

        // query database for user
        $sql = "SELECT * FROM admin WHERE username = '$username'";

        $rows = query($sql);

        // if we found user, check password
        if(is_array($rows) && count($rows) == 1)
        {
            // first (and only) row
            $row = $rows[0];

            // compare hash of user's input against hash that's in database
            if ($_POST["username"] == $row["username"] && crypt($_POST["password"], $row["hash"]) == $row["hash"])                    
            {
                // remember that user is now logged in by storing user's ID in session
                $_SESSION["admin_id"] = $row["admin_id"];

                // redirect to admin home
                redirect("index.php");
            }
        }
        else
        {
            // else apologize
            adminapologize("Invalid username and/or password.");
        }
    }
    else
    {
        // else render form
        adminrender("login_form.php", ["title" => "Admin Log In"]);
    }

?>

Your help is always appreciated. And thanks in advance for your continued support.

[ryantroop]

Consider why you would get this result logically --

Your check states that if $rows is not an array, or the length of the array is NOT 1, then it's a failure.

In PHP, var_dump() is your friend (your best friend. Take it out to dinner and treat it nice sometimes. He works so hard for you!). It will help you debug almost anything. In this case, on line 25 I would put

var_dump($rows); 
exit(); 

and see what the output is.

You will likely find that rows is not an array, but instead may be some sort of string or object - alternatively, you may have multiple results or no results, and you will have to check your query to make sure it is doing exactly what you want it to be doing. Perhaps you will need more constraints in your where clause to limit your result set, though I find it strange that you would allow multiple admins with the same username (however, in testing you may have accidentally inserted the same user multiple times).

Hope that helps,

Ryan

[mexabet]

@nyantroop,
Thanks for your time and insight. This is the result of var_dump() after I tried to log in with the username, "admin":

object(PDOStatement)[2]
  public 'queryString' => string 'SELECT * FROM admin WHERE username = 'admin'' (length=44)

Any idea what the bug is and how to fix it?

You said something that is of interest to me: "allow multiple admins with the same username". I actually do not want to allow multiple admins with the same username. Please, how do I correct that?

[ryantroop]

Your second question is probably easiest to address:

You said something that is of interest to me: "allow multiple admins with the same username". I actually do not want to allow multiple admins with the same username. Please, how do I correct that?

You said something that is of interest to me: "allow multiple admins with the same username". I actually do not want to allow multiple admins with the same username. Please, how do I correct that?

You do this in two ways - one at the database level and one in your code. At the database level, you put a unique index on the table, which means everything in the column indexed must be unique (tada!) And it will raise an error whenever you try to insert a non-unique value. That said, you should probably get into the habbit of uppercasing or lowercasing all the email addresses when they are sent to your table, thus preventing oddities like "ryan" and "Ryan" being identified as 2 distinctly different strings.

Then, in your code, before you insert a new admin, you would do a select against your admin table to see if that username/email/whatever already exists. If it does, don't do the insert, return some sort of kind error message to your user.

Now for your first issue:

your "query()" function appears to not be returning an associative array as you believe. var_dump very clearly shows it instead returned the PDO object. Therefore, your logical check of if (is_array($rows)...) evaluates to false (as we discussed in my previous post). Looking at the code your posted, and reading the very last line with very kind comments, you will see that you are indeed returned the PDO object, and not the associative array you seemed to expect.

In fact.. looking at the query function you have, I am confused as to the implementation.. It looks like a stub of some sort, and not a completed method for doing a full query. On top of that, I am not sure how the $parameters argument controls if you are going to do a prepared statement or a straight SQL call, because neither of them seem to consume anything passed in anyway...

The problem seems to be that you are being returned a prepared statement, and not any sort of SQL result...

On a side note, I am not completely convinced that using static inside a function as you have is the best course of action. I get what you are trying to do, but I think scoping your PDO object outside the function explicitly would be far more readable, usable, and expandable, and less prone to forgetting where you silly PDO object lives later on down the road when your code is 1000 lines longer. However, if you know WHY you are declaring it as static and you are ok with that, more power to you - you taught me something "new" :-/

[ryantroop]

Of course... I was a bit tired when I wrote the above and re-reading it, I meant "when you try to insert a username already in the table it will return an error," and "email address" should be "username."

Sorry for the morning stupids.

[mexabet]

@ryantroop, thanks for your lengthy explanations. I'm still struggling to make sense of what you said.

How can I make the query() function to return an associative array, instead of the PDO object?

You talked about removing the static/PDO object from the custom query function and placing it outside. Do you mean remove the whole of this code and place it somewhere in the same file (functions.php)?:

    static $pdo; // define the var as static so that it will persist between function calls



    try

    {

        // if no db connection, make one

        if (!isset($pdo))

        {

            // connect to database



            // you should set the character encoding for the connection



            $pdo = new PDO("mysql:dbname=" . DB_NAME . ";host=" . DB_SERVER, DB_USERNAME, DB_PASSWORD);

            $pdo->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION); // set the error mode to exceptions

            $pdo->setAttribute(PDO::ATTR_EMULATE_PREPARES,false); // turn emulated prepares off

            $pdo->setAttribute(PDO::ATTR_DEFAULT_FETCH_MODE,PDO::FETCH_ASSOC); // set default fetch mode to assoc so that you don't have to explicitly list the fetch mode every place

        }



        if(empty($parameters)){

            // no bound inputs

            $stmt = $pdo->query($sql);

        } else {

            // has bound inputs

            $stmt = $pdo->prepare($sql);



             // you should use explicit bindValue() statements to bind any inputs, instead of supplying them as a parameter to the ->execute() method. the comments posted in your thread lists the reasons why.



            $stmt->execute($parameters);

        }



    }

I've made some modifications to the login.php file and I placed a check to test the query, and now it tells me that "No admin yet", when I know there is an admin:

<?php

    // configuration
    require("../../includes/config.php");

    // if form was submitted
    if ($_SERVER["REQUEST_METHOD"] == "POST")
    {
        // validate submission
        if (empty($_POST["username"]))
        {
            adminapologize("You must provide your username.");
        }
        else if (empty($_POST["password"]))
        {
           adminapologize("You must provide your password.");
        }

        $username = $_POST["username"];

        // query database for user
        $sql = "SELECT * FROM admin WHERE username = '$username'";

        $result = query($sql,array($username));        
        //var_dump($result);
        //exit;

        if($sql != false)
        {
            if($result->rowCount() == 0)
            {
                printf("No admin yet.");
            }

            // if we found user, check password
            if($result->rowCount() == 1)
            {
                // first (and only) row
                $row = $result->fetch();

                // compare hash of user's input against hash that's in database
                if ($_POST["username"] == $row["username"] && crypt($_POST["password"], $row["hash"]) == $row["hash"])
                //if ($_POST["username"] == $row["username"] && password_verify($_POST["password"], $row["hash"]))                    
                {
                    // remember that user is now logged in by storing user's ID in session
                    $_SESSION["admin_id"] = $row["admin_id"];

                    // redirect to admin home
                    redirect("index.php");
                }
            }
        }
        else
        {
            // else apologize
            adminapologize("Invalid username and/or password.");
        }
    }
    else
    {
        // else render form
        adminrender("login_form.php", ["title" => "Admin Log In"]);
    }

?>

[ryantroop]

So at this point I'm gathering that you did not initially write this, nor do you understand how the static keyword works in php functions. In order to explain some of this, I will have to explain some other things so I can get my point across clearly.

First - $pdo outside the function...
If you are not familliar with scope, you're going to get a crash course. In a blank <?php ?> block, any variable declared is considered in the "global" scope. This means any declared function, any sub function, any include, any whatever, can access the value of that variable without explicitly being passed into the function.

When functions run, they do something, and then the memory to said function gets released and "garbage collected." If inside of the function's scope you add the "static" keyword to a declared variable, it gets put in a psudo global state - basically, you tell PHP to NOT garbage collect it. This does a few things... one, it prevents the function from being "garbage collected," and second, it gives access to the variable outside of the function. In this programmer's opinion, this is bad design - if you want a global variable, make a global variable - making one on the fly from within a function is asking for trouble when debugging.

Now, the why - whoever wrote this for you had the intention of $pdo of being reused - and also to save trips to the database every time this function was called. If you have to recreate a new PDO object every time the function runs, you are not only making a query to the database, but you are also authenticating, and creating a lot of behind the scenes traffic. This is bad - it's slow, inefficient, and sucks up resources.

Now, if the intention is to not make a database object unless you need it, that's a different discussion - however, there is always a way to make a logical check before creating something and leaving it in the proper scope.

So now, the answer to your actual question - how to return an associative array:

This line:

return $stmt; // if the query ran without any errors, return the pdo statement object to the calling code

explains it all to you in a clear concise comment - return the PDO statment object.

Now, whoever wrote this for you clearly did not explain how to use this function very well - they just assumed you would read the code and see the self explanatory bits and use it as needed. It appears, to me anyway, that your biggest issue is a lack of understanding of primitive types in a language (in this case, an array vs an object and what they do and how to use them). I encourage you to read up a bit on PHP arrays and objects, and then reading the PHP help on the PDO object and how to use it. In theory, nothing was wrong with your original code other than you were looking for an array instead of an object, and then iterating through whatever the object returned (or, actually executing it because you were originally being returned a prepared statement).

If this all seems confusing, that's fine - but if that's the case, then you really need to just write some more code and practice - learn how all of this stuff works, and figure out how everything works together. If it does make sense, I hope this clarifies what's going on, and gives you some direction on how to get yourself up and running.

To get you started -->

//you can add logic here to only create if you are going to use the "query" function below
$pdo = new PDO("mysql:dbname=" . DB_NAME . ";host=" . DB_SERVER, DB_USERNAME, DB_PASSWORD);
$pdo->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION); // set the error mode to exceptions
$pdo->setAttribute(PDO::ATTR_EMULATE_PREPARES,false); // turn emulated prepares off
$pdo->setAttribute(PDO::ATTR_DEFAULT_FETCH_MODE,PDO::FETCH_ASSOC); // set default fetch mode to assoc so that you don't have to explicitly list the fetch mode every place



function query($sql)
{
  $stmt = $pdo->query($sql);
  //since your query is simple, I took out the prepared statement portion
  return $stmt;
}

//usage

$dbObj = query("select * from table");
foreach ($dbObj as $result)
{
  echo $result["columnName"];
}

Now, the original function you provided has some wonderful benefits - including error handling. If you can keep that, I would. However, you need to understand how the middle bit works, and how to pass paremeters to be bound to a stored procedure / prepared statement (and the original author expected you to implement this).

This page:
http://wiki.hashphp.org/PDO_Tutorial_for_MySQL_Developers

will give you a few examples on how to use PDO and their depricated counterparts (which are kind of important to understand and know), and will give you a few other code examples of how to use PDO and get data from the object for consumption.

I hope that helps, and didn't make things more difficult :-/

Ryan

[mexabet]

@Ryantroop,
Thanks for your lengthy explanation. Yes, I didn't initially write the query() function with PDO. I became interested in PDO because of its handling of security regarding SQL injection, but I'm yet to grasp how to effectively apply it.

I know what scope means, but got confused with using someones function. My script worked well, until I introduced PDO. I like to keep my original function because, like you said, it has some wonderful benefits - including error handling. But I "need to understand how the middle bit works, and how to pass parameters to be bound to a stored procedure / prepared statement." That's my challenge now. I'm struggling to learn it. Thanks for the hashphp link.