3
\$\begingroup\$

I'm trying to code a simple and scalable PHP framework for my own use. Could you please offer some critiques or mandatory improvements for these two classes?

<!-- language: PHP -->
<?php

class Xmysqli extends mysqli
{
    public function __construct($dataDB = array())
    {
        global $config, $debug, $err;

        if (empty($dataDB['host']))
            $dataDB['host'] = $config['db']['host'];
        if (empty($dataDB['user']))
            $dataDB['user'] = $config['db']['user'];
        if (empty($dataDB['pass']))
            $dataDB['pass'] = $config['db']['pass'];
        if (empty($dataDB['base']))
            $dataDB['base'] = $config['db']['base'];

        parent::__construct($dataDB['host'],$dataDB['user'],$dataDB['pass'],$dataDB['base']);
        if (mysqli_connect_error())
        {
            $error_message = "mysqli::connect(): Connect Error (' . mysqli_connect_errno() . ') ' . mysqli_connect_error() . ";
            if ($config['core']['debug'])
                $debug['db']['connect'] = $error_message;
            else
                $err->set($error_message);
        }
        if(!$this->set_charset("utf8"))
            $err->set('mysqli::set_charset(): Error while loading utf8 charset');
    }

    public function query($q)
    {
        global $config, $debug, $err;
        $start = microtime(true);
        $res = parent::query($q);
//    my_dump($q,false);
        $end = microtime(true);
        if ($config['core']['debug'])
        {
            $time = ($end - $start);
            $debug['db']['counter']++;
            $debug['db']['timer'] += $time;
            $debug['db']['qry'][$debug['db']['counter']]['query'] = $q;
            $debug['db']['qry'][$debug['db']['counter']]['time'] = $time;
            $debug['db']['qry'][$debug['db']['counter']]['rows'] = $this->affected_rows;
        }
        if (!$res)
        {
            $error = $this->error;
            $iid = $_SERVER["REMOTE_ADDR"]."-".$_SERVER["REMOTE_PORT"]."-".$_SERVER["SERVER_ADDR"]."-".substr(md5($error), 0, 8);

            $name = 'SqlLog-'.date('Y_m_d') . '.err';
            $errtxt = "\n$iid\n\n".
            "QUERY: $q\n".
            "Error: $error\n".
            "Date: ".date("r")."\n".
            "path: ".$_SERVER["REQUEST_URI"]."\n".
            "backtrace: ".backtrace().
            "-------------\n";

            if ($fd = fopen('../tmp/logs/' . $name, 'a'))
            {
                fputs($fd,$errtxt);
                fclose($fd);
            }
            else
                saveAccessLog("ERR", "Echec ouverture fichier " .$name);
//die($errtxt);
            $err->set("An error occured please send this code to  <a href='mailto:".$config['core']['contact']."'>".$config['core']['contact']."</a>#".$iid);
        }
        return ($res);
    }
    public function prepare($data)
    {
        return $this->real_escape_string($data);
    }

    public function fetch_all($query)
    {
        $res = $this->query($query);
        $all = array();

        if (!is_object($res))
            die('Query ERROR: ');

        while($row = $res->fetch_array(MYSQLI_ASSOC))
        {
            $all[] = $row;
        }
        return $all;
    }

    public function fetch_array($query)
    {
        $res = $this->query($query);
        return $res->fetch_array(MYSQLI_ASSOC);
    }

    public function num_rows($q)
    {
        $res = $this->query($q);
        return $res->num_rows;
    }
}






<!-- language: PHP -->      
abstract class Model
{
protected $link; // (resource) link for mysqli connection
public $insert_id; //  
protected $affected_rows; // 
protected $fields; // (array) list of table fields
protected $primaryKey; // (string) PK name 
protected $tableName; // (string) Table name
protected $listfields; // (array)  list of table fields setted by child class

public function __construct()
{
    global $mysqli_link, $err;

    $this->link = $mysqli_link;

    if (empty($this->tableName))
        $err->set('Cannot create new Model: $this->tableName is not defined');
    if (empty($this->primaryKey))
        $err->set('Cannot create new Model: $this->primaryKey is not defined');

    $this->setFields();
}

protected function setFields()
{
    global $err;

    if (!empty($this->listfields))
        $this->fields = $this->listfields;
    else
    {
        $q = "SHOW COLUMNS FROM `".$this->tableName."`";

        $result = $this->link->fetch_all($q);
        foreach($result as $column)
        {
            if($column['Key'] == 'PRI')
                $this->primaryKey = $column['Field'];

            $this->fields[] = $column['Field'];
        }
    }

    if (empty($this->fields))
        $err->set('setFields(): no fields defined for table '.$this->tableName);
}

protected function format_opt($opt)
{
    $clauses = '';
    $nb_clauses = 0;
    if (isset($opt['related']) AND is_array($opt['related']))
    {
        foreach ($opt['related'] AS $clause)
        {
            if (!empty($clause['type']))
                $clauses .= ' '.strtoupper($clause['type']). ' JOIN ';
            else
                $clauses .= ' LEFT JOIN ';
            $clauses .= '`'.$clause['table_to'].'`';
            $clauses .= ' ON `'.$clause['table_to'].'`.`'.$clause['field_to'].'` = `'.$clause['table_from'].'`.`'.$clause['field_from'].'`';
        }
    }
    $nb_clauses = 0;
    if (isset($opt['where']) AND is_array($opt['where']))
    {
        foreach ($opt['where'] AS $clause)
        {
            if ($nb_clauses == 0)
                $clauses .= ' WHERE ';
            else
                $clauses .= ' AND ';
            if (!empty($clause['table']))
                $clauses .= "`".$clause['table']."`.";
            else
                $clauses .= "`".$this->tableName."`.";
            $clauses .= "`".$clause['field']."` ". $clause['op'];
            $clauses .= ($clause['op'] !== 'NOT IN' AND $clause['op'] !== 'IN' AND $clause['value'] !== 'NOW()') ? " '".$this->link->prepare($clause['value'])."'" : " ".$this->link->prepare($clause['value']);
            $nb_clauses++;
        }
    }
    if (isset($opt['groupby']) AND is_array($opt['groupby']))
    {
        $clauses .= ' GROUP BY ';
        if (!empty($opt['groupby']['table']))
            $clauses .= '`'.$opt['groupby']['table'].'`';
        else
            $clauses .= '`'.$this->tableName.'`';
        $clauses .= '.`'.$opt['groupby']['field'].'`';
    }
    $nb_clauses = 0;
    if (isset($opt['orderby']) AND is_array($opt['orderby']))
    {
        foreach ($opt['orderby'] AS $clause)
        {
            if ($nb_clauses == 0)
                $clauses .= ' ORDER BY ';
            else
                $clauses .= ', ';
            if (!empty($clause['table']))
                $clauses .= "`".$clause['table']."`.";
            else
                $clauses .= "`".$this->tableName."`.";      
            $clauses .= "`".$clause['field']."`";
            if (!empty($clause['sort']))
                $clauses .= " ".$clause['sort'];          

            $nb_clauses++;
        }
    }
    if (isset($opt['limit']) AND is_array($opt['limit']))
    {
        $clauses .= ' LIMIT ';
        if (!empty($opt['limit']['start']))
            $clauses .= $opt['limit']['start'].',';
        $clauses .= $opt['limit']['len'];
    }
    return $clauses;
}

public function hydrate()
{
    global $err;

    if (empty($this->{$this->primaryKey}))
        $err->set(get_called_class().': cannot hydrate without primary key value');

    $q = "SELECT * FROM `".$this->tableName."` WHERE `".$this->primaryKey."` = ".intval($this->{$this->primaryKey});
    $data = $this->link->fetch_array($q);

    foreach ($this->fields as $key => $name)
    {
        $setter = 'set_'.$name;
        $this->$setter($data[$name]);
    }
}

public function delete($opt = array())
{ 
    if (empty($opt)) 
    {
        $q = "DELETE FROM `".$this->tableName."`";
        $count = 0;
        foreach ($this->fields as $k =>$name)
        {
            $getter = 'get_'.$name;
            $value = $this->$getter();

            if (!empty($value))
            {
                if ($count == 0)
                    $q .= " WHERE ";
                else
                    $q .= " AND ";
                $q .= "`".$this->tableName."`.`".$name."` = '".$value."'";
                $count++;
            }
        }

    }
    else 
        $q = "DELETE FROM `".$this->tableName."`" . $this->format_opt($opt);
    $this->link->query($q);
    $this->affected_rows = $this->link->affected_rows;
}

public function save()
{
    $count = 0;
    $nb_fields = count($this->fields);
    if($this->{$this->primaryKey} != null)
    {
        $q = "UPDATE `".$this->tableName."` SET ";
        foreach ($this->fields as $key => $name)
        {
            if($count < ($nb_fields - 1))
                $q .= "`".$name."` = '".$this->link->prepare($this->$name)."',";
            else
                $q .= "`".$name."` = '".$this->link->prepare($this->$name)."'";

            $count++;
        }
        $q .= " WHERE `".$this->primaryKey."` = '".intval($this->{$this->primaryKey})."'";
        $this->link->query($q);
        $this->affected_rows = $this->link->affected_rows;
    }
    else
    {
        $q = "INSERT INTO `".$this->tableName."` (";
            foreach ($this->fields as $key => $name) 
            {
                if($count < ($nb_fields - 1))
                    $q .= "`".$name."`,";
                else
                    $q .= "`".$name."`)";

$count++;
}


$count = 0;

$q .= " VALUES (";
    foreach ($this->fields as $key => $name) 
    {
        if($count < ($nb_fields - 1))
            $q .= "'".$this->link->prepare($this->$name)."',";
        else
            $q .= "'".$this->link->prepare($this->$name)."')";

$count++;
}
$this->link->query($q);
$this->insert_id = $this->link->insert_id; 
}


}
public function format_select($opt)
{
    $out = '';
    if (!empty($opt['constraint']))
        $out .= ' ' .strtoupper($opt['constraint']);
    if (!empty($opt['fields']))
    {
        $nb_field = 0;
        foreach ($opt['fields'] AS $field)
        {
            if ($nb_field !== 0)
                $out .= ', ';
            if (!empty($field['function']))
                $out .= $field['function'].'(';
                    if (!empty($field['table']))
                        $out .= ' `'.$field['table'].'`.';
                    $out .= $field['field'];
                    if (!empty($field['function']))
                        $out .= ')';
if (!empty($field['as']))
    $out .= ' AS "'.$field['as'].'"';
$nb_field++;
}
}
return $out;
}
public function getAll($opt = array(), $first_only = FALSE)
{
    if (empty($opt)) 
    {
        $q = "SELECT `".$this->tableName."`.`id` FROM `".$this->tableName."`";
        $count = 0;
        foreach ($this->fields as $k =>$name)
        {
            $getter = 'get_'.$name;
            $value = $this->$getter();

            if (!empty($value))
            {
                if ($count == 0)
                    $q .= " WHERE ";
                else
                    $q .= " AND ";
                $q .= "`".$this->tableName."`.`".$name."` = '".$value."'";
                $count++;
            }
        }
        if ($first_only === TRUE)
            $q .= " LIMIT 1";
    }
    else 
        if (!empty($opt['select']))
            $q = "SELECT ".$this->format_select($opt['select'])." FROM `".$this->tableName."`" . $this->format_opt($opt);
        else
            $q = "SELECT `".$this->tableName."`.`id` FROM `".$this->tableName."`" . $this->format_opt($opt);

        $ids = $this->link->fetch_all($q);
        $objs = array();
        if (!empty($opt['select']))
            return $ids;
        else
        {
            $child_class_name = get_called_class();

            foreach($ids as $id)
            {
                $obj = new $child_class_name();
                $obj->set_id($id['id']);
                $obj->hydrate();
                if ($first_only === TRUE) 
                    return $obj;
                $objs[] = $obj;
            }
        }
        return $objs;
    }
}  
?>
\$\endgroup\$
1
\$\begingroup\$

Sorry, but I would not use this class. Please check the SOLID design.

  1. Instead of passing an array in the xmysqli construct and expecting some array keys, I would just pass the variables directly:

    public function __construct($host,$username,$password,$database)
    

    but then it would be the same as a MySQLi base class. I would instead create a database configuration class and use typehints in construct.

    public function __construct(XmysqliConfig $config)
    

    If someone passes something different into your database class, he/she will see an error.

  2. Please remove all your globals. You have a class which contains properties, so just don't use globals.

  3. Don't use profiling and logging within a function. Create a method to add a profiler class and a method to add a logger.

  4. Your table creation methods or table modifications don't belong to a user model. You instead require that a Schema class doctrine is using a schema manager for this. Your model has just the responsibility of the data inside the table, not the table itself.

This was just an overview of the points, but there are more.

\$\endgroup\$
1
\$\begingroup\$

My opinion:

@BlackScorp said:

instead of passing an array in xmysqli construct und expect some array keys, i would just pass the variables directly

I'd keep it as is, since personally, I don't really like methods with long argument signatures, where you need to split your code to match the signature, and inside the class, the code is just build together again. However, I would optimize the options handling by merging the given options with default ones, eg.:

$this->config = array_merge($defaultConfig, $givenConfig)

I can more or less +1 the statements from @BlackScorp. Don't use globals, and make profiling and logging optional by moving it somewhere else.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.