Getting started with MVC

Having read the article on The MVC Pattern and PHP, I decided to try and write a bit of code using MVC. I’ll add my code at the bottom of the article, if anyone wants to look through it and tell me everything I’m doing wrong, that would be great!

The code is very simple so far, it is just a form with a checkbox and a textbox for a date. When submitted the controller pushes the submitted data to the model, which stores it in a property $data ($formData might actually be a better description). The model also validates the date input. Then the view just displays the page again, including any error messages.

Particular questions I have are:

  • If the data formatting is performed in the template, what purpose does the view have? Or should the view be doing the formatting, e.g. should it be one view calling different templates depending on the wanted data format (HTML, JSON, XML etc.), or should it be a different view for each format?
  • According to the article
    > The View is really much more than just a template
    , but this is not really expanded on. The only info I could find on anything else the view does is that it might cache the output. What else can the view be used for?
  • I am a bit confused on whether the controller should call the view or not. The article indicates it shouldn’t, but if the controller handles user interactions, then isn’t one of those user interactions requesting the view (web page)?
  • In my code I am copying the data from the model to the view, and the view implements methods for the template to access the data. Is this correct, or should the data stay in the model?

Thanks

Dave

The page:

<?php
include('globalFuncs.inc.php');
$imageLicenseModel = new imageLicenseModel($conn);
$imageLicenseController = new imageLicenseController($imageLicenseModel);
if(isset($_POST['submit'])){
    $imageLicenseController->process($_POST);
}
$imageLicenseView = new imageLicenseView($imageLicenseModel);
$title="License an image";
$metaDescription = 'Purchase a license to use a photo on a website or in print';
$h1="<h1>$title</h1>";
include('pageTop.php');
$imageLicenseView->process('imageLicenseTemplateHTML.php');
include('pageBottom.php');

Controller:

class imageLicenseController{

    /**
     *Creates an instance of the imageLicenseController class
     *@param imageLicenseModel $imageLicenseModel The $imageLicenseModel to pull data from
     */
    public function __construct(&$imageLicenseModel){
        $this->_imageLicenseModel = &$imageLicenseModel;
    }

    /**
     *Passes the data to the model for processing
     *If data okay pushes the data to paypal for payment processing
     */
    public function process($data){
        //pass the data to the model for processing
        if($this->_imageLicenseModel->setData($data)){
            //if success post on to paypal
        }
    }
}

View:

<?php
class imageLicenseView{

    //The model
    protected $_imageLicenseModel;
    //The data (retrieved from the model)
    protected $_data;
    //The errors (retrieved from the model)
    protected $_errors;

    /**
     *constructs an instance of the imageLicenseView object
     *@param imageLicenseModel $imageLicenseModel The $imageLicenseModel to pull data from
     */
    public function __construct(&$imageLicenseModel){
        $this->_imageLicenseModel = &$imageLicenseModel;
    }

    /**
     *Returns the value of a named data property
     *@param string $property The name of the property to get the value of
     *@return mixed The value of the property
     */
    public function get($property){
        if(isset($this->_data[$property])){
            return $this->_data[$property];
        }
    }

    /**
     *Returns the value of a named error property
     *@param string $property The name of the property to get the value of
     *@return string The value of the property
     */
    public function getError($property){
        if(isset($this->_errors[$property])){
            return $this->_errors[$property];
        }
    }

    /**
     *Includes the specified template
     *@param string $template The template file to include
     */
    public function process($template){
        $this->_data = $this->_imageLicenseModel->getData();
        $this->_errors = $this->_imageLicenseModel->getErrors();
        include($template);
    }
}

Model:

<?php
class imageLicenseModel{

    //holds the db connection
    protected $_conn;
    //holds the data
    protected $_data=array();
    //holds any errors that occur
    protected $_errors=array();


    /**
     *constructs an instance of the imageLicenseModel object
     *@param mysqli $conn The database connection
     */
    public function __construct(&$conn){
        $this->_conn = &$conn;
    }


    /**
     *Gets the data
     *@return array The data
     */
    public function getData(){
        return $this->_data;
    }


    /**
     *Sets and validates the provided data
     *@param array $data The data to set
     *@return bool Whether the data was updated or not
     */
    public function setData($data){
        $this->_data = $data;
        //validate
        if($this->_validate($data)){
            //update the db
        }
        if(!empty($this->_errors)){
            return false;
        }
        return true;
    }


    /**
     *Validates the provided data
     *@param array $data The data to validate
     *@return bool Whether the data validated or not
     */
    protected function _validate($data){
        //date-start
        //It would be nice to validate the date by using the DateTime object's inbuilt validation, but if a date is invalid this will throw a Fatal exception rather than a lower exception level - http://stackoverflow.com/questions/11343403/php-exception-handling-on-datetime-object
        if(strlen($data['date-start']) != 10 ||
           (($dateSplit = explode('-', $data['date-start'])) && count($dateSplit) != 3) ||
           !is_numeric($dateSplit[0]) ||  !is_numeric($dateSplit[1]) || !is_numeric($dateSplit[2]) ||
           !checkdate($dateSplit[1], $dateSplit[2], $dateSplit[0]) ||
           date('Y-m-d', strtotime($data['date-start'])) != $data['date-start']){
            $this->_errors['date-start']='Date must be a valid date in format YYYY-MM-DD';
        }
        elseif(strtotime($data['date-start']) < mktime (0,0,0)){
            $this->_errors['date-start']='Start date cannot be in the past';
        }
        if(!empty($this->_errors)){
            return false;
        }
        return true;
    }

    /**
     *Gets the errors
     *@return array The errors
     */
    public function getErrors(){
        return $this->_errors;
    }
}

Template:

<?php
//If there were any errors, print them out above the form
if(!empty($this->_errors)){
    ?><div class="msgBox error"><p>The request could not be processed, the following errors occurred:</p>
    <ul>
        <?php foreach ($this->_errors as $error){
            echo "<li>$error</li>";
        }?>
    </ul></div><?php
}?>
<form action="" method="post">
<ul>
<li><label for="non-commercial">Non commercial use</label><input type="checkbox" name="non-commercial" value="1" <?php if($this->get('non-commercial')){echo 'checked="checked"';} ?> /><span class="info">Please note that usage on a page that contains adverts constitutes commercial use.</span></li>
<li class="<?php if($this->getError('date-start')){echo 'error';}?>"><label for="date-start">License start date (YYYY-MM-DD)</label><input type="date" id="date-start" name="date-start" min="<?php echo gmdate('Y-m-d'); ?>" value="<?php echo $this->get('date-start');?>" /></li>
</ul>
<div><input type="submit" value="submit" name="submit" /></div>
</form>

To answer your questions in order:

  1. Data formatting is done in the view, whether the view uses a template system or not isn’t really defined by MVC. However, if you’re using a template in a view you’ll certainly want to do the formatting in the template. This lets you reuse the view with other templates which might want a different date format.

  2. With a template system, the view would handle all interactions with the template and request data from the model. This keeps the responsibilities separated and means that nothing else in the system knows the template even exists. Ideally the view would have a very minimal external API, probably just a constructor that asks for a specific model and a render method. This means you could have multiple views that use the same templates. They would use different models and supply the template with data in a specified format. Or allow the same view to use different template. Where possible you want to keep logic in the view and out of the template.

  3. The controller doesn’t need to know of the view. There’s no reason it ever needs to interact with it. This offers flexibility by allowing any view to be used with any controller and potentially visa versa. The controller sets the model’s state, the view reads the data from the model. Because of this, the view doesn’t care which controller set the model’s state and the controller doesn’t care which (if any!) view will be inquiring about the state of the model.

In a desktop application the view would know about its controller to register callbacks (E.g. call this controller action when this button is pressed) but on the web you need a router and the view stores links. You’ll likely want these to come from the router.

  1. Data should stay in the model. Depending on how your template system works, in a lot of cases you should simply be able to assign the model to the template and reference it directly. In some cases, the view might restructure this data before assigning it to the template.

One of the biggest reasons that “the view is more than a template” is to offer reusability. By encapsulating a template within a view, the view logic can be reused. Common tasks such as displaying a form (and any related validation errors), pagination, listing a set of records can have their logic moved to specific views and anywhere that needs this logic can either reuse it directly or extend it. The point being it can be reused with any template.

As for your code in general, it’s certainly along the right lines. My only criticism is that the view is acting like a model by storing the data. You questioned this and rightly so. Make the view read straight from the model rather than copying the data.

I think most of your questions can be answered with this explanation:

[INDENT]The definitions of MVC were written with desktop applications in mind, and it doesn’t translate perfectly to stateless, server-side web applications. MVC controllers are supposed to handle user input – which could be mouse clicks, key presses, touches, gestures, device orientation, etc – and translate that input into commands for the view and the model. But in server-side web applications, there’s only one input: the HTTP request. And MVC views are supposed to be able to react to user input, as well as to changes in the model. But in server-side web applications, there won’t be any new user input for the view to react to, nor will there be any changes to the model that would require the view to update itself.

Because of these differences, MVC was a bit bastαrdized in its adaptation to the web. Views in web applications are little more than templates, and the templates don’t access the model directly. Instead they are fed data by the controller, which acts as a mediator between the model and the view. It turns out there’s another design pattern that more accurately reflects the way server-side web applications work, called Presentation–abstraction–control.[/INDENT]

With that explanation out of the way, I’ll add my two cents… that even though what most developers and frameworks have been using all this time isn’t MVC in its purest sense, I think the architecture that web applications have settled on is a good one. For example, I think it’s a good thing that the views/templates don’t access the model directly. It lets them stay small in scope, ignorant of – and independent of – the larger application.

You may find the article From Flat PHP to Symfony2 useful. It takes you step-by-step to refactor old school PHP to the kind of architecture that frameworks are using today.

I’ve officially split the original thread into two threads, primarily because the topic changed focus to MVC vs PAC (new thread) and I think that makes sense as a thread of its own. Plus I feel the OP was afraid to post back after the debate got started, so hopefully cleaning up his original thread will grant him the option of returning and asking any additional questions about his MVC/PAC implementation.

Thanks,
cpradio

Hi @djeyewater;

I was introduced to MVC programming via CodeIgniter about five years ago and liked their loose interpretation. They have some excellent documentation and this diagram was nicked from their website:

A couple of points I noticed in your script:

  1. Only data should be passed from the Controller and Model. (NO H1 tags).

  2. Views should apply all HTML formatting.

  3. The view has PHP logic which should be in the Controller.

  4. Maybe try setting all Controller variables with default values.

CodeIgniter has a neat System View Function that is used in Controllers, an array is populated then passed to the View function along with the view (or partial view) which is then rendered:

The Controller:



<?php
class Blog extends CI_Controller {

function index()
{
  $data = array();

  $data['title_page']  = "My Page Title";
  $data['title_main']  = "My Main Title";
  $data['todo_list']   = array('Clean House', 'Call Mom', 'Run Errands');

  $output = $this->load->view('blogview', $data, true);
  echo $output; // allows additional processing
}#

}##


The View:


<html>
<head>
  <title><?php echo $title_page;?></title>
</head>
<body>
  <h1><?php echo $title_main;?></h1>

  <h3>My Todo List</h3>

  <ul>
    <?php foreach ($todo_list as $item):?>
      <li><?php echo $item;?></li>
    <?php endforeach;?>
  </ul>

</body>
</html>

Without wanting to stray back into the debate that was split into the other thread, that’s clearly not MVC but PAC even if CodeIgniter want to call it MVC. The OP was asking questions about MVC in particular (and linked to a tutorial that was discussing a pure MVC implementation) I’m not sure suggesting a different architecture really helps answer the original question.

Point taken about the MVC and PAC discrepancy I just believed CodeIgniter when they stated they have a MVC framework.

As far as the OP’s questions were concerned and the referenced article, I was trying to supply a clear and simple solution also with references to four points which have previously given me grief.

Hi Tom

Thanks for your reply. Your example of the NestedView does give a good argument for views being actual objects. I think it is always easier to understand things with worked examples like that rather than plain theory.

Cheers

Dave

Thanks Tom, Jeff, and John for your advice so far.

I’ve modified my classes, and just wanted to check I’m going along the right lines.

The goal of my code is to allow someone to purchase a license to use an image e.g. to use on a website or in a magazine etc.

What I’ve done is created a base Model class called Model_db, which just implements a property _dbData that data pulled from the db can be stored in, and a method to allow accessing this data.

<?php
class Model_db{
    //holds the db connection
    protected $_conn;
    //holds the data pulled from the database
    protected $_dbData=array();


    /**
     *constructs an instance of the imageLicenseModel object
     *@param mysqli $conn The database connection
     */
    public function __construct(mysqli &$conn){
        $this->_conn = &$conn;
    }

    /**
     *Gets the data
     *@param string $property[optional] The name of the property to get the value of (will return all if not supplied)
     *@return mixed The value of the property
     */
    public function getData($property=null){
        if(!isset($property)){
            return $this->_dbData;
        }
        if(isset($this->_dbData[$property])){
            return $this->_dbData[$property];
        }
    }
}

I’ve then extended this class to create Model_db_actions, which adds some properties and methods to do with user supplied data. So it has a property to store the data supplied by the controller (typically POST), and properties and methods to store and retrieve any success or error messages based on the processing of the data supplied by the controller.

<?php
class Model_db_actions extends Model_db{
    //holds the data input by the user (or defaults)
    protected $_formData=array();
    //holds any errors that occur
    protected $_errors=array();
    //holds any successes that occur
    protected $_successes=array();


    /**
     *Gets the form data
     *@param string $property[optional] The name of the property to get the value of (will return all if not supplied)
     *@return mixed The value of the property
     */
    public function getFormData($property=null){
        if(!isset($property)){
            return $this->_formData;
        }
        if(isset($this->_formData[$property])){
            return $this->_formData[$property];
        }
    }


    /**
     *Gets the errors
     *@param string $property[optional] The name of the property to get the value of (will return all if not supplied)
     *@return mixed The error or all errors
     */
    public function getErrors($property=null){
        if(!isset($property)){
            return $this->_errors;
        }
        if(isset($this->_errors[$property])){
            return $this->_errors[$property];
        }
    }


    /**
     *Gets the successes
     *@param string $property[optional] The name of the property to get the value of (will return all if not supplied)
     *@return mixed The success or all successes
     */
    public function getSuccesses($property=null){
        if(!isset($property)){
            return $this->_successes;
        }
        if(isset($this->_successes[$property])){
            return $this->_successes[$property];
        }
    }
}

Then I have my imageLicenseModel, which extends Model_db_actions, and adds various properties and methods specifc to an image license. I’ve included logic in here to do with working out the price, I presume the Model is the correct place for this?

When the model receives data from the controller, it validates the data, works out a price, updates the database (not written yet), and then sets a success flag that the data was validated and added okay.

<?php
class imageLicenseModel extends Model_db_actions{

    protected static $_noncommercial = array(
        0 => array('name' => 'Commercial', 'multiplier' => 1),
        1 => array('name' => 'Non-Commercial', 'multiplier' => 0.5)
    );
    //number of months license is for and corresponding multiplier to use when working out the pricing
    protected static $_timeOpts = array(
        1 => array('name' => 'Up to 1 month', 'multiplier' => 1),
        3 => array('name' => 'Up to 3 months', 'multiplier' => 1.5),
        6 => array('name' => 'Up to 6 months', 'multiplier' => 2),
        12 => array('name' => 'Up to 1 year', 'multiplier' => 2.5),
        36 => array('name' => 'Up to 3 years', 'multiplier' => 3),
        0 => array('name' => 'Indefinitely', 'multiplier' => 4)
    );
    //The image this license is for (an instance of image_basicModel)
    public $img;

    /**
     *Sets and validates the provided data
     *@param array $formData The formData to set
     *@return bool Whether the data was updated or not
     */
    public function setFormData(array $formData){
        $this->_formData = $formData;
        //validate
        if($this->_validate($formData)){
            $this->_dbData['cost'] = $this->calculateCost();
            //update the db
        }
        if(!empty($this->_errors)){
            return false;
        }
        $this->_successes['validated'] = true;
        return true;
    }

    /**
     *Calculate the cost of the license
     *@return int|float The cost
     */
    public function calculateCost(){
        $cost = 10;
        return $cost*(self::$_noncommercial[$this->_formData['non-commercial']]['multiplier'])*
                (self::$_timeOpts[$this->_formData['time']]['multiplier']);
    }

    /**
     *returns whether the data submitted was valid
     */
    public function getValidated(){
        return $this->_dataValidated;
    }

    /**
     *Sets the image that this license is for
     *@param image_basicModel $img The instance of image_basicModel the license should be tied to
     */
    public function setImg($img){
        $this->img = &$img;
    }

    /**
     *Validates the provided data
     *@param array $data The data to validate
     *@return bool Whether the data validated or not
     */
    protected function _validate(array $formData){
        //date-start
        //It would be nice to validate the date by using the DateTime object's inbuilt validation, but if a date is invalid this will throw a Fatal exception rather than a lower exception level - http://stackoverflow.com/questions/11343403/php-exception-handling-on-datetime-object
        if(strlen($formData['date-start']) != 10 ||
           (($dateSplit = explode('-', $formData['date-start'])) && count($dateSplit) != 3) ||
           !is_numeric($dateSplit[0]) ||  !is_numeric($dateSplit[1]) || !is_numeric($dateSplit[2]) ||
           !checkdate($dateSplit[1], $dateSplit[2], $dateSplit[0]) ||
           date('Y-m-d', strtotime($formData['date-start'])) != $formData['date-start']){
            $this->_errors['date-start']='Date must be a valid date in format YYYY-MM-DD';
        }
        if(!empty($this->_errors)){
            return false;
        }
        return true;
    }


    /**
     *Gets the different time periods that a license can be purchased for
     *@return array The time options
     */
    public function getTimeOpts(){
        return self::$_timeOpts;
    }

}

The imageLicenseModel needs to know which image the license is for, so I’ve created a class image_basicModel that gets basic info from the db for a given image id, and can be passed into the imageLicenseModel

<?php
/**
 *Gets the basic info from the db for a single image
 */
class image_basicModel extends Model_db{

    public function __construct(&$conn, $img_id){
        parent::__construct($conn);
        if(!is_numeric($img_id)){
            throw new Exception('Invalid image id');
        }
        if($result = $conn->query('SELECT * FROM images WHERE id='.$img_id)){
            $this->_dbData = $result->fetch_assoc();
            $result->close();
        }
        else{
            throw new Exception('Invalid image id');
        }
    }
}

The controller doesn’t do anything other than pass any user supplied data onto the model.

<?php
class imageLicenseController{

    //The model the controller is attached to
    protected $_model;

    /**
     *Creates an instance of the imageLicenseController class
     *@param imageLicenseModel $imageLicenseModel The $imageLicenseModel to pull data from
     */
    public function __construct(&$imageLicenseModel){
        $this->_model = &$imageLicenseModel;
    }

    /**
     *Passes the data to the model for processing
     *If data okay pushes the data to paypal for payment processing
     */
    public function process($formData){
        //pass the data to the model for processing
        $this->_model->setFormData($formData);
    }
}

The View contains a couple of helper functions for formatting an array of error or success messages as an HTML list. Other than that, it is pretty generic, so I have just called it View_actions

<?php
class View_actions{

    //The model
    protected $_model;

    /**
     *constructs an instance of the View_actions class
     *@param Model_actions $imageLicenseModel The model to pull data from
     */
    public function __construct(Model_db_actions &$model){
        $this->_model = &$model;
    }

    /**
     *Formats a list of errors as HTML
     *@param string $title The title to use in the error box e.g. 'Sorry, the following errors occured'
     *@return string The errors formatted as HTML
     */
    protected function _listErrors($title){
        if(!($errors = $this->_model->getErrors())){
            return;
        }
        return '<div class="msgBox error"><p>'.$title.'</p>'.$this->_listRecursive($errors).'</div>';
    }

    /**
     *Formats an array as an HTML unordered list
     *@param array $list array of values to printed as a list
     *@return string The array values formatted as an HTML unordered list
     */
    protected function _listRecursive($list){
        $str = '<ul>';
        foreach ($list as $item){
            $str .= '<li>'.(is_array($item) ? $this->_listRecursive($item) : $item).'</li>';
        }
        return $str.'</ul>';
    }

    /**
     *Formats a list of success messages as HTML
     *If there is only one message, will print in a paragraph rather than a list
     *@param string $title[optional] The title to use in the success box e.g. 'The following records were updated'
     *@return string The messages formatted as HTML
     */
    protected function _listSuccesses($title=null){
        if(!($successes = $this->_model->getSuccesses())){
            return;
        }
        return '<div class="msgBox success"><p>'.$title.'</p>'.$this->listRecursive($successes).'</div>';
    }

    /**
     *Includes the specified template
     *@param string $template The template file to include
     */
    public function process($template){
        $model = &$this->_model;
        include($template);
    }
}

For the template I have split it into two files, one that contains the form (imageLicenseTemplateHTML.php), and another that contains the confirmation message (imageLicenseTemplateHTML_confirm.php). This second template is included from the first one if the form was submitted and validated okay.

imageLicenseTemplateHTML.php:

<?php
//If there were any errors, print them out above the form
echo $this->_listErrors('The request could not be processed, the following errors occurred:');
if($model->getSuccesses('validated')){
    include('imageLicenseTemplateHTML_confirm.php');
}?>
<form action="" method="post">
<ul>
<li><label for="non-commercial">Non commercial use</label><input type="radio" name="non-commercial" id="non-commercial" value="1" <?php if($model->getFormData('non-commercial')){echo 'checked="checked"';} ?> />
    <label for="commercial">Commercial use</label><input type="radio" name="non-commercial" id="commercial" value="0" <?php if(!$model->getFormData('non-commercial')){echo 'checked="checked"';} ?> />
    <span class="info">Please note that usage on a page that contains adverts constitutes commercial use.</span></li>
<li class="<?php if($model->getErrors('date-start')){echo 'error';}?>"><label for="date-start">License start date (YYYY-MM-DD)</label><input type="date" id="date-start" name="date-start" min="<?php echo gmdate('Y-m-d'); ?>" value="<?php echo $model->getFormData('date-start');?>" /></li>
<li><label for="time">Time period</label>
    <select name="time" id="time">
        <?php
        $options = $model->getTimeOpts();
        foreach($options as $val => &$option){
            $option = '<option value="'.$val.'">'.$option['name'].'</option>';
        }
        $selectedOption = $model->getFormData('time');
        if(isset($selectedOption)){
            $options[$selectedOption] = '<option selected="selected" '.substr($options[$selectedOption],8);
        }
        echo implode("\
", $options);?>
    </select></li>
</ul>
<input type="hidden" name="id" value="<?php echo $model->getFormData('id');?>" />
<div><input type="submit" value="submit" name="submit" /></div>
</form>

imageLicenseTemplateHTML_confirm.php:

<div id="confirm">
    <p>Please check the following details are correct, then click the buy now button to make payment through paypal.</p>
    <ul>
        <li><?php if($model->getFormData('non-commercial')){echo 'Non-';} ?>Commercial use</li>
        <li><?php //if(strtotime($model->getFormData('date-start')) < mktime (0,0,0)){
        $dateStart = new DateTime($model->getFormData('date-start'));
        $now = new DateTime('today');
        if($dateStart < $now){
            echo 'Retrospective ';
        }?>License starting from <?php echo $dateStart->format('jS F Y');
        if($model->getFormData('time') != 0){
            echo ' and running for '.$model->getFormData('time').' months until '.$dateStart->add(new DateInterval('P'.$model->getFormData('time').'M'))->format('jS F Y');
        }else{
            ' and running indefinitely';
        }
        ?></li>
    </ul>
    <p>License cost: <span class="license-cost"><?php setlocale(LC_MONETARY, 'en_GB.UTF-8'); echo money_format('%n',$model->getData('cost'));?></span></p>
    <form action="https://www.paypal.com/cgi-bin/webscr" method="post">
        <input type="hidden" name="cmd" value="_xclick" />
        <input type="hidden" name="business" value="email@domain.com" />
        <input type="hidden" name="lc" value="GB" />
        <input type="hidden" name="item_name" value="Image license ref.<?php echo $model->getFormData('id');?> for image <?php echo $model->img->getData('Headline');?> (image id <?php echo $model->img->getData('id');?>)" />
        <input type="hidden" name="item_number" value="<?php echo $model->img->getData('id');?>" />
        <input type="hidden" name="amount" value="<?php echo $model->getData('cost');?>" />
        <input type="hidden" name="currency_code" value="GBP" />
        <input type="hidden" name="button_subtype" value="services" />
        <input type="hidden" name="no_note" value="0" />
        <input type="hidden" name="bn" value="PP-BuyNowBF:button.png:NonHostedGuest" />
        <input type="image" src="https://www.paypalobjects.com/en_GB/i/btn/btn_buynow_LG.gif" name="submit" alt="PayPal — The safer, easier way to pay online." />
        <img alt="" src="https://www.paypalobjects.com/en_GB/i/scr/pixel.gif" width="1" height="1" />
    </form>
</div>

If instead of having one template including the other, I wanted to display a different page depending on success or not, would it be suggested to modify the view to accept a standard template and a success template? e.g.

View->setSuccess('imageLicenseTemplateHTML_confirm.php');
View->setStandard('imageLicenseTemplateHTML.php');
View-show();

Finally there is the page, I’ve stripped out the bits about setting the title and h1 tag as they’re not really relevant. I’m just trying to work on getting the imageLicensing aspect of the page working under MVC principles, rather than the whole page. (Thanks for the example though, it may come handy in future).

include('globalFuncs.inc.php');
if(empty($_REQUEST['img_id']) || !is_numeric($_REQUEST['img_id'])){
    throw new Exception('Invalid image id');
}
$image_basicModel = new image_basicModel($conn,$_REQUEST['img_id']);
$imageLicenseModel = new imageLicenseModel($conn);
$imageLicenseModel->setImg($image_basicModel);
if(isset($_POST['submit'])){
    $imageLicenseController = new imageLicenseController($imageLicenseModel);
    $imageLicenseController->process($_POST);
}
$imageLicenseView = new View_actions($imageLicenseModel);
$imageLicenseView->process('imageLicenseTemplateHTML.php');

If anyone can spot anything where I’m obviously going wrong, or something could be done in a better way, please let me know.

Thanks

Dave

You’ve clearly understood why a view is not a template which is great! Your helper functions are generic enough to be useful in any template that might have errors and your view can be used with any template! Very good! It’s good to see someone who has a good grasp of separation of concerns. I have some very minor criticisms, but nothing major. Firstly, I’d probably make the error message HTML templates themselves, it gives you more flexibility and keeps HTML out of your PHP code which is a lot cleaner.



<?php
class View_actions{ 

    //The model 
    protected $_model; 
    protedted $_errorTemplate;
    /** 
     *constructs an instance of the View_actions class 
     *@param Model_actions $imageLicenseModel The model to pull data from 
     */ 
    public function __construct(Model_db_actions &$model, $errorTemplate = 'defaultErrorTemplate.php'){ 
        $this->_model = &$model; 
	$this->_errorTemplate = $errorTemplate;
    } 
     
    /** 
     *Formats a list of errors as HTML 
     *@param string $title The title to use in the error box e.g. 'Sorry, the following errors occured' 
     *@return string The errors formatted as HTML 
     */ 
    protected function _listErrors($title){ 
        if(!($errors = $this->_model->getErrors())){ 
            return; 
        } 

	$errorList = $this->_listRecursive($errors);
	ob_start();
	include $this->_errorTemplate;
	return ob_get_contents();
    } 
    //.....

For convenience I’d make it so a default template is set for each of your helper functions as it gives you a nice convention-over-configuration approach where you only have to supply the name of a template if it’s not the default one.

The second change I’d make, and this is very minor, but it’s better to use private wherever possible instead of protected. Giving things the least amount of visibility possible means you can be more confident of exactly where they’re being used. You don’t have to worry about breaking the behaviour of another class by drastically changing the implementation. If it’s a private method and you need to completely rewrite it, or change the output (e.g. from a string to an array) you know the only place that can possibly be broken by this change is the class you’re currently looking at.

As you suggest, your controller isn’t doing much here because there’s not many user actions. This is a good thing but it does look rather sparse. Just to add some completeness to this, on forms there aren’t many user actions that involve the server but a couple of the common ones are: Populating a second drop down after selecting from a first, clicking “preview” to create a preview of their submission a preview above the form. These would obviously update the model’s state accordingly and the view would then decide how to deal with this. Finally, if it was a normal form which was submitted and didn’t take you to paypal, you’d do the redirect to the “Order Complete”/“Thank you” page in the controller based on whether the model reported a success or failure.

Finally, I’d make the imageLicenseModel protected variables normal variables rather than static. This just gives you better flexibility. There’s no reason these need to be static and in the future you might want an instance of the model which had different values. For example you might have a special customer type who gets different costs. By making these values static you lock yourself into a specific set of values that’s shared across every instance. By making them normal variables it’s possible to initialise two instances of the classes with different sets of data. You may never need it, but it gives you the flexibility down the line if you do… and at no expense!

Overall though, I’d say this is a very strong starting point. As the project grows you’ll probably want a router to manage your initialisation code. My blog post here may be useful for that.

Just wanted to say thanks for all the advice, greatly appreciated!

Dave