views:

320

answers:

2

I am using a function in php for all select queries so that i can dynamically retrieve data from my database ..... I just wanted to know that is my code secure and efficient or if their is a better way to do this, if so please point me to the right direction...thanks

class mysql {
 private $conn;  
 function __construct(){
    $this->conn= new mysqli(DB_SERVER, DB_USER, DB_PASSWORD, DB_NAME);
    if( mysqli_connect_errno() )  
    {  
      trigger_error('Error connecting to host. '.$this->connections[$connection_id]->error, E_USER_ERROR);  
    }
 }
function extracting_data($table, $fields,$condition,$order,$limit){
  $query="SELECT ".$fields."
    FROM ".$table."
    WHERE id =".$this->sql_quote($condition)."
    ORDER BY ".$order."
    LIMIT ".$limit." ";
  //echo $query;
  if($stmt = $this->conn->prepare($query)) {
   $stmt->execute();
   $row = array_pad(array(), $stmt->field_count, '');
   $params = array();
    foreach($row as $k=>$v) {
      $params[] = &$row[$k];
    }
   call_user_func_array(array($stmt,'bind_result'),$params);
   $result = array();
            while($stmt->fetch()) {
                foreach ($row as $b=>$elem) {
                 $vals[$b]=$row[$b];
                }
                $result[]=$vals;
            }
            $stmt->close();
            return $result;

  }

 }
 function sql_quote( $value )
 {
     if( get_magic_quotes_gpc() )
     {
         $value = stripslashes( $value );
     }
    //check if this function exists
     if( function_exists( "mysql_real_escape_string" ) )
     {
           $value = mysql_real_escape_string( $value );
     }
    //for PHP version < 4.3.0 use addslashes
     else
     {
           $value = addslashes( $value );
     }
     return $value;
 }

}

Now to call the function I am using ::>

    $connection=New mysql();
$extract=$connection->extracting_data("tablename","id,name,points","$_GET['id']","date desc","0,10");

The function returns a multi-dimensional array in $result and stores it in $extract ,depending on the data I want to extract.. Any improvements or other suggestions would be appreciated ...

A: 
  1. You should watch where the parameters for your function come from. If they come from an unreliable source, then it's very insecure.

    If someone passes something like 1 ; DROP TABLE tablename ; SELECT * FROM dual WHERE 1 in the $condition parameter, you'll get the Little Bobby Tables scenario.

  2. Your query will look like the following:

     SELECT  id, name, points
     FROM    tablename
     WHERE   id
     ORDER BY
             DATE DESC
     LIMIT 0, 10
    

    The id here will be casted to BOOLEAN, and the query will select all ids except 0 and NULL.

    Is it really what you want?

    You probably want to change your $condition to 'id = $id' or something like that.

  3. Do you really need this level of abstraction: generating queries from uknown tables with unknown fields but with predefined SELECT / FROM / ORDER BY / LIMIT stucture?

Quassnoi
Yes, I am planning to take care of the parameters apart from that, are their any redundant lines of code or some bad practices maybe....
halocursed
will it not select only the 10 id's due to limit 0,10 or should I remove the where clause if I don't need it in some of my queries
halocursed
.Creating different functions for every extraction would increase the size of the class and the number of functions, so I thought why not create a single function to extract all kinds of data...
halocursed
+1  A: 

Instead of binding the results and having to do loads of looping, you could simply use mysqli::query() and mysqli_result::fetch_all().

if($stmt = $this->conn->query($query)) {
    $result = $stmt->fetch_all(MYSQLI_ASSOC);                
    $stmt->close();
    return $result;
}

You would be better off binding your input variables rather than building up an SQL string containing them, but that may not be feasible using your current approach.

Edit

Sorry, I was an idiot and didn't notice that fetch_all() is only in PHP >= 5.3. You can still do this though which is simpler:

if($stmt = $this->conn->query($query)) {
    $result = array();
    while ($row = $stmt->fetch_assoc()) {
        $result[] = $row;
    }               
    $stmt->close();
    return $result;
}
Tom Haigh
I would need php ver 5.3 for that???...
halocursed