L
L
lexstile2021-07-03 14:21:47
PHP
lexstile, 2021-07-03 14:21:47

What is the correct way to write a model method with dynamic fields?

The user has 10 fields.
From 1 to 5 fields come from the front, they need to be changed in the database, I don’t want to write a separate method in the model for each field and call it one by one.

I threw in such a solution +/-, is it possible to somehow simplify it, and maybe I didn’t take something into account in it?
Perhaps there are some functions that can be used instead of a loop? (Have not found)

public function updateUser($params) {
    $fields = '';

    foreach ($params as $key => $value) {
      if ($key !== 'user_id') {
        $fields .= $key . ' = :' . $key . (end($params) !== $value ? ', ' : '');
      }
    }

    $this->db->query('UPDATE users SET ' . $fields . ' WHERE user_id = :user_id', $params);
  }

Answer the question

In order to leave comments, you need to log in

1 answer(s)
F
FanatPHP, 2021-07-03
@lexstile

The question is very good. And the code is good overall. Mistakes are common, everyone makes them.
Let's start with the fact that this is not a model.
The model is the entire business logic of the application.
And this is a primitive class for manipulating a single table in a database. Suitable for the TableGateway pattern.
Moreover, it does not use any of the advantages of OOP.
This class is a stupid collection of functions. And for the next table you will have to write exactly the same.
Accordingly, you first need to make the code universal, and move it to an abstract class.
At the same time, fixing one critical error in it (with security) and one logical one.
What happens if the values ​​of the last two elements of the array are the same? Then it is necessary to compare keys, instead of values.
And what for in general such tricks with search of the last element if you have a $fields variable? If it is empty, then this is the first turn of the cycle. That's all, that's enough
As for security, I'm already tired of explaining.
We carefully protect the values ​​passed to the request, but at the same time skip the injection through field names without blinking an eye.
The list of fields should always be written explicitly. Moreover, it will come in handy in a million cases later.

abstract class AbstractTableGateway
{
    protected $db;
    protected $table;
    protected $fields;
    protected $primary = 'id';

    public function __construct(DB $db)
    {
        $this->db = $db;
    }
    public function update(array $params): void
    {
        $this->validate($params);

        $set = "";
        foreach($params as $key => $value)
        {
            if ($key !== $this->primary)
                $set .= ($set ? "," : "") . "`$key` = :$key";
            }
        }
        $sql = "UPDATE `$this->table` SET $set WHERE `$this->primary`=:$this->primary";
        $this->db->query($sql, $params);
    }
    protected function validate($data)
    {
        $diff = array_diff(array_keys($data), $this->fields);
        if ($diff) {
            throw new \InvalidArgumentException("Unknown field(s): ". implode(",",$diff));
        }
    }
}

But in general, I do not recommend using named placeholders for automatic requests. The field name may contain characters (for example, spaces) that are not suitable for placeholder names.
After that, you can already create a user class (and do not repeat, like a parrot, user user 10 times)
class UserGateway extends AbstractTableGateway {
    protected $table = 'users';
    protected $fields = ['email', 'password', 'name', 'birthday'];
    protected $primary = 'user_id';
}

And feel free to refer to it in code
$user = new UserGateway();
$user->update($params);

Didn't find what you were looking for?

Ask your question

Ask a Question

731 491 924 answers to any question