2
\$\begingroup\$

I have been writing PHP programs in the MVC pattern for quite some time. But I am not sure if I am doing it right.

For instance, I have this PHP file that prints results in XML, and I put this file in 'model':

header("Content-Type: text/xml");
echo '<?xml version="1.0" encoding="utf-8" standalone="yes" ?>
<response>
';

# Set object property/ variable.
$post = $connection->arrayToObject($_POST,$property_overloading = true);
//print_r($post);

# Assuming no error.
$error = false;

# Get the required field and put them in an array.
$array_required = is_array($post->required)? $post->required : explode(' ',$post->required);

# Get title
if (!$post->title)
{
    if(in_array('title', $array_required))
    {
        $error = true;
        echo '<error elementid="title" message="TITLE - Please entitle this album." />';
    }
}

# Get description
if ($post->description)
{
    if(in_array('description', $array_required))
    {
        $error = true;
        echo '<error elementid="description" message="DESCRIPTION - Please enter something." />';
    }
}
else
{
    # Ceck if there is a line break, if the match is found, replace it with an empty space.
    if (preg_match("/\n/", $post->description))
    {
        $post->description = preg_replace("/\n/is", " ", $post->description);
    }
}

# Get hide
if(!$post->hide) $post->hide = 0;
    else $post->hide = 1;

# Print general error message.  
if ($error) echo '<errorgeneral message="Please amend the error above." />';


# check if no more errors   
if (!$error) 
{
    $sql = "
    UPDATE album 
    SET 
        title = ?, 
        description = ?,
        hide = ?
    WHERE album_id = ?
    ";

    # use the instantiated db connection object from the initiator.php, to process the query.
    $result = $connection->executeSQL($sql,array(
        $post->title,
        $post->description,
        $post->hide,
        $post->album_id
    ));

    # Must convert all special characters to XML.
    # http://en.wikipedia.org/wiki/List_of_XML_and_HTML_character_entity_references
    echo '<result message="Updated." itemid="'.$post->album_id.'" itemtitle="'.htmlspecialchars($post->title).'" itemdescription="'.htmlspecialchars($post->description).'" itemcategory="album"/>';
}

echo '</response>';

So, which part of it should be in the controller, and which should be in the model, helper, or view?

\$\endgroup\$
4
\$\begingroup\$

Ok, this might come across as blunt and hurtful, but it's meant to be helpful (code-review IMO should be harsh, here's why):

"I have been writing PHP programs in the MVC pattern for quite some time"
No, you haven't. Really. M-V-C stands for, as you well know: Model-View-Controller. The code you pasted just strings all three together in one single script. That's spaghetti-code (at best), not framework-like MVC code. Not even close.

"I have this PHP file that prints a result in XML, and I put this file in 'model'"
Again: any file that generates output belongs in the view. As the name suggests, anything that is sent to the client (ie the client gets to "see"), is part of the clients view of your application. You really should read up on MVC, to find out what belongs where. (there's a link at bottom of my answer)


At the top, you set your header, then you process the request (without checking if there has been a POST request). Setting the headers as soon as possible (before anything has been sent to the client) is understandable. But in the context of the MVC pattern, it shouldn't really matter. In fact, it's one of the great things about the real MVC pattern: inside your controller and model layer, you're absolutely sure the headers haven't been sent yet, because the view is only generated (and sent to the client) at the very end of the request.
So, as your app sets about processing the request, and you hit a point where the client should be redirected, you can do so, almost blindly. Safe in the knowledge that the headers aren't sent yet. That's why MVC is so successful, and why redirects are mostly found in the controller. To clarify, look at this very simple graph (from cakePHP site, but it applies to MVC in general):

Simple MVC chart
(source: cakephp.org)

Here you see the flow of an MVC webapp:

  1. The client interacts with the view, and a request is sent to the server.
  2. The dispatcher receives teh request and does some initial processing. Based on this request, the dispatcher deceides which controller needs to be constructed, and which method needs to be called to set things in motion
  3. The controller can do some more checks, to make sure the request is valid. If this is not the case the Model layer is left alone, and the client is redirected. Back over to the dispatcher, (2). This step should be uncommon (as it only occurs when something isn't quite right.
    Normally, the inital processing on the request data consists mainly of pouring the data into objects, and deciding what the Model layer needs to do with them.
  4. The model layer receives data from the controller and takes over. Here's where you'll find the actual logic. DB connections, computation, ... it's all contained here (which is why this is also referred to as the business layer). The data provided by the controller can be broken down more (checked/processed), used in queries etc... The result is returned to the controller. Note that the model layer does not communicate with the view in any way!
  5. The controller checks what the Model-layer returned. This can either be data, needed to build a new view, or an object/string saying the data turned out to be invalid/insufficient or nothing at all. Whatever it is, the controller can decide to redirect (back to step 1), call the model again (back to 3) or moving on: build the view, passing the data it received from the Model-layer.
  6. Finally, we can start building the view. The controller passes the data it received from the model layer and the View takes over. It isn't untill we've reached this step that the headers are set. During steps 1 through 4, the headers are nowhere near being set, but at this point, it's too late to go back.
    That's why the view is a simple script, containing markup and simple PHP (some loops, some if's and the occasional switch). A view contains no logic at all!.
  7. Finally, the resulting page (view) is sent to the client. Job done.

I hope this makes a few things clear, but now
Moving on:

$connection->arrayToObject($_POST,$property_overloading = true);

$property_overloading = true? why not pass true? now you have a global variable that is just sitting there. $connection, as I gather from the query at the bottom of your code, is a DB class. What is it doing processing your POST parameters? That's not it's job. That's a gross violation of the Single Responsability Principle. Don't. Just Don't!
SRP states that an object/class should have one and only one task (reason to change): processing POST params and connecting to the DB are 2 distinct tasks.

Following that, you check a series of parameters that may or may not be set, and sanitize them one by one. I mean: really! you're using an object, clearly that object should take care of sanitizing the data, and, using getters, you could just do something like this

$post->get('hide', false);
//with getter in Post class:
public function get($param, $defaultValue = null)
{
    if (!isset($this->data[$param]))
    {//not set, return default
        return $defaultValue;
    }
    return $this->data[$param];
}

The same applies to setters, only the setter sanitizes the data.
Though at this point, you have bigger fish to fry: don't do things like

if (preg_match("/\n/", $post->description))
{
    $post->description = preg_replace("/\n/is", " ", $post->description);
}

In the if experession, you're using a regular expression, only to then use another regex to remove the unwanted chars. Either do:

public function setDescription($desc)
{//in object, of course
    $this->description = preg_replace('/\n/',' ',$desc);
    return $this;
}
//or:
$this->description = implode(' ', explode(PHP_EOL, $desc));

If the string doesn't contain any new-lines, it's not going to be changed! Why bother checking?

Next, you're echo-ing all over the place, to which I can only say: don't. as Dave Jarvis says use an API like simplexml.

Stringing a query at the end and executing it, depending on what $error hold (is it a global??) is just terrible By the time you're generating output, you should have all your data ready set, good to go. Building the response/view should be a formality. That's why a view should not contain any logic. All data has been fetched and structured accordingly.


To answer your question (on which part should go where):

processing the request is usually done in the Routing objects, and the processed request is then used to create/and pass to the (front)controller. The controller passes the bits and pieces of the request to the model-LAYER (services, helpers, data-models), which is where all the logic is kept. The controller then receives the results from the model-layer, and passes it on to the view, which (through a series of loops and if's) generates the output.

If any of this is still a bit merky, refer to the MVC wiki or ask for more details.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Fair and balanced, +1 \$\endgroup\$ – konijn Sep 11 '13 at 12:44
  • \$\begingroup\$ @tomdemuyt: I hope I was more fair and balanced than some of the things I've seen on Fox, though :-P, but thanks \$\endgroup\$ – Elias Van Ootegem Sep 11 '13 at 12:53
  • \$\begingroup\$ Thanks Elias for the answer! I have made another post/ question to implement MVC to my existing code, would you like to take a look? codereview.stackexchange.com/questions/31301/… \$\endgroup\$ – laukok Sep 15 '13 at 4:48

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.