laravel / framework

The Laravel Framework.
https://laravel.com
MIT License
32.29k stars 10.94k forks source link

[Proposal] Adding "view creators" to complement view composers #1822

Closed phpmycoder closed 11 years ago

phpmycoder commented 11 years ago

The docs for View Composers originally misled me to think that listeners would receive the view before View::make() returns it:

View composers are callbacks or class methods that are called when a view is created. If you have data that you want bound to a given view each time that view is created throughout your application, a view composer can organize that code into a single location.

But, I soon found out that the composing: * event is fired just before the view is composed (which in hindsight makes sense). However, this seems to me like a perfect opportunity to add a new hook into the View lifecycle: creators. Consider a master layout, which requires a nested header, sidebar, and main content view. My previous solution was to add a new method to the BaseController:

// Invoked from Basecontroller::setupLayout() (after view creation)
protected function prepareLayout() {
    $this->layout->nest('header', array('links' => array(...)))
                 ->nest('sidebar', array('widgets' => array(...)));
    $this->layout->with('breadcrumbs', new ArrayObject());
}

However, this seems like the perfect opportunity for a view creator so that the main layout isn't so tightly coupled to the BaseController. I propose a syntax similar to View::composer that registers a closure, class, or class method as a listener for when a view is created:

View::creator('master', function($view) {
    $view->nest('header', array('links' => array(...)))
         ->nest('sidebar', array('widgets' => array(...)));
    $view->with('breadcrumbs', new ArrayObject());
});

This way whenever View::make('master') is called, the view object already has the necessary nestings (and initialized arrays) so that any class can merge their own properties without needing to worry about instantiation details.

Implementation would be fairly simple. A creating: {view} event could be dispatched in Illuminate\View\Environment::make() and subsequent methods could be added for the View::creator method.

Classes which registered as listeners could implement a create() method (similar to the compose() method). And it would be a nicety if classes could implement both compose() and create() and then register for both

Perhaps a parent class for ViewComposers could help here to define empty bodies for both the create() and compose() methods so that a class could register for one or both by calling either View::composer() or View::creator() and the listener could add them to both events.

taylorotwell commented 11 years ago

I'm curious why you personally aren't using Blade layouts to accomplish this sort of thing? Just to help me get a better idea of how people are using View composers it would be helpful for me to know as I personally haven't had a big use for them since Blade got template inheritance capabilities.

phpmycoder commented 11 years ago

In this instance, the reason why I'm not using Blade layouts is because I can't access the subviews from the creator of the parent view. A good (not contrived) example of this is a project that I'm currently working on, which has a slideshow view:

<div id="slideshow">
    @foreach($images as $image)
        {{ HTML::image($image->url, array('alt' => $image->alt)) }}
    @endforeach
</div>

If this view was @included from a view called master, I couldn't append data to it from the controller that created master. Now arguably, I could just add $images to the parent view since the child would inherit the parent's data, but I'd rather not pollute the parent view's data.
The view "creator" event also allows for another opportunity in this instance: instantiating the $images array.

View::creator('slideshow', function($view) {
    $view->slideshow = array();
});

Otherwise, you'd have to instantiate it each time you create the view (and in the case of Controller layouts, it can become difficult to remember if it's been instantiated already). I've also encountered this need when working with a $breadcrumbs array, which is appended to in multiple places.

Kindari commented 11 years ago

not arguing against a creator event, but you can already do this using the existing composers.

View::composer('master', function($view) {
    $view->nest('header', array('links' => array(...)))
         ->nest('sidebar', array('widgets' => array(...)));
    $view->with('breadcrumbs', new ArrayObject());
});

I use code like this all the time - The only thing changed from your code example was replacing creator with composer. The composing event fires before the view is rendered, you nest the elements, pass the variables, then the view is rendered, which sees the nested elements and renders them (recursively, I might add) and so on. This is how it also worked in L3.

phpmycoder commented 11 years ago

@Kindari Yes, you can bind things to the view pre-render. However, the trouble is you can't bind to it post-create (right after the view is created). So in my example, I need the breadcrumbs array to be initialized so that I can append to it after creating the view. The following will produce an error:

View::composer('master', function($view) { $view->breadcrumbs = array(); });

$master = View::make('master');
$master->breadcrumbs[] = 'Home'; // Error: breadcrumbs has not been defined yet
$master->render(); // now breadcrumbs is defined right before render, but useless to us
ronaldcastillo commented 11 years ago

I tried the following (in my BaseController class):

    protected function setupLayout()
    {
        if ( ! is_null($this->layout))
        {
            $this->layout = View::make($this->layout);//->with('breadcumbs', $this->breadcumbs);
            $this->layout->breadcumbs[] = 'Home';
        }
    }

And it seemed to work (using Laravel 4.1.x). No error was shown. Am I missing something or is it fixed in the latest version?

phpmycoder commented 11 years ago

@ronaldcastillo You are correct. For some reason I was under the impression that appending to an uninitialized array would throw an E_NOTICE like accessing an undefined variable does. It seems like even with ArrayAccess (or magic __get()/__set()) PHP is able to predict that you want the property to be an array and initialize it first.

However, I would still assert that the best practice would be to initialize the array first before using it as you would any other variable.

khandieyea commented 11 years ago

@phpmycoder, I think that's more a case of PHP Versions / Error levels / Reporting no?

I get

Fatal error: [] operator not supported for strings.

When appending to an uninitialized array ie $x[] = 'Home';

However, manually giving the item a key works:

$x['key'] = 'home';

I tend to agree with your last sentence - initialization is best.

phpmycoder commented 11 years ago

@khandieyea Possibly (see this codepad). Are you running Laravel 4.0.*? In which case, your problem could relate to Issue #1829, which has been merged to master but you need 4.1.*.

jasonlewis commented 11 years ago

There's also View::share, although I'm not sure at which point that data is bound to each view.

JoostK commented 11 years ago

@jasonlewis View::make (Illuminate\View\Environment::make that is) passes the shared variables from the environment to the view instance, so at creation time of a view. It is not updated when new shared variables are registered, I don't think.

Kindari commented 11 years ago

Shared data is included at rendering.

https://github.com/laravel/framework/blob/master/src/Illuminate/View/View.php#L81 https://github.com/laravel/framework/blob/master/src/Illuminate/View/View.php#L100 https://github.com/laravel/framework/blob/master/src/Illuminate/View/View.php#L110

render() calls getContents() which calls gatherData() which merges the views data with the environments shared data.

JoostK commented 11 years ago

Ah, my mistake then, thanks for clearing things up. I went through the code yesterday and thought having seen what I said.

phpmycoder commented 11 years ago

So given the agreed upon need to initialize certain view data variables as well as make subviews whose data can be accessed from the parent view (via $parentView->subview->someData = 'abc';), I propose the following amendments to the View API (that is, behind the scenes the Illuminate\View\Environment class):

  1. Adding: public Closure creator ( mixed $views, mixed $callback )

    This method will register $callback (or in the instance of a class with no provided method, $callback::create()) as a listener for the creating view: {viewname} view event, which is fired from Illuminate\View\Environment::make() upon the instantiation of each view.

    Parameters: In a similar manner to View::composer(), this method would accept either a string view name or an array of string view names as its first parameter and either a Closure or string of a class name (which implements a public function create(View $view)) or reference to a public class method in the form ClassName@method, which accepts one parameter of type View.

  2. Adding: public Closure manager ( mixed $view, string $callback )

    This method will register $callback::create for the creating view: {viewname} event, which is fired on Illuminate\View\Environment::make(), and $callback::compose for the composing view: {viewname} event, which is fired on Illuminate\View\View::render().

    Parameters: The first parameter must be a string view name or an array of string view names. The second parameter must be a string of a class name (which implements both public function create(View $view) and public function compose(View $view)).

To store these declarations, the encouraged standard should be a managers.php, composers.php, or creators.php in the /app directory, which should be included from /app/start/global.php.

It also might be advantageous, to add a ViewManager class which can be extended by classes passed to View::manager() and provide empty bodies for the create() and compose() methods so classes can register as managers and easily add hooks to one or both events with minimal modification to managers.php.

Depending on the feedback to this proposal I can copy my own extension of \Illuminate\View\Environment into a pull request for review.

taylorotwell commented 11 years ago

Done.