PHP backend for an HTML5/JS mobile app

Question

I would really appreciate any feedback on the back-end of my mobile application. I tried to keep an MVC pattern, where I considered the app to be my view. Perhaps an REST framework would have been good to use, but for now I am not attempting cross domain requests.

Controller

The Group Controller, that receives JSON requests and mediates all activities involved in managing a group.

A couple notes:

1. I have not yet added a validation step to incoming data. I plan on creating a validation class to help me with that.

2. I was planing on putting said validation in the filter method.

3. I still need to add some methods for UPDATE and DELETEing groups, but this should be enough to get a feel for it.

<?php
require_once("../model/group_model.php");
require_once("../helper_validation.php");
header('Content-type: application/json');
//controller
session_start();

$group_array = array('msg'          => 'Oops, we messed up. Try again soon.',
               'success'            => 0,
               'group_action'       => -1,
               'session_logged in'  => $_SESSION['logged_in']);


if (isset($_POST['group_action']) && ($_SESSION['logged_in'] === 1) && isset($_SESSION['user_id'])){
    capture();
} else{
    echo die(json_encode($group_array));
} 

function capture() {
    $group_array['group_action'] = (int)$_POST['group_action']; 
    direct($group_array['group_action']);
}

//directs action depending on registration state
function direct($i) {
    switch ($i) {
        case 0:
            $new_group = new create_group;
            $new_group->create_new_group();
            break;
        case 1:
            $search = new search_group;
            $search->initial_search_group();
            break;
        case 2:
            $join = new join_group;
            $join->start_join_group($join->join_array);
            break;
        case 3:
            $search = new search_group;
            $search->my_search_group();
            break;
        case 4:
            $join = new join_group;
            $join->start_unjoin_group($join->join_array);
            break;
        case 5:
            $search = new search_group;
            $search->show_one();
            break;              
        default: 
            echo die(json_encode($group_array));
    }
}

class create_group {

    public $create_group_array = array();

    public function __construct() {
        $this->create_group_array['group_name']      = $this->filter($_POST['group_name']);
        $this->create_group_array['description']     = $this->filter($_POST['description']);
        $this->create_group_array['location_name']   = $this->filter($_POST['location']);
        $this->create_group_array['privacy']         = (int)$this->filter($_POST['privacy']);
        $this->create_group_array['lat']             = $this->filter($_POST['lat']);
        $this->create_group_array['lng']             = $this->filter($_POST['lng']);
        $this->create_group_array['state']           = $this->filter($_POST['state']);
        $this->create_group_array['user_id']         = (int)$this->filter($_SESSION['user_id']);
        $this->create_group_array['created_by_name'] = $this->filter($_SESSION['username']);
        if($_POST['password']!==""){        
            $this->create_group_array['password']        = hash('sha256', $this->filter($_POST['password'].SALT));
        }
    }

    public function create_new_group() {    
        //need to add validation with regex
        $this->create_group_array['action'] = 0;
        $create = new create_group_model($this->create_group_array);
        $data = $create->insert_new_group($create->input);

        ///after group is created, join it
        if($data['success']===1) {
            $join = new join_group;
            $join->start_join_group(array(
                'user_id' => $this->create_group_array['user_id'], 
                'group_id'=> (int)$data['results'][0],
                'privacy' => $this->create_group_array['privacy'],  
                'password'=> @$this->create_group_array['password'],
            ));
        }
        echo die(json_encode($data));
    }

    public function filter($var) {
        $var = trim($var);  
        return $var;
    }

}

class search_group{
    public $search_group_array = array();

    public function __construct() {
        //$this->search_group_array['keyword']  = $this->filter($_POST['keyword']);
        @$this->search_group_array['lat']       = $this->filter($_POST['lat']);
        @$this->search_group_array['lng']       = $this->filter($_POST['lng']);     
        $this->search_group_array['user_id']    = (int)$this->filter($_SESSION['user_id']);
        @$this->search_group_array['group_id']  = (int)$this->filter($_POST['group_id']);
    }

    public function filter($var) {
        $var = trim($var);  
        return $var;
    }
    //show local groups that i am not a part of
    public function initial_search_group() {
        $this->search_group_array['action']    = 1;
        $search_c = new search_group_model($this->search_group_array);
        $data  = $search_c->default_list_groups();
        echo json_encode($data);
    }
    ///show my groups
    public function my_search_group() {
        $this->search_group_array['action']    = 3;
        $search_c = new search_group_model($this->search_group_array);
        $data = $search_c->my_list_groups($search_c->input['user_id']);
        echo json_encode($data);
    }
    //show one group profile
    public function show_one() {
        $this->search_group_array['action']    = 5;
        $this->search_group_array['privacy']   = (int)$this->filter($_POST['privacy']);
        $show = new search_group_model($this->search_group_array);
        $data = $show->show_one($show->input['user_id'],$show->input['group_id'],(int)$show->input['privacy']);
        echo json_encode($data);
    }


}

class join_group {
    public $join_array = array();

    public function __construct() {
        $this->join_array['user_id']     = (int)$this->filter($_SESSION['user_id' ]);
        @$this->join_array['group_id']   = (int)$this->filter($_POST['group_id']);
        $this->join_array['action']      = (int)$_POST['group_action'];
        @$this->join_array['privacy']    = (int)$this->filter($_POST['privacy']);
        if((int)$this->join_array['privacy'] !==0 ){
            $this->join_array['password']   = hash('sha256', $this->filter($_POST['password'].SALT));
        }
    }

    public function filter($var) {
        $var = trim($var);  
        return $var;
    }
    //join group, only send info to client if they requested it with corresponding action 2/4
    public function start_join_group($group) {
        $join_c = new join_group_model($group);
        $data = $join_c->join_group();
        if(@$group['action']===2) {
            echo json_encode($data);
        }
    }

    public function start_unjoin_group($group) {
        $join_c = new join_group_model($group);
        $data = $join_c->unjoin_group();
        if(@$group['action']===4){
            echo json_encode($data);
        }
    }
}

?>

Model

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

Application wide database methods. 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 {
            echo 'fuckin fail';
            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;

Answer 1

Try to avoid the include/require_once functions. These are much slower than their counterparts, for obvious reasons. This may not be as noticeable when only importing a few files, but it is still good practice not to use them as you should always know if you have already included a file.

This code looks like it is generating errors that are probably preventing it from even working. If you are starting sessions, then you should almost always have the session_start() call as the first function to avoid headers being sent to the page prematurely. The only exception I can think of, off the top of my head, is when you have persistent classes and need to include those class files before calling the session to maintain persistence.

An "array" suffix is typically unnecessary. I'm not going to say to remove it, just pointing it out. This is purely a stylistic choice.

Before using a session variable it is usually advisable to ensure that variable has been set by using the isset() function on it. Otherwise you will get NULL pointers and perhaps silent warnings about the array index being out of bounds or something.

I believe you are recreating a boolean with your $_SESSION[ 'logged_in' ] variable. It stands to reason that there are only two possible states for this variable, TRUE or FALSE. I'm assuming you are using 1 and 0 or -1. Either way, the TRUE/FALSE pair are better and do not require any sort of type conversion, meaning you can do something like this.

if( $_SESSION[ 'logged_in' ] ) {
    //user logged in
}
if( ! $_SESSION[ 'logged_in' ] ) {//more appropriately "else" in this context
    //user not logged in
}

How are you accessing the $group_array outside of its scope? This smells like a global, which are EXTREMELY BAD. If you need to access a variable inside a function, then it should be passed in as a parameter and then returned to be added on to it. Or you could use referencing, but I tend to stay away from those for legibility reasons. Of course there are other methods, such as using session variables, cookies, post and get. The situation will determine which method you use. Just remember, globals are bad. From this point forward, pretend you have never heard of the word. In no context could I ever see globals being necessary or good. They are old and full of security issues.

You should also try to avoid die() as it is a very inelegant way of escaping a sequence. Typically this is seen a lot when an error occurs and the proper way would be to log that error then redirect the user to an error page that informs the user that an error has occurred. In this context perhaps it is ok as it is "creating" a JSON document, but in other cases this may not be the case. Also, echo is unnecessary, die() automagically prints the output of whatever parameters it receives.

If your PHP version is >= 5.2 you can use filter_input() on your POST data to filter and sanitize it.

$action = filter_input( INPUT_POST, 'group_action', FILTER_SANITIZE_NUMBER_INT );

Your direct() function is a little confusing. The biggest problem I have here is your switch statement. They do not always need to be passed integers, you can pass it strings as well, and I believe this would be much easier to follow if it did use strings. Another issue I am having is all of this repetition. This is somewhat related to SOLID, what Peter Kiss mentioned in his answer. "Don't Repeat Yourself" (DRY) Principle. As the name implies, you code should not repeat, but be reused for maximum efficiency. The SOLID Principles are similar and very useful to remember when learning OOP. Here, and later, you are violating the DRY Principle. You create the same objects in a couple of these cases, only the methods change. There's got to be some way around this. With a numerical switch it would be easier, but you would lose legibility; With a string value you would need to do some fancy work with arrays or REGEX. Additionally, when creating a new object, you should do so using the proper syntax, new object();, note the parenthesis. Below is how you could rewrite this with the numerical switch, but I still think it would be better to use a string.

if( $i == 1 ) {
    $obj = new create_group();
} else if( $i > 1 ) {
    if( $i < 3 ) {
        $obj = new search_group();
    } else {
        $obj = new join_group();
    }
}

The way you are currently assigning your associative indices to your arrays is very tedious and inefficient. You use the proper way later in your code, but not consistently. Its only when you are redefining indices or adding to a preexisting array that you need to define each element separately. Unless you had a lot of indices you wished to add or redefine, then it would be better to create a new array and merge the two after you are done.

$this->create_group_array = array(
    'group_name' => $this->filter( $_POST[ 'group_name' ] ),
    'description'   => $this->filter( $_POST[ 'description' ] ),
    //etc...
);
//OR for preexisting arrays
$temp = array(
    'group_name' => 'new group name',
    //more changes or additions
);
$this->create_group_array = array_merge(
    $this->create_group_array,
    $temp
);

DON'T use error suppression! This is a sign of bad code. Maybe in production code where you are debugging this would be fine, but in live code, these should be removed.

I'm going to stop here. There's a lot of code, and a lot of the same issues. I skimmed some of the rest, but nothing else really caught my eye. The biggest thing I see is that there is no need for these classes, at least not as they are. They are just glorified data repositories. You could more easily accomplish this with normal functions, not to mention the amount of processing you would save due to the code reuse this would open up. For instance, you have a filter() method in each of the classes I looked at. How many other "shared" resources would there be? Classes are good, but only if they follow proper OOP principles, otherwise they just add overhead and hinder development.

In the future, I would suggest splitting up large posts, there is just entirely too much here. If you had posted just a few (no more than four) classes, then not as many people would be intimidated by this post and would be more likely to help.

Answer 2

If you want to apply the MVC pattern then you should learn the main OO principles (SOLID is a great word for this). The actual state of your code smells. Global variables half OO and half procedural style and you are creating GOD objects whish is really bad.