current community

your communities

more stack exchange communities

Stack Exchange
tour help
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.
up vote 0 down vote favorite

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.";
}
?>
share|improve this question
    
What you may and may not do after receiving answers. I've rolled back Rev 3 → 2. –  200_success♦ Feb 17 at 19:39

1 Answer 1

up vote 2 down vote accepted

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

share|improve this answer
    
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. –  jordan.peoples Feb 17 at 19:28
    
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. –  KIKO Software Feb 17 at 19:40

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.