Problem
I think I put too much code in my controller that was supposed to go in the model.
This is a part of my controller, I’m not gonna paste everything since there is a lot of code.
public function ajaxUsers() {
if($_GET["action"] == "listUsers") {
if(!isset($_POST["search"])) {
$this->_data['Records'] = $this->_model->getUsers();
$this->_data['Result'] = "OK";
$this->_data['TotalRecordCount'] = $this->_model->countUsers();
}
else {
foreach($_POST['fields'][0] as $key => $post) {
if ($post != "" && $key != "reg_date") {
$searchTerms = explode(' ', $post);
foreach ($searchTerms as $term) {
$term = trim($term);
if (!empty($term)) {
$like[] = $key." LIKE '%".trim($term, ''')."%'";
}
}
}
else if ($post != "" && $key == "reg_date") {
foreach ($post[0] as $key2 => $date) {
$datetofrom = strtotime($date);
$datetofrom = date('Y-m-d', $datetofrom);
if ($date != "" && $key2 == "datefrom") {
$like[] = "DATE_FORMAT(".$key.", '%Y-%m-%d') >= '".$datetofrom."'";
}
if ($date != "" && $key2 == "dateto") {
$like[] = "DATE_FORMAT(".$key.", '%Y-%m-%d') <= '".$datetofrom."'";
}
}
}
}
($like) ? $where_clause = "WHERE ". implode(' AND ', $like) : $where_clause = "";
$this->_data['Records'] = $this->_model->filterUsers($where_clause);
$this->_data['Result'] = "OK";
$this->_data['TotalRecordCount'] = $this->_model->countfilterUsers($where_clause);
}
echo json_encode ($this->_data);
}
}
And my model is mostly database queries:
public function getUsers() {
$data = $this->_db->select("SELECT * FROM ".PREFIX."users" . $this->_sort);
return $data;
}
public function countUsers() {
$data = $this->_db->select("SELECT count(*) as id FROM ".PREFIX."users");
return $data[0]->id;
}
public function filterUsers($like_clause) {
$data = $this->_db->select("SELECT * FROM ".PREFIX."users " .$like_clause. $this->_sort);
return $data;
}
public function countFilterUsers($like_clause) {
$data = $this->_db->select("SELECT count(*) as id FROM ".PREFIX."users ".$like_clause);
return $data[0]->id;
}
Should I moved the foreach loops in the model?
Solution
Should I moved the foreach loops in the model?
Yes, definitely.
Currently, you have SQL in the model and the controller, which isn’t a good idea. It makes it hard to make out what the actual query will be, and splitting up one functionality (database lookup in this case) in multiple classes is not a good idea in general. SQL should be part of the user model (or DAO), so create a method called searchBy($keywordsArray) there and use that.
Security
You are completely open to SQL injection, as you put $_POST values directly into your query as column name and like clause. When putting variable content into queries, you should always use prepared statements to defend against SQL injection if possible.
When it’s not possible, for example with variable column names, then you should prepare a whitelist of your columns, and check if the user input is in it.