Making CRUD abstracted class in PHP

Question

I did a working CRUD abstracted class, and I am not sure that this is a good way for it, so I would like to hear your opinion.

protected function properties(){
 $properties = array();
 foreach (self::$table_fields as $db_field){
 if(property_exists($this, $db_field)){
 $properties[$db_field] = $this->$db_field;
 }
 }
 return $properties;
}
public function create(){
 $db = db::getConnection()->conn;
 $properties = $this->properties();
 $key = implode(",",array_keys($properties));
 $value = implode(",:",array_keys($properties));
 $sql = "INSERT INTO ".self::$table."(" . $key . ") ";
 $sql.= "VALUES (:" . $value . ")";
 $stmt = $db->prepare($sql);
 foreach($properties as $key=>$value){
 $stmt->bindParam($key,$value);
 }
 if($stmt->execute()){
 print_r($stmt);
 $this->id = db::the_insert_id();
 return true;
 }else{
 print_r($stmt);
 return false;
 }
 return $stmt;
}

This is working well: I am getting TRUE as response and it creates a user in database, but I am just not sure about it.

• 200_success: Just to be clear — what class is this, and what is db::?
• Никола Р.: db is database and its calling from another class. it's static method, i have also auto loader.
• 200_success: I know that db stands for "database", but what is that code? Mysqli?
• Никола Р.: it is PDO github.com/anonunder/firstep/blob/master/singleton_db.php

Answer 1

1. Format all identifiers in order to avoid a syntax error. Assuming it's mysql, use backticks for the purpose:

   $key = "`".implode("`,`",array_keys($properties))."`";
   

2. You are going to repeat the second half of the code in the every crud method. To avoid that, add a query() method to your db class

   public function query($sql, $params = []) {
    $stmt = self::getConnection()->conn->prepare($sql);
    $stmt->execute($params);
    return $stmt;
   }
   

   so you'll be able to make your crud methods much more concise.

   public function create(){
    $properties = $this->properties();
    $key = "`".implode("`,`",array_keys($properties))."`";
    $value = implode(",:",array_keys($properties));
    $sql = "INSERT INTO `".self::$table."` (" . $key . ") ";
    $sql.= "VALUES (:" . $value . ")";
    db::query($sql, $properties);
    $this->id = db::the_insert_id();
   }
   

3. There is no point in returning true or false from such a method, the only reason for it to return false is a database error and such an error should be thrown in the form of Exception and handled elsewhere, making all this true false stuff rather useless.

4. Your current setup is following Active Record pattern, making a data object to contain all the database interaction related code. Consider switching to Data Mapper pattern where you have two classes - your data class and a mapper that is responsible for all the database interactions.

• Никола Р.: 2. What about binding params? What is the difference between using query and bindparam? also, I already have that method. Thanks.
• Your Common Sense: Don't you realize that your method doesn't use a prepared statement?
• Никола Р.: It is using, it is just automatic and that is why I asked is that good way to do it. This is output: prntscr.com/i601uc
• Your Common Sense: From the link you posted above, it is clear that your db:query() does NOT use a prepared statement, so I have no idea what are you talking about.
• Никола Р.: $stmt = $db->prepare($sql); foreach($properties as $key=>$value){ $stmt->bindParam($key,$value); } $stmt->execute() This isn't prepared statement? I'm not executing the script from query method.. look my firstpost.