Loop through post and use in query

Question

so i'm using a foreach loop to loop through a form i've send with javascript. In this loop I use the keys(input fields names) and values(input values) and use them to build my query, I still check for nulled values and other field types and then i add them to a variable . I'm wonderin if this is really the best idea if I value the security of my application. Could someone give me some insight? this is a piece of code to better describe what I mean:

$isValid = true;
        $fouteVelden = array();
        $verplichtArray = array('naam','categorie','prijs_inkoop');
        foreach($_POST['gegevens'] as $key => $value){
            if(in_array($key,$verplichtArray)){
                if($value == ""){
                    $isValid = false;
                    $fouteVelden[] = $key;
                    $message = "Een of meerdere van de velden waren leeg.";
                }
            }
        }

        if($isValid == true){
            foreach($_POST['gegevens'] as $key => $value){
                if($key == "categorie"){
                    if($value == "kies"){
                        $isValid = false;
                        $fouteVelden[] = $key;
                        $message = "Kies eerst een categorie.";
                    }
                }
            }
        }

        if($isValid == true){
            foreach($_POST['gegevens'] as $key => $value){
                if($key == "prijs_inkoop"){
                    $value = str_replace(',','.',$value);
                    if(!is_numeric($value)){
                        $isValid = false;
                        $fouteVelden[] = $key;
                        $message = "Er zijn alleen getallen mogelijk bij prijs inkoop.";
                    }
                }
            }
        }

        if($isValid == true){

            $branche = new Branches;
            $branche->getBranche('id',$_SESSION['branche']);
            $conn = new Connection2($branche->database,$branche->dbuser,$branche->dbpass);
            $notneeded = array("subcatvan");
            $nulledout = array("besteleenheid");


            foreach($_POST['gegevens'] as $key => $value){
                if(!in_array($key,$notneeded)){
                    if($key == "prijs_inkoop" || $key == 'prijs_verkoop'){
                        $value = str_replace(',','.',$value);
                        $fields .= "`".$conn->real_escape_string($key)."`,";
                        $values .= "'".$conn->real_escape_string($value)."',";
                    }else{
                        if(in_array($key,$nulledout)){
                            if($value != ""){
                                $value = str_replace(',','.',$value);
                                if(!is_numeric($value)){
                                    $isValid = false;
                                    $fouteVelden[] = $key;
                                    $message = "Er zijn alleen getallen mogelijk bij ".$key.".";
                                }else{
                                    $fields .= "`".$conn->real_escape_string($key)."`,";
                                    $values .= "'".$conn->real_escape_string($value)."',";
                                }
                            }else{
                                $fields .= "`".$conn->real_escape_string($key)."`,";
                                $values .= "'0',";
                            }
                        }else {
                            $fields .= "`".$conn->real_escape_string($key)."`,";
                            $values .= "'".$conn->real_escape_string($value)."',";
                        }
                    }
                }
            }
            $fields .= "`merk`,`thema`,`visible`,`volgnr`,`gewijzigd`";
            $values .= "'0','0','Y','0',now()";
            if($isValid == true){
                $conn->query("INSERT INTO `product` (".$fields.") VALUES (".$values.")");   
                $message = $conn->inserted_id();
            }
        }

        if($isValid == false){

            $dataArray = array(
                "isValid" => $isValid,
                "message" => $message, 
                "fouteVelden" => $fouteVelden
            );

            echo json_encode(utf8json($dataArray)); 

        }
        if($isValid == true){
            $dataArray = array(
                "isValid" => $isValid,
                "message" => $message, 
            );

            echo json_encode(utf8json($dataArray)); 
        }

it really helps if your code is written in English, as most of the Code Reviewers here are English speaking, and it's the Stack Exchange way. I don't know what $fouteVelden[] is, I can guess but I could be wrong. I don't know what the $message variable is set to... the list goes on. please translate before you post, or even better, use English when coding. Not sure what the standard is on this. — Malachi
– Malachi, Commented Sep 16, 2014 at 14:17
$fouteVelden = $wrongFields. $verplichtArray = $requiredArray. 'naam' = 'name'. 'categorie' = 'catagory'. 'prijs_inkoop' = 'price_purchase'. 'gegevens' = 'data'. "Een of meerdere van de velden waren leeg." = "One or more of the fields was empty.". "kies" = "choose". "Er zijn alleen getallen mogelijk bij prijs inkoop." = "Only numbers possible for price purchase.". — Pimgd
– Pimgd, Commented Sep 16, 2014 at 14:36

Community · Accepted Answer · 2017-04-13 12:40:49Z

Most important of all, you should not use string concatenation to set values in an SQL statement like this:

$conn->query("INSERT INTO `product` (".$fields.") VALUES (".$values.")");

You should use prepared statements instead. It will make your queries safer as prepared statements check value types and prevent sql injection attacks, and it can make queries more efficient as they can be compiled and only the dynamic parameters inserted.

When you check if a boolean value is true or false, it's better to write like this:

// instead of $valid == true
if ($isValid) {

// instead of $valid == false
if (!$isValid) {

Related to this, at the end of your code you do this:

if($isValid == false){
    // ...
}
if($isValid == true){
    // ...
}

Inside the { ... } blocks you don't change the value of $isValid, so these two blocks are mutually exclusive and should have been written with an if-else like this:

if (!$isValid) {
    // ...
} else {
    // ...
}

This looks strange:

foreach($_POST['gegevens'] as $key => $value){
    if(in_array($key,$verplichtArray)){
        if($value == ""){
            $isValid = false;
            $fouteVelden[] = $key;
            $message = "Een of meerdere van de velden waren leeg.";
        }
    }
}

If there are multiple $value == "", the $message will be overwritten. But of course that's normal if I paste your message into Google Translate, it turns out it means "One or more of the fields were empty" which makes sense :-) So yes as @malachi it would have helped to translate the non-English texts when you post on an English-language forum like this one ;-)

Also, the two if statements can be combined, and it's good to put a space after comma, after ) and before (:

foreach($_POST['gegevens'] as $key => $value) {
    if (in_array($key, $verplichtArray) && $value == "") {
        $isValid = false;
        $fouteVelden[] = $key;
        $message = "Een of meerdere van de velden waren leeg.";
    }
}

Aah thanks, So do you know of any way i can bind the Fields and the Values of a prepared statement or is it impossible? — Sjoerd de Wit
– Sjoerd de Wit, Commented Sep 17, 2014 at 7:44
It's certainly possible! On the page I linked, Example #1 demonstrates how to do it in a simple case. Your case is not that simple, but certainly doable. — janos
– janos, Commented Sep 17, 2014 at 8:27

Stack Exchange Network

Loop through post and use in query

1 Answer 1

You must log in to answer this question.

Hot Network Questions