thedigicraft / Atom.CMS

Atom.CMS
56 stars 52 forks source link

Converting to PDO from MySQLi - bindParam #176

Closed dbashby closed 8 years ago

dbashby commented 8 years ago

I have started at the very beginning of the series and am trying to convert all the MySQLi to PDO.

I have retrieved the data as per video 9 which is the page title, page body and header however I would appreciate someone advising me regarding bindParam as my query is still open to mysql injection?

So far I have

`#Page setup - Gets data for all pages $stmt = $pdo->prepare("SELECT * FROM posts WHERE id = 1 LIMIT 1"); $stmt->execute();

    $page = $stmt->fetchAll(PDO::FETCH_ASSOC);

    //echo"<pre>";
    //print_r($result);

    foreach($page as $row){
    }`

That is in my setup.php page and on my index.php I have

<title><?php echo $row["title"] . ' | ' . $site_title; ?></title>

As well as echoing the relevant content to the other areas of the page.

Thanks for any help

dbashby commented 8 years ago

I now have the query in setup.php as

`if(isset($_GET['page'])){ $pageid = $_GET['page']; //Set $pageid to equal the value given in the URL } else { $pageid = 1; //Set $pageid equal to 1 or the home page }

#Gets data for all pages
$stmt = $pdo->prepare("SELECT * FROM posts WHERE id = $pageid");
    $stmt->execute();

    $page = $stmt->fetchAll(PDO::FETCH_ASSOC);

    //echo"<pre>";
    //print_r($result);

    foreach($page as $row){
    }`

And I am echoing out the data and everything is working but I still think I should be binding the query. Should I be using something like the following ?

$stmt = $db->prepare("select * from users where email=? and password = ?"); $stmt->bindParam(1, $email); $stmt->bindParam(2, $pword);

So in this instance do I need to name each field in the database, for example

`if(isset($_GET['page'])){ $pageid = $_GET['page']; //Set $pageid to equal the value given in the URL } else { $pageid = 1; //Set $pageid equal to 1 or the home page }

#Gets data for all pages
$stmt = $pdo->prepare("SELECT * FROM posts WHERE id = $pageid");
    $stmt->bindParam(":id", $pageid);
    $stmt->execute();

    $page = $stmt->fetchAll(PDO::FETCH_ASSOC);

    //echo"<pre>";
    //print_r($result);

    foreach($page as $row){
    }`

This is working but I would like to know if this protects against SQL injection as well as being written correctly or if there is a better way to write it?

Thanks

dbashby commented 8 years ago

As soon as I try to turn this SELECT query into a function, as per video 11, I do not get anything returned.

`function data_page($pdo, $pageid){

    $stmt = $pdo->prepare("SELECT * FROM posts WHERE id = $pageid");
    $stmt->bindParam(":id", $pageid);
    $stmt->execute();

    $page = $stmt->fetchAll(PDO::FETCH_ASSOC);

    //echo"<pre>";
    //print_r($result);

    foreach($page as $row){
        //return $page;
    }

    //return $page;
}`

I have tried return $page in both the foreach loop and outside of it but neither returns any results. Could someone advise please as to where I am going wrong?

Thanks

dbashby commented 8 years ago

My main question is do I need to bind each field that is being retrieved from the database through the query?

and secondly how do I turn this query into a function, is the foreach loop the best method?

Thanks

creptor commented 8 years ago

For PDO statements you'll need to adapt every query, so it can be used with the PDO syntax.

For security you should be fine by just adding the variables... PDO detects the content of the string, and then it process it, but it's good practice to always define which type of variables you're passing in. To do so, you just need to declared it in the bind param function. Like:

$conn->bindParam(1,$var,PDO::PARAM_STR);

Another thing is that PDO can define values, but not tables, and that kind of stuff... So if you want to set it safely (if imputed by users) you must define the possible values first. Example:

$this=array('a','b','c');
//the search array thing (I don't remember and I'm on the phone)
$conn=$pdo->prepare("SELECT * FROM $array WHERE id = ?");
creptor commented 8 years ago

Also you should always try to avoid a query to repeat multiple times to request what you can in 1, it takes more time to process (option function pls). So if you're just going to used it once, you should have no problems.

Based on that, you're required to insert every option you define to the prepared statement, so if you what to send or look for something with empty values just use null.

dbashby commented 8 years ago

This is where Allan is just building in the navigation to the project and calling the data specific to the page id such as title, label, header etc. So at this stage there is no data coming from users input.

If I follow your example

$conn->bindParam(1,$var,PDO::PARAM_STR);

And the order of my database table columns change would that not mean that I would need to revisit all my queries as I am naming them by integer rather than column name?

Question 2

So my existing query retrieving all data from pages instead of looking like

`$stmt = $pdo->prepare("SELECT * FROM posts WHERE id = $pageid"); $stmt->bindParam(":id", $pageid); $stmt->execute();

    $page = $stmt->fetchAll(PDO::FETCH_ASSOC);

    foreach($page as $row){
    }`

Should look like

`#Gets data for all pages // $stmt = $pdo->prepare("SELECT * FROM posts WHERE id = $pageid"); $stmt->bindParam(":id", $pageid,PDO::PARAM_INT); $stmt->bindParam(":user", $pageid,PDO::PARAM_INT); $stmt->bindParam(":type", $pageid,PDO::PARAM_INT); $stmt->bindParam(":slug", $pageid,PDO::PARAM_STR); $stmt->bindParam(":title", $pageid,PDO::PARAM_STR); $stmt->bindParam(":header", $pageid,PDO::PARAM_STR); $stmt->bindParam(":body", $pageid,PDO::PARAM_STR); $stmt->bindParam(":label", $pageid,PDO::PARAM_STR); $stmt->execute();

    $page = $stmt->fetchAll(PDO::FETCH_ASSOC);

    foreach($page as $row){
    }`

I am just trying to research your 2nd point in the first post. I have had my results displayed in array form prior to managing to get them to echo out in the relevant places, i.e. title, body and header

dbashby commented 8 years ago

If I use fetch rather than fetchAll I only get the first letter of each result. I understand why it is better to use fetch, takes up less memory, than fetchAll but fetch makes repeated requests on the database as it only gets 1 record at a time where as fetchAll will do exactly that, fetch all the data/records.

Is my understanding correct?

dbashby commented 8 years ago

Creptor, so is the point you are making in your second post that I should use fetchAll ?

Could you advise as to whether there is more I need to change on the query.

Also when I go to turn it into a function which I have not managed so far that the foreach loop needs to be inside the function? or will the foreach loop need to be on my index.php at the top of the document?

Thanks

dbashby commented 8 years ago

So now I have functions.php - following code to make the function

`function data_page($pdo, $pageid){

    $stmt = $pdo->prepare("SELECT * FROM posts WHERE id = $pageid");
    $stmt->bindParam(":id", $pageid,PDO::PARAM_INT);
    $stmt->bindParam(":user", $pageid,PDO::PARAM_INT);
    $stmt->bindParam(":type", $pageid,PDO::PARAM_INT);
    $stmt->bindParam(":slug", $pageid,PDO::PARAM_STR);
    $stmt->bindParam(":title", $pageid,PDO::PARAM_STR);
    $stmt->bindParam(":header", $pageid,PDO::PARAM_STR);
    $stmt->bindParam(":body", $pageid,PDO::PARAM_STR);
    $stmt->bindParam(":label", $pageid,PDO::PARAM_STR);
    $stmt->execute();

    $page = $stmt->fetchAll(PDO::FETCH_ASSOC);

    return $page;
}`

I also have setup.php with the following

`$page = data_page($pdo, $pageid); //turn SELECT query below into function foreach($page as $row){

}`

Everything is working but I also want to make sure that everything is written correctly, could someone advise please as to where if any I need to correct please.

Thanks

creptor commented 8 years ago

I have seen that you had problems getting the correct way to handle PDO, and to understand me.. so I'm making an example :3

Now I'll try to be very detailed in every step of the call so this issue doesn't get too extensive, in case of another problem please create another issue so this one doesn't get too long (new people don't read a lot).

PDO

Rule n° 1 PHP 5.1+ is required!

Example:

//creation of the connection details
$dsn = $db.system.':host='.$db.host.';dbname='.$db.basename.';port='.$db.port.';connect_timeout=15';

/*
$db.system. <- What type of database system is running
'mysql:host='.$db.host. <- IP of database host
';dbname='.$db.basename. <- Name of the database
';port='.$db.port. <- Port of the database host
';connect_timeout=15' <- Connection timeout
*/

//start the connection, and set it to a variable
$pdo= new PDO($dsn,$user,$password);//$user, and $password are the database log-in credentials

function get_something ($pdo,$id) {
    if($id!=null){

        $conn=$pdo->prepare('SELECT * FROM `table` WHERE id = ?');
        /*
        The PDO prepare SQL syntax can be in three different ways...
        - It can be used declaring the variables as `?`, and then using numbers relating to it's position in the bindParam.
            Example:
                bindParam(1,$var,etc);
                bindParam(2,$var2,etc);

        - It can be used declaring variables as name-substitution variables => `:(anything)`, and then using the same name substitution variable for the bindParam.
            Example:
                bindParam(':awesome',$var,etc);
                bindParam(':second',$var2,etc);

        - It can be replaced with a variable directly to the SQL syntax (not recommended).
            Example:
                $pdo->prepare("SELEC * FROM $this_shiet WHERE id = $no_rules")
        */

        //Binding params, in this case for `?`

        $conn->bindParam( 1 , $id , PDO::PARAM_STR , 12 );

        /*
        Too extensive to cover, check the links bellow...
        */

        //makes the call to the server
        $conn->execute();

        //gets the results, and then it returns them
        if($conn->rowCount()==1){
            $data=$conn->fetch(PDO::FETCH_ASSOC);
        }else{
            $data=null;
        }
        return $data;
    }else{
        return null;
    }
}

For bindParam reference check this, and this for the constants that it uses. (they start with PARAM)

NOTES

If you want to select multiple columns (like user 1,2,3,4 at the same time). you must use fetchAll instead of fetch.

Working loop example:

if($conn->rowCount()>0)
{
    foreach($conn->fetchAll(PDO::FETCH_ASSOC) as $data)
    {
        return $data;
    }
}
dbashby commented 8 years ago

Ok so from your information I have gone through the function and have ended up with

`function data_page($pdo, $id){ if($id != null){

        $stmt = $pdo->prepare("SELECT * FROM posts WHERE id = ?");
        $stmt->bindParam(":id", $id,PDO::PARAM_INT);
        $stmt->bindParam(":user", $user,PDO::PARAM_INT);
        $stmt->bindParam(":type", $type,PDO::PARAM_INT);
        $stmt->bindParam(":slug", $slug,PDO::PARAM_STR);
        $stmt->bindParam(":title", $title,PDO::PARAM_STR);
        $stmt->bindParam(":header", $header,PDO::PARAM_STR);
        $stmt->bindParam(":body", $body,PDO::PARAM_STR);
        $stmt->bindParam(":label", $label,PDO::PARAM_STR);
        $stmt->execute();
        if($pdo->rowCount()>0){
            foreach($pdo->fetchAll(PDO::FETCH_ASSOC) as $data){
                return $data;
            }
        }
    }
}`

When I run the function data_page();

$data = data_page($pdo, $id);

I have lost all the data from the database, as well as the basic site template.

creptor commented 8 years ago

Ok, I'm just going to give you out the solution for this, but next time read carefully.

Code:

function data_page($pdo, $id){
if($id != null){
    if(is_numeric($id)){
        $conn=$pdo->prepare("SELECT * FROM posts WHERE id = ?");
        $conn->bindParam(1,$id,PDO::PARAM_INT);
    }else{
        $conn=$pdo->prepare("SELECT * FROM posts WHERE slug = ?");
        $conn->bindParam(1,$id,PDO::PARAM_STR);
    }
    $conn->execute();
    if($conn->rowCount()==1){
        $data=$conn->fetch(PDO::FETCH_ASSOC);
    }else{
        $data=null;
    }
    return $data;
}
}

the prepare with ? is different that the one with :(something). In the ? you use numbers, and in the other one you use the same constant.

Example:

//with ?
$conn=$pdo->prepare("SELEC * FROM this WHERE id = ?");
$conn->bindParam(1,$var,(PDO thing));

//with :(something)
$conn=$pdo->prepare("SELECT * FROM this WHERE id = :id");
$conn->bindParam(':id',$var,(PDO thing));
dbashby commented 8 years ago

Thanks for the help with this. I tried your code from above and got a blank page returned. I then looked at the code and made the error of thinking you had used $conn for the connection. I reverted back to what you have given me.

Unfortunately I get a blank page, no results displayed.

UPDATE

The only way I have been able to get it working so far is your code with 1 amendment, fetchAll rather than fetch.

function data_page($pdo, $id){ if($id != null){ if(is_numeric($id)){ $conn=$pdo->prepare("SELECT * FROM posts WHERE id = ?"); $conn->bindParam(1,$id,PDO::PARAM_INT); }else{ $conn=$pdo->prepare("SELECT * FROM posts WHERE slug = ?"); $conn->bindParam(1,$id,PDO::PARAM_STR); } $conn->execute(); if($conn->rowCount()==1){ $data=$conn->fetchAll(PDO::FETCH_ASSOC); }else{ $data=null; } return $data; } }

And then on the setup.php page

`if(isset($_GET['page'])){ $pageid = $_GET['page']; //Set $pageid to equal the value given in the URL } else { $pageid = 1; //Set $pageid equal to 1 or the home page }

$page = data_page($pdo, $pageid); 
foreach($page as $row){
    return $data;           
}`

Is the code in setup.php correct. I know I am getting the results but should I really need the foreach loop?

creptor commented 8 years ago

As long has data is returned, fetch is not much different from fetchAll. You shouldn't run into any problems.

dbashby commented 8 years ago

Thanks for all your help!

1 more question if you don't mind, the foreach loop I have, if I comment out the return $data it still works, should I really be using a foreach loop in this way

foreach($page as $row){ //return $data; }

creptor commented 8 years ago

yes i'm sorry, it was my mistake, the return should be right after the loop.

foreach($page as $row){
    $data.=$row;
}
return $data;
dbashby commented 8 years ago

Thanks for that.