[SOLVED] Would like my first oop object to be checked

[Potatis]

I am learning as much as I can about OOP at the moment, it's slow going but I'm very interested in it. I am also converting to msqli syntax so it is all a big learning curve for me.

 

The first object I have tried to create is a database object that connects to the database and selects the database. It also has a method ready to execute sql queries and sanitize user input. It seems fine, at least I get no errors, I hope someone can have a quick look through.

 

I want to have a method in the database class that sanitizes user input. I'm not sure if sanitizing in oop is handled differently to the procedural method. I am hoping someone can check my translation.

 

Here is the clean() function I was using in the procedural php method:

 

<?php

function cleanInput($input){
   
   $db_user = "root";
   $db_pass = "";
   $db_host = "localhost";
   $db_name = "db";
  
   $link = mysql_connect($db_host, $db_user, $db_pass);
   mysql_select_db($db_name, $link);
   if (is_array($input)){
      foreach ($input as $key=>$value){
         $output[$key] = mysql_real_escape_string($value);
      }
   }
   else{
      $output = mysql_real_escape_string($value);
   }
   mysql_close($link);
   return $output;
}
?>

 

Which I translated to this (hoping someone can check it, and comment whether I should scrap this and do some other method):

 

 

public function clean(){
        $this->open_connection();

             if (is_array($input)) {
                 foreach ($input as $key=>$value){
                     $output[$key] = $mysqli->real_escape_string($value);
                 }
                 } else {
                     $output = $mysqli->real_escape_string($value);
             }
             close_connection();
             return $output;
         }
   

 

I'm not sure if that was a good enough translation?

 

Here is my full database.php, should this do the job? (connect to database, select database, execute sql queries and sanitize user input):

 

<?php



class database {

private $connection;

function __construct() {
    $this->open_connection();
}

public function open_connection() {
    $this->connection = $mysqli = new mysqli('$host', '$user', '$pass', '$db');
         if (mysqli_connect_error()) {
             die('Connect Error ('. mysqli_connect_errno() . ') '
                 . mysqli_connect_error());
         } else {
             $db_select = mysqli_select_db($this->connection, "oop");
             if(!$db_select) {
                 die('Database select failed ('. mysqli_connect_errno() . ') '
                 . mysqli_connect_error());
             }
         }
     }

public function close_connection() {
    if(isset($this->connection)){
        mysqli_close($this->connection);
        unset($this->connection);
        }
    }

public function query($sql){ //to execute queries, so use $sql = "SELECT * FROM table";
        $result = $mysqli_query($sql, $this->connection);
        if(!$result) {
            die('Database Query Failed ('. mysqli_connect_errno() . ') '
                 . mysqli_connect_error());
        }
        return $result;
    }

public function clean(){
       
             $this->open_connection();  //first connect to database 

             if (is_array($input)) {
                 foreach ($input as $key=>$value){
                     $output[$key] = $mysqli->real_escape_string($value);
                 }
                 } else {
                     $output = $mysqli->real_escape_string($value);
             }
             close_connection();
             return $output;
         }
   
}


?>

 

Thanks in advance.

[Ken2k7]

Why do you need to open a DB connection to clean or sanitize some data?

 

But here are a few things to also take into consideration:

1. If you already have the constructor function that calls the open_connection() function, why not make the open_connection function private?

2. I'm not familiar with mysqli, but does mysqli_connect_error() really a function? I thought it was mysqli->connect_error. But if you're checking for error like that, you should suppress the error first.

public function open_connection() {
    $this->connection = $mysqli = @new mysqli('$host', '$user', '$pass', '$db');
         if (mysqli_connect_errno()) {
             die('Connect Error ('. mysqli_connect_errno() . ') '
                 . mysqli_connect_error());
         } else {
             $db_select = @mysqli_select_db($this->connection, "oop");
             if(!$db_select) {
                 die('Database select failed ('. mysqli_connect_errno() . ') '
                 . mysqli_connect_error());
             }
         }
     }

 

3. Shouldn't the query function make a call to clean rather have the client call it manually?

4. If number 3 applies, then set the clean function to private.

5. You don't need to open another connection in the clean function. Also, you didn't call close_connection properly in that same function. You forgot $this.

public function clean(){
             if (is_array($input)) {
                 foreach ($input as $key=>$value){
                     $output[$key] = $mysqli->real_escape_string($this->connection, $value);
                 }
                 } else {
                     $output = $mysqli->real_escape_string($this->connection, $value);
             }
             return $output;
         }

 

Make sense?

[Potatis]

Hi Ken,

 

Thanks for your reply!

 

I don't know what I was thinking opening the database connection before sanitizing the user's input. That was stupid, and shows how things that would normally be obvious, are unclear when I am looking at the OOP method.

 

With point 1, thanks, I'll change it to private.

 

With point 2, I got that mysqli_connect_error straight from the php.net website. http://au2.php.net/manual/en/mysqli.connect.php

 

With 3, I'm not sure if the query should call clean(), should it? The query would not always be to process user input, it might be to count rows or something. It would be good to have an automatic call to clean() when it involved user input though.

 

Thanks very much for point 5. I see the errors, I will fix them. Thanks very much for taking the time to look through this, Ken2k7, I really appreciate it.

 

 

[Ken2k7]

2. Ah so it's just a PHP version thing. Well if your PHP version supports mysqli->connect_error, use that instead because you're using OOP. Either one works. =]

3. Your query function is fine. What I meant is for you to clean the query before passing it as a param in mysqli_query(). Also, why did you have $mysql_query()? What's with the $ sign there? You may want to extend this function later on though. But for the time being, it's fine. Just have the function make a call to clean().

 

A few things to consider when extending the function. You may want to store the result from mysqli_query() into a variable in the class. Then have methods to iterate over the result, etc. =]

 

And once you get comfortable enough, break it down even further. What I mean is this class is only supposed to do DB connection work, not SQL stuff. Have another class for that.

[aeonsky]

Can't you just use 'or die();' when connecting? @ is significantly slower.

[Ken2k7]

Can't you just use 'or die();' when connecting? @ is significantly slower.

Yes, but I don't like to die on the spot and stop the entire script from running.

[Potatis]

 

3. Your query function is fine. What I meant is for you to clean the query before passing it as a param in mysqli_query().

 

 

Aha, I see.

 

Also, why did you have $mysql_query()? What's with the $ sign there?

 

Habit of having a $ from my procedural days which might not yet be over. OOP is like a foreign language, it doesn't look much like the PHP I've known and loved.

 

 

You may want to extend this function later on though. But for the time being, it's fine. Just have the function make a call to clean().

 

 

Is that by just putting $this->clean(); at the top of the function?

 

A few things to consider when extending the function. You may want to store the result from mysqli_query() into a variable in the class. Then have methods to iterate over the result, etc. =]

 

And once you get comfortable enough, break it down even further. What I mean is this class is only supposed to do DB connection work, not SQL stuff. Have another class for that.

 

For sure, I'll look to separate the sql stuff from the database stuff, it makes good sense. I'll get comfortable with fully understanding what I have going on right now in this code before I stare at the screen and scratch my head over what methods I want iterating over my query results.

[9three]

I suggest learning to use exceptions. It helps you separate your logic code from your design code.

 

In your code, if I wanted to change how the error would be handled, I would have to go into your class and edited it. That becomes a problem because I might break the code and not even realize it. Here is a small example of exceptions:

 

class connection
{
  private $host;
  private $username;
  private $password;
  private $link;
  
  public function __construct($host = '', $username = '', $password = '')
  {
    $this->host = $host;
    $this->username = $username;
    $this->password = $password;
    $this->connect();
  }
  
  public function connect()
  {
    $this->link = mysql_connect($this->host, $this->username, $this->password);
    if (!$this->link)
      Throw New Exception(mysql_error());
  }
}

try
{
  $db = new connection('localhost', 'root', 'password');
}
catch (Exception $e)
{
  echo 'There was a problem with our connection '.$e;
}

 

The "try" block is going to try and execute everything, if it runs into a problem it will catch the exception and throw you the error. The variable $e can be substituted by whatever variable you want, it's the standard variable used across other languages.

[Potatis]

I suggest learning to use exceptions. It helps you separate your logic code from your design code.

 

In your code, if I wanted to change how the error would be handled, I would have to go into your class and edited it. That becomes a problem because I might break the code and not even realize it. Here is a small example of exceptions:

 

class connection
{
  private $host;
  private $username;
  private $password;
  private $link;
  
  public function __construct($host = '', $username = '', $password = '')
  {
    $this->host = $host;
    $this->username = $username;
    $this->password = $password;
    $this->connect();
  }
  
  public function connect()
  {
    $this->link = mysql_connect($this->host, $this->username, $this->password);
    if (!$this->link)
      Throw New Exception(mysql_error());
  }
}

try
{
  $db = new connection('localhost', 'root', 'password');
}
catch (Exception $e)
{
  echo 'There was a problem with our connection '.$e;
}

 

The "try" block is going to try and execute everything, if it runs into a problem it will catch the exception and throw you the error. The variable $e can be substituted by whatever variable you want, it's the standard variable used across other languages.

 

Your code works nicely. I will read about exceptions to get familiar with how all this works. Throw, try and catch are all new to me. Thanks for this info.

[9three]

With your code it would be like this

 

public function open_connection() {
    $this->connection = $mysqli = @new mysqli('$host', '$user', '$pass', '$db');
         if (mysqli_connect_error()) { //$this->connection->connect_error
             Throw New Exception (mysqli_connect_error());

 

You should be using $this->connection->connect_error but it's been bugged in the last couple of builds of PHP.

[Potatis]

Thanks very much again. I am at php.net reading about exceptions.

[Potatis]

I have been testing this, and it seems to work, but not the query($sql) part and I can't see any errors in the code.

 

On a simple index page I did a test script:

 

<?php
        require_once('database.php');

        if(isset($db)) {echo "true"; } else {echo "false";}; //is echoing true.
        echo "<br />";
        echo "Is this working?";
        echo "<br />";
        echo "<br />";
        
        echo $db->clean("It's Working!"); // Testing escaped quote

        $sql = "INSERT INTO table (user) VALUES ('My Name')";
        $result = $db->query($sql); // Does call query($sql) but fails.

      
        ?>

 

I get feedback that the database does connect (value is true), and the revised clean does add a backslash before the quote.

 

The test INSERT brings back the error message that is in the query($sql) method in database.php which is "It's not working...YET AGAIN!: " but there is no mysqli error message after that, it is blank. Nothing is inserted into my test database either.

 

Here is database.php revised:

 

<?php
class database {

private $connection;


function __construct() {
    $this->open_connection();
}

public function open_connection() {
    
    $this->connection = $mysqli = new mysqli('localhost', 'root', '', 'oop');
         if (mysqli_connect_error()) { //$this->connection->connect_error
             Throw New Exception (mysqli_connect_error());
         }
}

public function close_connection() {
    if(isset($this->connection)){
        mysqli_close($this->connection);
        unset($this->connection);
        }
    }

public function query($sql){  //to execute queries, so use $sql = "SELECT * FROM table";

       $result = mysqli_query($this->connection,$sql);

        if(!$result){
            die("It's not working...YET AGAIN!: " . mysqli_connect_error());
        }
          return $result;
}

public function clean($output){
       
             return mysqli_real_escape_string($this->connection,$output);
              
         }
  
}
try
{
  $db = new database();
}
catch (Exception $e)
{
  echo 'There was a problem with our connection '.$e; //works when db info is wrong.
}
?>

 

Can anyone see what is wrong with the way I am trying to execute the query? I have tried so many different things, this is at least one that doesn't bring up a mysql error. Still no insert though.

 

[Ken2k7]

Well, I hope table is an actual table name, unless you changed it.

 

Try:

$sql = "INSERT INTO table ('user') VALUES ('My Name')";

[Philip]

 $result = mysqli_query($this->connection,$sql);

        if(!$result){
            die("It's not working...YET AGAIN!: " . mysqli_connect_error());
        }

 

Are you sure you want to check for a connect error there?

How about just mysqli_error

[Potatis]

Well, I hope table is an actual table name, unless you changed it.

 

Try:

$sql = "INSERT INTO table ('user') VALUES ('My Name')";

 

lol, yes this is just testing, so I called the table "table". Adding the quotes didn't work. I can't see what's wrong with it, I'll start all over again, and work with some code that already works. I've been doing it from scratch to help me to learn OOP properly, not just copying code. I can't believe I am having trouble inserting data into a database! lol!

 

 $result = mysqli_query($this->connection,$sql);

        if(!$result){
            die("It's not working...YET AGAIN!: " . mysqli_connect_error());
        }

 

Are you sure you want to check for a connect error there?

How about just mysqli_error

 

Indeed, I got frustrated and tried different things. The error wasn't working, but it is now when I use mysqli_error($this->connection))

 

Seems I have a syntax error, I'll have to check the correct way to write the statement using the OOP method.

 

"You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'table (user) VALUES ('My Name')' at line 1"

[Philip]

Well, table is a reserved word so it needs to be in ticks.

 

INSERT INTO `table` (`user`) VALUES ('My Name')

[Potatis]

Oh, of course! The reserved word strikes again!

 

Now it all works. Thanks very much!