Is this right structure for MVC / MVP application?

Question

I have written a code for an application/website that I wanted to be structured similarly to MVC design pattern (but I think it's rather MVP based on comparison I read). This functions as a kind of intranet where users can log in, see some news, short info about them - all on one page (some of the content is static and some dynamic based on the user).

I structured my code in the following way:

• presentation pages (index.php, main.php, admin.php, error.php)
• all input is processed by JavaScript (jQuery) by $.post function which calls my router.php script passing name of a class & method to call and additional params to be passed to that method; this script processes the input and uses Controller.php class to handle that request
• Controller class handles instantiating of the right class and calling its method and returning the results to presentation pages by using callbacks on $.post
• Controller calls specific methods in classes representing user, news etc.

Please mind the naming of my routher.php and Controller class - it's how I called them but this might not be correct from the MVC/MVP point of view - if so please point that out.

Questions that I have in my mind:

• Is this right structure?
• Should I combine code in router.php & Controller.php into one entity?
• How should JavaScript code be stored? At the moment I'm using one .js file for handling login included in index file and one for the rest of the presentation pages.
• I'm storing all info (User object, news etc.) in $_SESSION variable.
• Can I store connection details in global variable (I read a lot on Stack Overflow that this is discouraged/bad practice)?

Please comment, advise and share your opinion - all constructive criticism is welcome.

router.php:

<?php

require_once 'config.php';
require_once 'Controller.php';

ini_set("log_errors", 1);
ini_set("error_log", LOG_PATH . "Controller.log");
ini_set('display_errors', 0);
error_reporting(E_ALL);

startIntranetSession();

try
{
    $appController = new Controller();

    if (isset($_POST['params']))
    {
        if (sizeof($_POST['params']) > 1)
        {
            $method = filter_input(INPUT_POST, 'method', FILTER_SANITIZE_STRING);
            $class = filter_input(INPUT_POST, 'class', FILTER_SANITIZE_STRING);
            $params = filter_input(INPUT_POST, 'params', FILTER_DEFAULT, FILTER_REQUIRE_ARRAY);
            $methodToCall = $class . "::" . $method;
            echo $appController->callMethod($methodToCall, $params);
        }
        elseif (sizeof($_POST['params']) == 1)
        {
            $method = filter_input(INPUT_POST, 'method', FILTER_SANITIZE_STRING);
            $class = filter_input(INPUT_POST, 'class', FILTER_SANITIZE_STRING);
            $params = filter_input(INPUT_POST, 'params', FILTER_DEFAULT);
            $methodToCall = $class . "::" . $method;
            echo $appController->callMethod($methodToCall, $params);
        }
    }
    else
    {
        $method = filter_input(INPUT_POST, 'method', FILTER_SANITIZE_STRING);
        $class = filter_input(INPUT_POST, 'class', FILTER_SANITIZE_STRING);
        $methodToCall = $class . "::" . $method;
        echo $appController->callMethod($methodToCall);
    }

}
catch (Exception $e)
{
    echo "error";
    $file = LOG_PATH . 'router.log';
    $date = date("d-m-Y H:i:s", time());
    $message = $date . " - " . $e->getMessage() . " \r\n" . $e->getTraceAsString();
    file_put_contents($file, $message, FILE_APPEND);
}

Controller.php:

<?php

require_once 'config.php';
require_once 'News.php';
require_once 'User.php';
require_once 'KPI.php';

ini_set("log_errors", 1);
ini_set("error_log", LOG_PATH . "Controller.log");
ini_set('display_errors', 0);
error_reporting(E_ALL);

startIntranetSession();

class Controller
{
    public function __construct()
    {
        $GLOBALS['connection'] = new mysqli(SQL_HOST, SQL_USER, SQL_PASS, SQL_DB, SQL_PORT);
    }

    public function callMethod($classNameAndMethod, $parameters = null)
    {
        $callMethodArray = explode("::", $classNameAndMethod);

        if (sizeof($callMethodArray) != 2)
        {
            throw new Exception("Wrong parameter in callMethod()");
        }

        $className = $callMethodArray[0];
        $methodName = $callMethodArray[1];

        if ($className == 'User')
        {
            if (isset($_SESSION['current_user']))
            {
                $classObject = unserialize($_SESSION['current_user']);
            }
            else
            {
                $classObject = new $className();
            }
        }
        else
        {
            $classObject = new $className();
        }

        if (!method_exists($classObject, $methodName))
        {
            throw new Exception("Given method does not exist!");
        }

        if (!isset($parameters))
        {
            $parameters = array();
        }

        if (!$result = call_user_func_array(array($classObject, $methodName), $parameters))
        {
            throw new Exception("Call_user_func_array failed!");
        }
        else
        {
            return $result;
        }
    }

}

Answer 1

For now, here is what I have noticed. I'll come back when I get some time to take a better look.

I'm seeing some repetition in that initial try statement too. It looks as though you can take

$method = filter_input(INPUT_POST, 'method', FILTER_SANITIZE_STRING);
$class = filter_input(INPUT_POST, 'class', FILTER_SANITIZE_STRING);
$methodToCall = $class . "::" . $method;

and place it outside of the if statements.

Also, to better organize your errors, and keep a better record of error recording, I'd consider you create your own exception classes that extend the built-in Exception object, or use the SPL exceptions.

For instance, you could, in Controller.php, throw a InvalidParameterCallException, MethodExistanceException, and a FunctionCallException. In your first file, you can use multiple catch statements to properly handle anything that's thrown. Like:

try {
    $appController = new Controller();
    ...
} catch (InvalidParameterCallException $error) {
    ...
} catch (MethodExistanceException $error) {
    ...
} catch (FunctionCallException $error) {
    ...
}

However, the best option may be to implement the SPL BadFunctionCallException or the BadMethodCallException class. I suggest you take a look at these classes, I think you could benefit from them. Here is a good article on PHP 5.3 exception usage.

This way you can supply the user (or the error logs) with a more specific error.

Is this right structure?

First off, here is a very good, short and simple write-up of MVC vs. MVP.

I would say that router.php and Controller.php are the Presenter layer. However, in router.php, for me, it's hard to classify this explicitly. It's taking in user input and deciding how to react, but you end up using echo? Is a user physically visiting this page or not? If it's meant to be a Presenter (or Controller) file, then I think any visiting to it requires a 404 or a redirect.

Should I combine code in router.php & Controller.php into one entity?

No. They both handle very different things. router.php is taking in input and making a convienent way to use it. Controller.php is delegating the workload that your routing file used it for.

I'm unsure of why you have the class and method passed as one argument, but then split it up right after? It would seem to make more sense if you allowed for:

function callMethod($className, $methodName, $parameters = null)

instead. That way you don't need to explode the sting. Now you can call:

$appController->callMethod($class, $method, $params);

I would rewrite your callMethod() function to something a bit like:

public function callMethod($className, $methodName, $parameters = null)
{

    if (count($callMethodArray) !== 2) {
        throw new Exception("Wrong parameter in callMethod()");
    }

    if ($className == 'User' && isset($_SESSION['current_user'])) {
        $classObject = unserialize($_SESSION['current_user']);
    } else {
        $classObject = new $className();
    }

    if (is_null($parameters)) {
        $parameters = array();
    }

    if (!method_exists($classObject, $methodName)) {
        throw new Exception("Given method does not exist!");
    }

    if ($result = call_user_func_array(array($classObject, $methodName), $parameters)) {
        return $result;
    } else {
        throw new Exception("Call_user_func_array failed!");
    }
}

This clears up your code a bit, notice the minimization of the nested ifs, the use of is_null, and my switching of the negation if at the end.

To further break this down, you could create a ClassValidator class to validate classes, functions, and arguments.

storing all info (User object, news etc.) in $_SESSION variable.

User objects (Username, First name, User Id, etc.) are something that works well in the session global. When you say news though, I'm getting a little weary. How much news? If it's the top 5 or so article synopses, I think you'll be okay. If it's full length articles for 10+ different stories, you may run into issues down the line.

Typically, the $_SESSION variable is used to store basic information about the user for quick and easy access.

Can I store connection details in global variable (I read a lot on Stack Overflow that this is discouraged/bad practice)?

I agree that this is a bad practice. Remember, you define it as:

$GLOBALS['connection'] = new mysqli(SQL_HOST, SQL_USER, SQL_PASS, SQL_DB, SQL_PORT);

The $GLOBAL variable is one that should be avoided. If you code your stuff correctly, there should hardly ever be a need for it. At the very least, you could create a new private $_connection = null; to hold your database.

But since this is a controlling class, there should be no need to even declare a mysqli object! You should have a class (the model) that handles all of the database action for you.