Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

As a learning exercise, I'm developing my own PHP framework. I'm looking for a way to "load views" (kinda like CodeIgniter does it), without polluting my general scope.

I came up with the following, basic, example:

$data['test'] = 'Hello world';

$str = load_view($data, true);
echo $str;

function load_view($data, $store = false) {
    extract($data);

    ob_start();
    include('view.php');

    if($store)  return ob_get_clean();
    else        ob_end_flush();
}

// End of file

view.php
<p><?php echo $test; ?></p>

I could also use load_view($data), which would output the contents of view.php immediately.

Edit: I'm mostly worried about performance. As Peter pointed out, I'm aware that the function should be a class method that's seperate from the logic.

share|improve this question
add comment

4 Answers 4

I agree that its a better pattern to wrap your view logic in a class.

Here is some code I whipped together tonight - It represents the simplest class I could devise that contains the minimum logic that I require in a view:

  • Includes - Ability to include other views

  • Captures - Ability to easily capture content within your view

  • Layouts - Ability to inject data into a re-usable layout template

  • Fetching - Ability to fetch view output instead of sending it to output buffer

  • data - Ability to access the resulting data once the view is finished

[edit] Refactored per conversation in comments regarding passing $data by reference.

View.php

<?php
/**
 * Simple view class that supports includes, capturing, and layouts, as well
 * as retrieving rendered view content and resulting data.
 *
 * *NOTE* When a view uses a layout, the output of the view is ignored, as
 *        as the view is expected to use capture() to send data to the layout.
 *
 * @author David Farrell <DavidPFarrell@gmail.com>
 */
class View implements ArrayAccess
{
    /**
     * View file to include
     * @var string
     */
    private $file;

    /**
     * View data
     * @var array
     */
    private $data;

    /**
     * Layout to include (optional)
     * @var string
     */
    private $layout;

    /**
     * Constructor
     *
     * @param string $file file to include
     */
    public function __construct($file)
    {
        $this->file = $file;
    }

    /**
     * render Renders the view using the given data
     *
     * @param array $data
     * @return void
     */
    public function render($data)
    {
        $this->data = $data;
        $this->layout = null;

        ob_start();

        include ($this->file);

        // If we did not set a layout
        if (null === $this->layout)
        {
            // flush view output
            ob_end_flush();
        }
        // We set a layout
        else
        {
            // Ignore view output
            ob_end_clean();

            // Include the layout
            $this->include_file($this->layout);
        }
    }

    /**
     * fetch Fetches the view result intead of sending it to the output buffer
     *
     * @param array $data
     * @return string The rendered view content
     */
    public function fetch($data)
    {
        ob_start();
        $this->render($data);
        return ob_get_clean();
    }

    /**
     * get_data Returns the view data
     *
     * @return array
     */
    public function get_data()
    {
        return $this->data;
    }

    /**
     * include_file Used by view to include sub-views
     *
     * @param string $file
     * @return void
     */
    protected function include_file($file)
    {
        $v = new View($file);
        $v->render($this->data);
        $this->data = $v->get_data();
    }

    /**
     * set_layout Used by view to indicate the use of a layout.
     *
     * If a layout is selected, the normal output of the view wil be
     * discarded.  The only way to send data to the layout is via
     * capture()
     *
     * @param string $file
     * @return void
     */
    protected function set_layout($file)
    {
        $this->layout = $file;
    }

    /**
     * capture Used by view to capture output.
     *
     * When a view is using a layout (via set_layout()), the only way to pass
     * data to the layout is via capture(), but the view can use capture()
     * to capture text any time, for any reason, even if the view is not using
     * a layout
     *
     * @return void
     */
    protected function capture()
    {
        ob_start();
    }

    /**
     * end_capture Used by view to signal end of a capture().
     *
     * The content of the capture is stored under $name
     *
     * @param string $name
     * @return void
     */
    protected function end_capture($name)
    {
        $this->data[$name] = ob_get_clean();
    }

    /* ArrayAccess methods */
    public function offsetExists($offset)      { return isset($this->data[$offset]); }
    public function offsetGet($offset)         { return $this->data[$offset]; }
    public function offsetSet($offset, $value) { $this->data[$offset] = $value; }
    public function offsetUnset($offset)       { unset($this->data[$offset]); }

}

run.php

<?php
require "View.php";

$v = new View('view_main_simple.php');
$fetch = $v->fetch(array('message' => 'Hello, world'));
print("Fetch result: {$fetch}\n");

$v = new View('view_main_complex.php');
$v->render(array('one' => 1, 'two' => 2, 'rows' => array('a','b','c')));

$data = $v->get_data();

print("\n");
var_export($data);

view_main_simple.php

The message is: <?php echo $this['message'] ?><br/>

view_main_complex.php

<?php $this->set_layout('view_layout.php') ?>
<?php $this->capture() ?>
    one=<?php echo $this['one'] ?><br/>
    <?php $this->include_file('view_include.php') ?>
    three=<?php echo $this['three'] ?><br/>
<?php $this->end_capture('body') ?>

view_include.php

two=<?php echo $this['two'] ?><br/>
<?php $this['three'] = 3 ?>
<ul>
    <?php foreach($this['rows'] as $row) { ?>
        <li><?php echo $row ?></li>
    <?php } ?>
</ul>

view_layout.php

<html>
<body>
<pre>
<?php echo $this['body'] ?>
</pre>
</body>
</html>

Program Output

Fetch result: The message is: Hello, world<br/>

<html>
<body>
<pre>
    one=1<br/>
    two=2<br/>
<ul>
            <li>a</li>
            <li>b</li>
            <li>c</li>
    </ul>
    three=3<br/>
</pre>
</body>
</html>

array (
  'one' => 1,
  'two' => 2,
  'rows' => 
  array (
    0 => 'a',
    1 => 'b',
    2 => 'c',
  ),
  'three' => 3,
  'body' => '   one=1<br/>
    two=2<br/>
<ul>
            <li>a</li>
            <li>b</li>
            <li>c</li>
    </ul>
    three=3<br/>
',
)
share|improve this answer
    
Do not use passing by reference: schlueters.de/blog/archives/125-Do-not-use-PHP-references.html ## Ha-ha my answer is similar to yours but i have -1 point. :) –  Peter Kiss Dec 11 '12 at 18:57
    
Greetings @PeterKiss! You'll notice that the public functions render() and fetch() do NOT accept references, meaning that they act on a copy of the passed in $data - I then use references internally so that modifications to MY $data can be maintained across includes and layouts - I then offer that (possibly) modified $data back to the caller via getData(). So I never alter the user's personal $data, but do (carefully) manage references within my own scope for the benefit of the view (and the user, if they choose) - So I'll take my +1 back now please :) –  David Farrell Dec 11 '12 at 19:27
    
There is no need to do that hack. If you assign $data to $this->data in render then the do_render doesn't need any parameter. And why is there render and do_render? do_render is private you still calling it as public: (new View($file))->do_render($this->data); –  Peter Kiss Dec 11 '12 at 19:55
    
@PeterKiss - Your statement isn't fully correct as it doesn't address my need to allow includes to modify $data - But You are right in that I don't need to pass $data around by reference - I already had all the tools I needed, namely the getData() function (now renamed to get_data() for consistency. I have refactored the class to no longer pass $data by reference, merged do_render() into render(), and now reassign my local $data based on the result of the included view. –  David Farrell Dec 11 '12 at 20:27
add comment

One thing to note is that in MVC proper, views get their own data from the model. The view should be passed a model and call methods on it to access enterprise data. This makes the common extract($data); method redundant.

A simplistic view API may be:

$view = new View($model); echo $view->render();

If you enter templates into the equation then you could use $view = new View($model, $template); but the important thing to note is that the View does not get fed data by the controller in MVC. And although many web "MVC" frameworks take this approach, this is technically not MVC but PAC.

I don't have enough rep to post images but see the image of MVC on the wikipedia article. You'll see the controller never interacts with the view.

For more information on the MVC architecture see: http://st-www.cs.illinois.edu/users/smarch/st-docs/mvc.html , and http://www.itu.dk/courses/VOP/E2005/VOP2005E/8_mvc_krasner_and_pope.pdf

share|improve this answer
add comment

I don't see any huge problem with what you have - specifically the use of extract(). I doubt you'll notice any difference in either memory use or execution time. The caveats you should be aware of are:

  1. What happens if your $data array contains a key "store"? It will overwrite your $store argument.

  2. Although it would be unusual for view files to initialize variables, it's possible they could. In such cases, you'll have variable name collisions.

A big advantage I see in passing the rendered view back as a string is that you can use views inside other views or layouts. Second, if you wish to test your controllers, you can assert things about the returned view. You might consider simply removing the second argument and always return a value - would make one less variable (to get collided with) and would leave you with one return type.

share|improve this answer
    
+1. as a simplistic method the code is fine - there's no value in the store argument though - just use $foo = load_view($bar); or echo load_view($bar) as appropriate. it's also exactly how CakePHP does it. Should point at the warnings for extract though - it can be dangerous if used on untrusted data without using e.g. EXTR_SKIP. –  AD7six Dec 11 '12 at 9:33
add comment

No this is an ugly way to do it.

extract() (PHP4 stuff in 2012/2013?)

The problem with exract() is that it's creating variables ionto the global hyperspace (oh God, why) where can exist (in localy also!) any other variable and can have name collisions and can overwrite the old values. Beside this you are losing the control over your code and it will be hard to maintain (debug, fix also) and to extend.

load_view()

Not always returns a value: in PHP this is okay but in general programming it's a bad habit. I do one thing in a case and in another one i do a complete different stuff in same function? And why am i in a function? How about a non-static class method where i know everything in my small environment?

Here is a small example of an idea:

View class

<?php
class View {

    private $_file;
    public $Data;

    public function __construct($viewFile, array $data) {
        $this->_file = $viewFile();
        $this->Data = $data;
    }

    public function Render()
    {
        require $this->_file;
    }

}

Usage

In the view file:

<ul>
<?php
foreach ($this->Data["rows"] as $object) {
    echo "<li>";
    echo $object->Name;
    echo "</li>";}
?>
</ul>

The output buffer handling is never part of the view it self. There have to be some kind of ViewEngine which can handle this thing. One thing one responsibility.

The problem with the example is that is's missing a whole lat infrastucture! In my MVC expiriment i have Controller, ControllerBuilder, ActionInvoker, abstract ActionResult, ViewResult a lot of other stuff what is necessary to get thing done. In my default IView implementation i have a Model "property" beside the ViewData, i can map a lot helper like Html or Url and any other and just then comes the others: RenderPartial(), RenderSection(), Section(), SectionStart(), SectionEnd(), Layout(); these are pointing to my current IViewEngine implementation which is handling all these stuff.

share|improve this answer
    
Honestly, I don't see how your example is better. Yes, you have put it in a class, which I would do as well, but didn't for the sake of keeping things simple. Your method also doesn't provide a way to return the output of the view to a string, it merely echoes it when calling the Render method. –  Bram Dec 10 '12 at 15:24
    
Also, I feel like it's very unclean to use stuff like $this->Data["rows"] inside a view file. $this has no logical context there in my opinion, it would make much more sense to use $rows (that's why I used the extract function). Unless that's a huge memory hog/is bad for performance. –  Bram Dec 10 '12 at 15:45
    
Made an edit to my answear to explain my thoughts. –  Peter Kiss Dec 10 '12 at 17:54
1  
@PeterKiss extract only imports into the scope it's used. In this case, he's using it inside a function so it is not importing the variables into the global scope. –  Rob Apodaca Dec 11 '12 at 0:40
2  
-1 . what control? How is the use of extract php4? Note that extract has a number of flags. I think taking a simple method and then saying the problem is it doesn't look like your MVC class structure isn't helpful. forcing $this->Data['x'] instead of $x only means more verbose view files, it doesn't offer benefits over the use of extract where extract is used in not-global-scope. –  AD7six Dec 11 '12 at 9:26
show 2 more comments

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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