I am posting the a section of the model of my application.
Any/all feedback would be very helpful as I am trying to build my PHP skills, but I will ask a couple specific questions that might help others as well.
Is my escaping, as is, effective enough?
I tried to separate my model into a file that will deal with functions exclusive to 'Groups' and a file that will deal with database functions used by all sections of the app. Is this a good strategy?
My Group model could use some more refactoring, but what do you think of the way I take and return data? (by using $data and $input, $data being the variable that is returned the controller and will be eventually sent to the view via JSON)
What are your thoughts about 'public function show_one($user_id, $group_id, $privacy)' which is meant to display one groups information, given proper privacy settings?
Any other input will be greatly appreciated. I am working hard to improve my programming skills everyday.
Group Model
2) The Group Model:
<?php
require_once("database.php");
class model {
///data is returned to the controller
public $data = array(
'success'=>0,
'msg' =>'There was a small problem, try again soon.',
'data_type' => 'group',
'action' => '',
'results'=> array(),
);
//anything that will be put into the DB will be held in input
public $input = array();
public $exclude =array();
public function __construct($a) {
Global $db;
$db->escape($a);
$this->input = $db->escape($a);
@$this->data['action'] = $a['action'];
}
//move insert and data return up here
}
class create_group_model extends model {
public function insert_new_group($a) {
Global $db;
$b = $db->insert_array('group', $a, 'action');
if ($b['mysql_affected_rows']===1) {
$this->data['success'] = 1;
$this->data['msg'] = 'Congrats, you created a new group.';
array_push($this->data['results'], $b['mysql_insert_id']);
return $this->data;
} else {
$this->data['success'] = 0;
$this->data['msg'] = 'No group created, try again soon.';
return $this->data;
}
}
}
class search_group_model extends model {
public function default_list_groups() {
$this->list_groups($this->input['lat'], $this->input['lng'], 'group', 10, 30, $this->input['user_id'], 'all' );
$this->data['msg'] = 'Updated';
return $this->data;
}
public function custom_list_groups() {//add custom settings
$this->list_groups($this->input['lat'], $this->input['lng'], 'group', 10, 10);
}
public function my_list_groups($id){
Global $db;
//fix distance 10000 here, so it doesnt take into account distance
$b = $db->def_g_list($this->input['lat'], $this->input['lng'], 'group', 10000, 30, (int)$id, 'mine');
if ($b !== 0) {
$this->data['success'] = 1;
while ($row = mysql_fetch_array($b, MYSQL_ASSOC)) {
array_push($this->data['results'], $row);
}
$this->data['msg'] = 0;
return ($this->data);
} else {
$this->data['msg'] = 'There was a small problem, try again soon.';
$this->data['success'] = 0;
return ($this->data);
}
}
public function show_one($user_id, $group_id, $privacy) {
Global $db;
(bool)$confirm = FALSE;
if($privacy === 0){
$confirm = TRUE;
}else {
$confirm = FALSE;
$privacy = 1;
}
if(!$confirm){
$s = 'group_id';
$f = 'user_group_rel';
$w = sprintf("user_id =%d AND group_id=%d",$user_id, $group_id);
$b = $db->exists($s,$f,$w);
if(mysql_num_rows($b) ===1 && !is_num($b)) {
$confirm=true;
}
}
if($confirm){
$s = 'group_id,group_name,location_name,description,
user_id,lat,lng,created_by_name,state';
$f = 'group';
$w = sprintf("group_id=%d AND privacy=%d",(int)$group_id, (int)$privacy);
$b = $db->exists($s,$f,$w);
if(mysql_num_rows($b) ===1 && !is_int($b)) {
$this->data['success'] = 1;
$this->data['msg'] = 0;
while ($row = mysql_fetch_array($b, MYSQL_ASSOC)) {
array_push($this->data['results'], $row);
}
$this->data['results'][0]['people']= $this->group_mems($user_id, $group_id);
$this->data['results'][0]['total_people']= $this->group_mems_count($group_id);
return ($this->data);
}
}
$this->data['msg'] = 'There was a small problem, try again soon.';
$this->data['success'] = 0;
echo 'still going';
return ($this->data);
}
public function group_mems($me, $group_id) {
Global $db;
$result = array();
$q = sprintf("
SELECT
t1.group_id, t2.user_id, t2.username, t2.first_name, t2.last_name, t2.user_id = %d as me
FROM user_group_rel t1
LEFT JOIN users t2 ON t1.user_id = t2.user_id
WHERE group_id = %d and status = 1
ORDER BY me desc",
(int)$me, (int)$group_id);
$b = $db->query($q);
if ($b !== 0 and is_resource($b)) {
while ($row = mysql_fetch_array($b, MYSQL_ASSOC)) {
array_push($result, $row);
}
return $result;
} else {
return 'err';
}
}
public function group_mems_count($group_id) {
Global $db;
$result = array();
$q = sprintf("
SELECT
count(t1.user_id) as people
FROM user_group_rel t1
WHERE group_id = %d and t1.status = 1",
(int)$group_id);
$b = $db->query($q);
if ($b !== 0 and is_resource($b)) {
while ($row = mysql_fetch_array($b, MYSQL_ASSOC)) {
array_push($result, $row);
}
return $result[0]['people'];
} else {
return 'err';
}
}
private function list_groups($lat, $lng, $table, $distance, $limit, $id, $whos) {
Global $db;
$b = $db->def_g_list($lat, $lng, $table, $distance, $limit, (int)$id, $whos);
if ($b !== 0) {
$this->data['success'] = 1;
while ($row = mysql_fetch_array($b, MYSQL_ASSOC)) {
array_push($this->data['results'], $row);
}
return ($this->data);
} else {
$this->data['msg'] = 'There was a small problem, try again soon.';
$this->data['success'] = 0;
return ($this->data);
}
}
}
class join_group_model extends model {
public function join_group() {
Global $db;
$pass = 0;
if(array_key_exists('password', $this->input) && strlen($this->input['password'])>20) {
$pass = $db->pass_check('group', $this->input['group_id'],$this->input['password'] );
}
else {
$pass = $db->pass_check('group', $this->input['group_id'],'NULL' );
}
if($pass !==0) {
array_push($this->exclude, 'password', 'action');
$b = $db->insert_array('user_group_rel',$this->input, $this->exclude);
//echo print_r($b);
if ($b !== 0) {
$this->data['success'] = 1;
$this->data['results'] = $this->input['group_id'];
$this->data['msg'] = ' you joined a new group. ';
return ($this->data);
} else {
$this->data['msg'] = 'There was a small problem, try again soon.';
$this->data['success'] = 0;
return ($this->data);
}
}
}
public function unjoin_group() {
Global $db;
$b = $db->delete('user_group_rel', (int)$this->input['user_id'], (int)$this->input['group_id']);
if ($b !== 0) {
$this->data['success'] = 1;
$this->data['results'] = $this->input['group_id'];
$this->data['msg'] = ' you left that group. ';
return ($this->data);
} else {
$this->data['msg'] = 'There was a small problem, try again soon.';
$this->data['success'] = 0;
return ($this->data);
}
}
}
Database
3) Application wide database methods. A couple notes:
a) Probably the most glaring mistake here is not using PDO statements.
<?php
require_once('../config/config.php');
class MySQLDatabase {
private $connection;
public $last_query;
private $magic_quotes_active;
private $real_escape_string_exists;
function __construct() {
$this->open_connection();
$this->magic_quotes_active = get_magic_quotes_gpc();
$this->real_escape_string_exists = function_exists( "mysql_real_escape_string" );
}
public function open_connection() {
$this->connection = mysql_connect(DB_SERVER, DB_USER, DB_PASS);
if (!$this->connection) {
die("Database connection failed: " . mysql_error());
} else {
$db_select = mysql_select_db(DB_NAME, $this->connection);
if (!$db_select) {
die("Database selection failed: " . mysql_error());
}
}
}
public function close_connection() {
if(isset($this->connection)) {
mysql_close($this->connection);
unset($this->connection);
}
}
public function query($sql) {
$this->last_query = $sql;
$result = mysql_query($sql, $this->connection);
$this->confirm_query($result);
return $result;
}
public function escape($q) {
if(is_array($q))
foreach($q as $k => $v)
$q[$k] = $this->escape($v); //recursive
else if(is_string($q)) {
$q = mysql_real_escape_string($q);
}
return $q;
}
private function confirm_query($result) {
if (!$result) {
$output = "Database query failed: " . mysql_error() . "<br /><br />";
die( $output );
}
}
public function exists($s,$f, $w) {
//rewrite bottom 2 functions rid of this function
$q = sprintf("SELECT %s FROM %s WHERE %s LIMIT 1",$s,$f, $w);
//echo $q;
$result = $this->query($q);
if (mysql_num_rows($result)===0) {
return 0;
} else if (mysql_num_rows($result)===1) {
return $result;
} else{
return 0;
}
}
public function pass_check($t, $id, $p) {
if ($p==='NULL'){
$q = sprintf("SELECT * FROM %s WHERE %s_id = %s AND password is NULL", $t,$t,$id);
}
else{
$q = sprintf("SELECT * FROM %s WHERE %s_id = %s AND password = '%s'", $t,$t,$id,$p);
}
$result = $this->query($q);
if (mysql_num_rows($result)===0) {
return (int)0;
} else if (mysql_num_rows($result)>0) {
return $result;
}
}
public function insert_array($table, $data, $exclude = array()) {
$fields = $values = array();
if( !is_array($exclude) ) $exclude = array($exclude);
foreach( array_keys($data) as $key ) {
if( !in_array($key, $exclude) ) {
$fields[] = "`$key`";
$values[] = "'" .$data[$key] . "'";
}
}
$fields = implode(",", $fields);
$values = implode(",", $values);
if( mysql_query("INSERT INTO `$table` ($fields) VALUES ($values)") ) {
return array( "mysql_error" => false,
"mysql_insert_id" => mysql_insert_id(),
"mysql_affected_rows" => mysql_affected_rows(),
"mysql_info" => mysql_info()
);
} else {
echo print_r(array( "mysql_error" => mysql_error() ));
return 0;
}
}
public function def_g_list($lat, $lng, $table, $dist, $limit, $id, $whos='all') {
//either refactor to make this function more flexible or get rid of table variable
$where = '';
if(is_int($id) && $id>0 && $id < 100000 && $whos == 'all'){
//subquery used to display only groups the user is NOT IN- probably a better way to do this
$where = sprintf("
t2.group_id NOT IN (
SELECT user_group_rel.group_id
FROM user_group_rel
WHERE user_group_rel.user_id =%d)",
(int)$id);
} else if (is_int($id) && $id>0 && $id < 100000 && $whos == 'mine') {
$where = 't3.user_id = ' . (int)$id;
} else {
return 0;
}
$d_formula = $this->distance_formula($lat, $lng, 't1');
//sorry for this query
$q = sprintf("
SELECT
t1.group_id, t1.group_name, t1.location_name, t1.description, t1.lat, t1.lng, t1.privacy,t2.people, %s
FROM %s AS t1
JOIN (SELECT group_id, count(group_id) as people
FROM user_group_rel
GROUP BY group_id) t2
ON t1.group_id = t2.group_id
JOIN (SELECT user_group_rel.group_id, user_group_rel.user_id
FROM user_group_rel ) t3
ON t1.group_id = t3.group_id
WHERE %s
GROUP BY t1.group_id
HAVING distance < %s
ORDER BY distance
LIMIT %s",
$d_formula, $table, $where,$dist, $limit );
$result = $this->query($q);
if (mysql_num_rows($result)===0) {
return 0;
} else if (mysql_num_rows($result)>0) {
return $result;
}
}
function delete($table,$uid,$cid) {
$q = sprintf('
DELETE FROM `disruptly`.`%s`
WHERE `%s`.`user_id` = %d
AND `%s`.`group_id` = %d
',$table, $table, $uid,$table, $cid
);
//echo $q;
$result = $this->query($q);
if ($result===0) {
return 0;
} else if ($result===1) {
return $result;
}
}
public function distance_formula($lat, $lng, $table) {
//get rid of the round after'SELECT *,' for more accurate results
$q = sprintf("round(3956 * 2 * ASIN(SQRT( POWER(SIN((%s -abs(%s.lat)) * pi()/180 / 2),2) + COS(%s * pi()/180 ) * COS(abs(%s.lat) * pi()/180) * POWER(SIN((%s - %s.lng) * pi()/180 / 2), 2) )),2) as distance ", $lat, $table, $lat, $table, $lng, $table);
return $q;
}
}
$database = new MySQLDatabase();
$db =& $database;
Edit - If anyone disagrees or has something to add, don't be shy.
I'm going to ask some follow-up questions:
You said, "...these are models whose functionality is designed for its controller rather than a controller whose functionality is designed for its model." This was spot on. I started coding my "controller" in mind first, or rather what data will the view be requesting from the model. In the future, I should construct my model first?
Globals are evil, lesson (re)learned. Never again. No question here, just a fact.
You say, "Let me just say that this is not the implementation I am suggesting..." I imagine in reference to my overall structure. I'm not asking you to outline a new implementation, but how you would approach/think about it differently. (2-3 sentences is fine)
"Why are all of your methods returning the $data property?" It is a phonegap application, so the whole 'view' is already on the client. The controller sends $data(JSON) for each ajax request for the info to be filled in. This might be the root of the botched MVC pattern, this was my thinking:
View: Client-side app, which has all the templates already loaded and fills them with requests to the controller via ajax
Controller: Skinny gatekeeper between client and model - not much going on here besides accepting get/post requests, validation(does username have 5 letters?) and forwarding data to the appropriate functions in the model. Then passing the model's data relatively untouched to the 'view'(will add an html sanitizing function).
Model (Group): All methods/classes pertaining to group functionality (SQL Statement prep, parse and return).
Model (Database): Where all the application wide sql queries actually get escaped, executed and returned 'untouched'
Q) Am I correct in my understanding that you're saying the Model(Group) looks more like a controller than a model because of the parsing?
- "There is no reason to typecast something that is already of the type you are trying to cast it to." My reasoning for using typcasting and sprintf(with %d) was perhaps a failed attempt at data integrity/security. For example, I expect user_id to be a number but what if someone sends a string or an sql injection technique? I thought those were 2 ways of potentially saving yourself from a 1=1. You say no not worth it? (probably the most correct answer is just use PDO).
@$this->data['action'] = $a['action'];
. If this buggers up, you'll never know. \$\endgroup\$