1
\$\begingroup\$

Is this correct MYSQLI implementation? IS it safe from injection? IS the object cleanup done properly?

<?php

include_once '../../securelogin/db_connect.php';
include_once '../../securelogin/functions.php';

sec_session_start();

if (login_check($mysqli) == true)
{
    $con = new mysqli("localhost", "username", "password", "databasename_todo");

    // Check connection
    if ($con->connect_errno)
    {
        echo "Failed to connect to MySQL: (" . $con->connect_errno . ") " . $con->connect_error;
    }
    else
    {
        $jsonText = file_get_contents('php://input');
        $decodedText = html_entity_decode($jsonText);
        $myArray = json_decode('[' . $decodedText . ']', true);

        $newText = $myArray[0]['TEXT'];
        $newOwnerID = $myArray[0]['OWNER_ID'];
        $newCreateDate = date("Y-m-d"); 
        $newCompleted = 'N';

        $newText = mysqli_real_escape_string($con, $newText);

        if(!($sql = $con->prepare("INSERT INTO checklistitems (TEXT, OWNER_ID, COMPLETED, CREATE_DATE, CREATE_USER) VALUES (?,?,?,?,?)")))
        { echo "Prepare failed: (" . $con->errno . ") " . $con->error;}

        if(!$sql->bind_param('sissi', $newText, $newOwnerID, $newCompleted, $newCreateDate, $newOwnerID))
        {echo "Binding parameters failed: (" . $sql->errno . ") " . $sql->error;}

        if(!$sql->execute())
        {echo "Execute failed: (" . $sql->errno . ") " . $sql->error;}

        echo "SUCCESS - Affected rows ADD: " . $con->affected_rows;

        $sql->close();
    }

    $con->close();
    $con = null;
}
else
{  
    echo "Error! You are not authorized to access this page. Please login.";
}
?>
\$\endgroup\$
1

1 Answer 1

3
\$\begingroup\$

What are these lines all about?

$thread = $con->thread_id;
$con->close();
$con->kill($thread);
$con = null;

it seems a bit, sorry for the pun, overkill. According to the PHP manual this should do just fine:

$con->close();

but even if you leave it out, the connection will be closed automatically when the script terminates. See:

http://php.net/manual/en/mysqli.construct.php

read the examples, and see:

http://php.net/manual/en/mysqli.close.php

\$\endgroup\$
2
  • \$\begingroup\$ Those lines are, literally, the least important thing in the code sample to me, and have been removed in hopes of better code reviews in the future. \$\endgroup\$
    – jordan
    Commented Feb 17, 2015 at 19:28
  • \$\begingroup\$ Sorry to disappoint you. Clearly there's not that much wrong with your code, if it were you would have had many useful answers. You error check, escape and bind, what more can be said? Perhaps that escaping is not needed because you bind the parameters? But then again you might not think that's important. \$\endgroup\$ Commented Feb 17, 2015 at 19:40

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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