You have an error in your SQL syntax error?

Question

I have this function with quite a simple SQL in it that gives error in the syntax where there couldn't be one.

function createPetition($conn, $pName, $pTarget, $pDescription, $userId) {
    $sql = "INSERT INTO petitions (petitionName, petitionTarget, petitionDescription, usersId) VALUES (?, ?, ?, ?);";
    //$sql2 = "INSERT INTO petitions (`petitionDate`) VALUES (NOW());";
    $stmt = mysqli_stmt_init($conn);
    if (!mysqli_stmt_prepare($stmt, $sql)) {
        header("location: ./index.php?error=stmtfailed");
        exit();
    }

    /*if (!mysqli_stmt_prepare($stmt, $sql2)) {
    header("location: ./index.php?error=stmtdatefailed");
    exit();
    }*/
    
    mysqli_stmt_bind_param($stmt, "ssss", $pName, $pTarget, $pDescription, $userId);

    if (!mysqli_query($conn, $sql)) {
        echo "Error description: " . mysqli_error($conn);
    } else {
        mysqli_stmt_execute($stmt);
        mysqli_stmt_close($stmt);
        header("location: ./petitionfinal.php?error=none");
        exit();
    }
}

The result of which is

Error description: You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near '?, ?, ?, ?)' at line 1

When there is clearly no error.

Looks like you are learning PHP from the infamous Dani Krossing videos. All your problems stem from it — Your Common Sense, Nov 07 '20 at 04:47

score 3 · Accepted Answer · answered Nov 07 '20 at 12:53

The problem is that you have a rogue mysqli_query() in your code. You need to remove it.

You might have missed it because you have way too much code. You should remove most of it. You really do not need all of it.

Here is what the code should look like:

function createPetition(mysqli $conn, $pName, $pTarget, $pDescription, $userId) {
    $sql = "INSERT INTO petitions (petitionName, petitionTarget, petitionDescription, usersId) VALUES (?, ?, ?, ?);";
    
    $stmt = $conn->prepare($sql);
    $stmt->bind_param("ssss", $pName, $pTarget, $pDescription, $userId);
    $stmt->execute();

    header("location: ./petitionfinal.php?error=none");
    exit();
}

However, having redirection inside of the function is not a good idea. I would strongly recommend to take it out of the function.

function createPetition(mysqli $conn, $pName, $pTarget, $pDescription, $userId) {
    $sql = "INSERT INTO petitions (petitionName, petitionTarget, petitionDescription, usersId) VALUES (?, ?, ?, ?);";
    
    $stmt = $conn->prepare($sql);
    $stmt->bind_param("ssss", $pName, $pTarget, $pDescription, $userId);
    $stmt->execute();
}

// when you call
createPetition($conn, $pName, $pTarget, $pDescription, $userId);

header("location: ./petitionfinal.php?error=none");
exit();

See how much nicer it is. It is so much easier to spot mistakes now.

If you are wondering what about errors then I must point you to How to get the actual mysql error and fix it?

score 0 · Answer 2 · edited Nov 07 '20 at 12:55

Notes

You probably shouldn't be using exit inside of a function: return is a better bet.
You can't flip-flop between query and prepare when you're using bound parameters. The parameter is only bound during the execute statement.
Database interaction is far easier if you use Object style methods as opposed to Procedural style functions.

Problem

Your final if statement has mysqli_query which doesn't work with prepared statements and bound variables.

It tries to run the query with the literal value of $sql. Which is why there are ??? in the error response.

Code example

function createPetition($conn, $pName, $pTarget, $pDescription, $userId){
    $sql = "INSERT INTO petitions (petitionName, petitionTarget, petitionDescription, usersId) VALUES (?, ?, ?, ?);";
    //$sql2 = "INSERT INTO petitions (`petitionDate`) VALUES (NOW());";
    $stmt = mysqli_stmt_init($conn);
    if (!mysqli_stmt_prepare($stmt, $sql)) {
    header("location: ./index.php?error=stmtfailed");
    exit();
    }

    /*if (!mysqli_stmt_prepare($stmt, $sql2)) {
    header("location: ./index.php?error=stmtdatefailed");
    exit();
    }*/
    
    mysqli_stmt_bind_param($stmt, "ssss", $pName, $pTarget, $pDescription, $userId);

    if (!mysqli_stmt_execute($stmt)) {
        echo("Error description: " . mysqli_error($conn));
      }else{     
        mysqli_stmt_execute($stmt);
        mysqli_stmt_close($stmt);
        header("location: ./petitionfinal.php?error=none");
        exit();
      }

}

Preferred code

function createPetition($conn, $pName, $pTarget, $pDescription, $userId){
    $sql   = "
        INSERT INTO petitions
        (petitionName, petitionTarget, petitionDescription, usersId)
        VALUES
            (?, ?, ?, ?)
    ";

    $query = $conn->prepare($sql);
    $query->bind_param("ssss", $pName, $pTarget, $pDescription, $userId);

    if($query->execute()){
        return TRUE;
    }
    else{
        // return $mysqli->error;
        return false;
    }
}

// You should carry out error reporting or re-directs outside of your function.

/*

For example:


$makePetition = createPetition($conn, $pName, $pTarget, $pDescription, $userId);

if($makePetition === TRUE){
    header("location: ./petitionfinal.php?error=none");
}
else{
    header("location: ./index.php?error=stmtfailed");
}
*/

Those exists are taught by some guy on YouTube, whose videos, unfortunately, have millions of views. As well as such error redirects that make very little sense from the HTTP correct usage point of view. — Your Common Sense, Nov 07 '20 at 05:17
Regarding the error reporting, however, a function that returns an error message is truly a terrible practice. Errors must be raised, not returned. For this purpose, one just has to [configure mysqli to throw exceptions](https://phpdelusions.net/mysqli/error_reporting). It will make *every single condition* in this code obsoleted and the code will become consistent, let alone two times smaller. — Your Common Sense, Nov 07 '20 at 05:21
@YourCommonSense I'm not quite sure what you mean, the function doesn't return an error message? There is a line in the code _commented out_ which would do that; perhaps I could have made that `echo` instead. But I don't see the issue in **temporary** reporting of errors for debugging purposes?.. (often times it's much easier than diving into logs which could be updating) — Steven, Nov 07 '20 at 10:32
Why are you checking the return value of `if($query->execute())`? — Dharman, Nov 07 '20 at 10:36
@Dharman to check if the query completed successfully? In an in `if` statement so that additional code _could_ be executed if needs be. Granted it could be written `return $query->execute()` and further code completed outside the function — Steven, Nov 07 '20 at 10:40
Fair question. The issue with temporary reporting is that it just makes no sense. You need the error message every time your query failed, not for some short period of time. Even commented out, this code suggests that the error message could be returned but that's not the way to go. Don't take me wrong, you made this function a lot better, I am just suggesting a further improvement. — Your Common Sense, Nov 07 '20 at 10:45
This makes no sense. What if prepare failed? What if bind_param failed? You should not check manually the return value. Enable error reporting and forget about checking each function call manually. — Dharman, Nov 07 '20 at 11:08

You have an error in your SQL syntax error?

2 Answers2

Linked

Related