1
\$\begingroup\$

I am not sure if I am correctly handling $_POST data in MVC pattern:

Dummy $_POST data

$_POST = array(
    "menu" => array(
        "key_name" => '',
        "pub_name" => ''
    ) 
);

View

$connection = null;
$entry = new menuPostController($connection);
echo $entry->renderXML();

I will get this result when there the input field is empty:

<xml>
<response>
<error elementid="key_name" message="Key name - You must not leave this field empty!" />
<error elementid="pub_name" message="Public name - You must not leave this field empty!" />
</response>
</xml>

Then if all fields are filled in, I will get this result:

<xml>
<response>
<success message="well done!" path="test.php?id=2" action="add" />
</response>
</xml>

Controller

class menuPostController {

    private $connection = null;
    public $data = array();

    public function __construct($connection) {
        $this->connection = $connection;
    }

    public function getData(){

      if(isset($_POST['menu'])){

            $menu = new menuSanitizor($this->connection);
            $this->data = $menu->getData($_POST['menu']);
            return $this->data;
        }  
    }

    public function renderXML(){

        $data = self::getData();

        if(isset($data)) {
            $xml = new xmlHelper();
            echo $xml->getXML($data);
        }
    } 
}

Model

class menuSanitizor 
{
    private $connection = null;
    public $error = array();
    public $key_name = null;
    public $pub_name = null;
    public $post = null;

    public function __construct($connection) {
        $this->post = new post();
        $this->connection = $connection;
    }

    public function getData($params = array()) {

        $this->key_name = self::setKeyName($params['key_name']);
        $this->pub_name = self::setPubName($params['pub_name']);

        if($this->error) {

           return array("error" => array_merge(
                is_array($this->key_name)? $this->key_name : array(),
                is_array($this->pub_name)? $this->pub_name : array()
            ));

        } else {

            // inject into the database
            $inject = self::inject();

            if($inject === true) {
                return array("success" => array(
                    "message" => "well done!",
                    "path" => "test.php?id=2",
                    "action" => "add"
                ));
            }
        }
    }

    public function inject(){

        /*
        $sql = "
            INSERT INTO menu (
        key_name,
        pub_name,
        created_on
            )VALUES(    
        ?,
        ?,
        NOW()
    )";

    $result = $this->connection->executeSQL($sql,array(
        $this->key_name,
        $this->pub_name
    ));

        return $result;
         * 
         * 
         */

        return true;
    }

        // only the setter sanitizes the data.
    public function setKeyName($key_name = null) {

        // Get the value first.
        $key_name = $this->post->get('key_name', $key_name);
        //print_r($key_name);

        if($key_name === null) {
            $this->error = array("key_name" => "Key name - You must not leave this field empty!");
            return $this->error;
        } else {
            // Set the value.
            $this->key_name = $key_name;
            return $this->key_name;
        }
    }

    public function setPubName($pub_name = null) {

        // Get the value first.
        $key_name = $this->post->get('pub_name', $pub_name);


        if($key_name === null) {
            $this->error = array("pub_name" => "Public name - You must not leave this field empty!");
            return $this->error;
        } else {
            // Set the value.
            $this->pub_name = $pub_name;
            return $this->pub_name;
        }
    }
}

Global getter helper

class post 
{
    public $data = array();

    // check a series of parameters that may or may not be set, and sanitize them one by one..
    public function get($param, $default = null) {

        // silently convert empty strings to NULLs
        if ($default === '') {
            $default = null;
        }

        if (!isset($this->data[$param])) {//not set, return default
            return $default;
        } 

        return $this->data[$param];
    }
}

Global XML helper

class xmlHelper
{
    public $xml = null;

    public function getXML($data = array()) {

        $this->xml = new SimpleXMLElement('<xml/>');
        $response = $this->xml->addChild('response');

        if(isset($data['error']) && count($data['error']) > 0){

            foreach($data['error'] as $key => $value) {
                $error = $response->addChild('error');
                $error->addAttribute('elementid', $key);
                $error->addAttribute('message', $value);
            }
        }

        else if(isset($data['success'])){

            $success = $response->addChild('success');
            foreach($data['success'] as $key => $value) {
                $success->addAttribute($key, $value);
            }
        }

        Header('Content-type: text/xml');
        return $this->xml->asXML();
    }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Well, don't. Honestly, the best way to go about this is to create a Service layer. The controller can interact with this service layer, and it's this service layer that creates the DB connections, and decides what methods are to be called. Read about this approach here, to see a simple graph click here, that should clarify what I'm on about \$\endgroup\$ – Elias Van Ootegem Sep 16 '13 at 13:48
  • 1
    \$\begingroup\$ Also check this. As you can see, the DB is not part of the controller's responsability. That's dealt with in the Model LAYER. I can't stress this enough the Model part of MVC isn't an object, it's a LAYER. It can (and IMHO should) consist of various object, each with their own, distinct tasks. No need to try and cram it all into a single object... \$\endgroup\$ – Elias Van Ootegem Sep 16 '13 at 13:51
  • 1
    \$\begingroup\$ Singletons have no point in PHP... but that's another story. You just need a config object, and pass that to a DB class constructor. That class can then use the config to connect to the DB. The config object can be passed to the service in the controller, hell: the config can even be global. But connections to DB's is something that is not done in a controller. A controller is something that should contain as little code as possible! (google dependency injection and singletons are evil, to learn why you shouldn't use them) \$\endgroup\$ – Elias Van Ootegem Sep 16 '13 at 14:48
  • 1
    \$\begingroup\$ On the config: A config file (ini, xml, yml... doesn't matter) should be parsed at the very beginning. This config can contain all sorts of information (like where the autoloader is, what routing plugins you're using and the DB connection info). This can be passed around freely, but you only connect to the DB once you know for a fact that you're going to query for data. It's how most MVC frameworks work: Zend, Symfony... they all do this. Just check their source code for examples... the config is the first thing they load... \$\endgroup\$ – Elias Van Ootegem Sep 17 '13 at 6:30
  • 1
    \$\begingroup\$ The Respository class, in the given example, then contains the logic. The DB connection can be handled by another abstraction layer (for example Doctrine). Creating the Repository instance could look like this: $inst = new Repository($configObject); and its constructor: __construct(Config $conf){ $this->connection = new DBWrapper($config->databaseConfig);}... that's what I mean by passing around the config, but keeping actual DB work to the model layer \$\endgroup\$ – Elias Van Ootegem Sep 17 '13 at 8:30
3
\$\begingroup\$

Since I've explained a lot in the comments, I thought I might distill those comments into an answer, so here goes:

You're definitely improving, considering your previous question. Well, to be honest: I must say the difference between your first code example, and the one you posted here now is night and day (in a good way)! The main problem with your code, at this point is that you're still connecting to the DB in the controller. Don't. DB work/logic resides in the model Layer.
The best way to structure more complex applications (within the confines of the MVC pattern) is by implementing a service layer. Read about it here. Use this graph to guide you along:

Schematic Service

Your DB-related objects are still having to sanitize the request data, which they really shouldn't. Processing the request should've been done way before they even come into play. The last point where you can still work on the request is the controller, as shown in this graph:

MVC with responsibilities

It's important to note that the DB is not part of the controller's responsibility. That's dealt with in the Model LAYER. I can't stress this enough the Model part of MVC isn't an object, it's a LAYER. It can (and IMHO should) consist of various object, each with their own, distinct tasks. No need to try and cram it all into a single object...
There is, however, one object that can be passed to all other objects in the MVC pattern (save for data models), and that is the object representing the app's configuration. You pass that object around freely: to a DB class constructor, via the service's constructor, or through setters. Each class can then use the Config object to extract whatever data it needs from the instance for it to do its job. Hell: the config can even be global. But connections to DB's is something that is not done in a controller. A controller is something that should contain as little code as possible.

On the config:
This can be any old file (ini, xml, yml... doesn't matter), but should be parsed at the very beginning. This config can contain all sorts of information (like where the autoloader is, what routing plugins you're using and the DB connection info). This can be passed around freely, but you only connect to the DB once you know for a fact that you're going to query for data. It's how most MVC frameworks work: Zend, Symfony... they all do this. Just check their source code for examples...

You refer to this answer, because it'd seem to create a DB connection in the controller. Well, in effect, it creates a service instance in the controller, and it's that service's job to connect to the DB, and query it, based on the methods that are called in the controller. These methods then return Data models, back to the controller, where they're passed on to the view. That's standard, by-the-book, MVC. Though the snippet doesn't make mention of any config object, it's quite easy to implement it:

//controller:
public function someAction(Config $conf)
{
    $service = new Repository($conf);
    $model = $service->getDataForUser(
        $this->request->get('User', 'guest')//get User param, or guest if User param not found
    );
    if ($model === null)
    {
        return $this->redirect('user_invalid_page');
    }
    $this->view->user = $model;
}
//Repository class (=== service)
class Repository
{
    private $config = null;
    private $conn = null;
    public function __construct(Config $conf)
    {
        $this->config = $conf;
    }
    public function getDataForUser($userName)
    {//LOGIC HERE!
        $db = $this->getDb();//Lazy-loader!
        $query = $db->buildQuery(
            array(
                'table'  => 'users',
                'where'  => array(
                    'usrName'  => $userName
                )
            )
        );
        $data = $db->execute($query);
        if (!$data)
        {
            return null;
        }
        $model = new UserModel;
        foreach($data as $field => $val)
        {//check my other answers on why I use setters here...
            $model->{'set'.$field}($val);
        }
        return $model;
    }
    private function getDb()
    {
        if ($this->conn === null)
        {
            $db = new DBWrapper($this->config->database);//pass DB connection config to class
            $this->setDb($db);
        }
        return $this->conn;
    }
    public function setDb(DBWrapper $db)
    {//injection => public
        if ($this->conn instanceof DBWrapper)
        {//close current connection
            $this->conn->close();
        }
        $this->conn = $db;
        return $this;
    }
}

This is a (somewhat) crude example of how to implement MVC can be implemented correctly. The DB connection is handled by a DB object. The instance of DBWrapper is created by the service, which also creates the data-models, based on the return values from the DB class. The controller calls upon the service to attempt to retrieve the desired data. What the service needs to do to get to that data is non of the controller's business. Nor does the controller need to know/care what type of DB is being used. That's up to the DBWrapper class.
Please note how little code the controller actually contains, and how much of the "heavy lifting" is done in the service and the classes this service actually creates. While writing code that goes into the controller, make sure you're not writing your application logic, save it for the service.

The service only cares if the requested data was found, and how it should be poured into objects. If no data was returned, it'll return null to the controller. If data was found, the service will try its very best to return a nice model to the controller. In short: to each class its own tasks/concerns. If you need to add logic, you open and edit the service class, if you need to add a user action, add an action method in the controller, if you want to change/add support for another type of DB, you turn to the DBWrapper class. Easy!

Please note, DBWrapper isn't a terrible good name, it suggests it's just a wrapper class for PDO or Mysqli, and I'm not a fan of such classes

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.