So basically I have a Database class in which you can do the basic operations (Create, Read, Update and Delete) and this is what I came up with for the update method:

public function updateById($table, $id, $updates = []): void
{
        // Generate the SQL statement based on the updates array
        // e.g: updateById('test', 1, [
        //     'column1' => 'newValue1',
        //     'column2' => 'newValue2'
        // ]);

        $st = "UPDATE $table SET ";
        $keys = array_keys($updates);
        $values = array_values($updates);

        for ($i = 0; $i < count($keys); $i  ) {
            if ($i != count($keys) - 1) {
                $st .= $keys[$i] . "='" . $values[$i] . "', ";
            } else {
                $st .= $keys[$i] . "='" . $values[$i] . "' ";
            }
        }

        $st .= "WHERE id=$id";

        $query = $this->connection()->prepare($st);
        $query->execute();
}

It does the trick but it looks messy and inefficient. Is there any way I can improve this piece of code?

CodePudding user response：

I think it's easier to work with arrays than strings.

Also, you used prepare in your code but you aren't actually utilizing it at all. Since you are concatenating values into your query instead of parameterizing, you are still just as vulnerable to SQL injection.

This code should do what you want, in a much more secure way. I've left comments along the way so you can tell what is going on.

It also allows you to pass in an array instead of a single value for the $id. This means you can update based on any column or even multiple columns instead of just the ID.

//pass in the table name and an array of ALL columns used, whether in `$update_ids` or `$updates`
private function verify_table_and_columns($table, $columns) {

    $db = $this->connection();

    //make sure table actually exists in your database
    $query = $db->query('SHOW TABLES');
    $all_tables = $query->fetchAll(PDO::FETCH_COLUMN);

    //if table does not exist, return false, thus preventing the query
    if(!in_array($table, $all_tables)) {
        return false;
    }

    //make sure all the columns used in the query are really in the table
    //concatenating the table should be safe here since it's already verified above
    $query = $db->query("SHOW COLUMNS FROM {$table}");
    $all_columns = array_column($query->fetchAll(), 'Field');

    //if any of the columns in the query do not exist in the table, return false, thus preventing the query
    foreach($columns as $column) {
        if(!in_array($column, $all_columns)) {
            return false;
        }
    }

    //if it gets to this point, return true
    return true;
}

public function updateTable($table, $update_ids, $updates = array())
{

    $db = $this->connection();

    if(!$this->verify_table_and_columns($table, array_merge(array_keys($update_ids), array_keys($updates)))) {
        return false;
    }

    //catch all values to be passed to the query at the end, in the correct order.
    $params = [];

    //loop over update fields to generate the SET clause. 
    //Also add the values to $params in the same order as the keys get added to the query.
    $set_clause_parts = [];
    foreach($updates as $key => $value) {
        $set_clause_parts[] = "`{$key}`=?";
        $params[] = $value;
    }
    $set_clause = implode(', ', $set_clause_parts);

    //add the ids to the query.
    //Also add the values to the $params in the same order as the keys get added to the query.
    //NOTE: MUST BE DONE AFTER THE ABOVE LOOP FOR THE ORDER OF THE VALUES TO BE CORRECT
    $where_clause_parts = [];
    foreach($update_ids as $key => $value) {
        $where_clause_parts[] = "`{$key}`=?";
        $params[] = $value;
    }
    $where_clause = implode(' AND ', $where_clause_parts);

    //prepare the query
    $query = $db->prepare("UPDATE {$table} SET {$set_clause} WHERE {$where_clause}");

    //execute the query, pass named parameters to execute.
    return $query->execute($params);

}