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.

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.