zendframework / zend-expressive-zendviewrenderer

zend-view PhpRenderer integration for Expressive
BSD 3-Clause "New" or "Revised" License
11 stars 12 forks source link

Add a public renderModel() method #2

Closed RalfEggert closed 7 years ago

RalfEggert commented 8 years ago

Currently, I can render my template like this:

$data = ['foo' => 'bar'];
$template->render('application::home-page', $data);

I really like the ViewModel stuff, so how about using it?

$viewModel = new ViewModel(['foo' => 'bar']);
$viewModel->setTemplate('application::home-page');

$template->renderModel($viewModel);

I know there is a private renderModel() method but I would like to have a public one to use ViewModels.

mtymek commented 8 years ago

I was thinking about exactly the same feature. However, I was in doubt if I should make a PR, as it means further deviating from TemplateRendererInterface's contract, making it more difficult to switch between templating engines.

Thinking more about it, I realized that:

Seeing this, and that someone else wants this feature, I created a PR: #3. If it is accepted, I will update docs on main expressive repo.

Usage (in action class):

$model = new ViewModel();
$model->setTemplate('app:index');
return new HtmlResponse($this->renderer->renderViewModel($model));

With different layout:

$model = new ViewModel();
$model->setTemplate('app:index');
$this->renderer->setLayout('app:alternative-layout');
return new HtmlResponse($this->renderer->renderViewModel($model));
weierophinney commented 8 years ago

If you implement and use renderViewModel(), you're then tying your code to a concrete implementation.

There's another path to use: the $vars argument to render() is not type-hinted, which means a fairly simple change to the render() logic in this implementation would allow you to pass a ViewModel instance instead. (I did similarly in my phly-expressive-mustache implementation, to allow passing objects instead of arrays: see its implementation.)

The one tricky part is enabling the ability to merge global/template parameters, but this can be accomplished. Since ModelInterface has both getVariables() and setVariables() specifications, these can be used to do the merging. I'll get a PR out shortly to demonstrate.

weierophinney commented 8 years ago

4 should solve this in a way that keeps the functionality compatible across implementations (assuming a given implementation can support view models; this will work for both zend-view and phly-mustache, but may break for Twig and Plates).

mtymek commented 8 years ago

I see a problem here: while you are sticking with render() method defined in an interface, actually you are also tying code to concrete implementation, just in a hidden, less obvious way. It introduces another "magic" of detecting parameter type and acting accordingly. I doubt if this approach would make switching renderer any easier than mine, I think it's going to make things more complex to understand. Another issue: you can now set template name in two ways, by using $model->setTemplate(), and by passing it as a first parameter. This can lead to confusion as well.

Perhaps the better solution would be to improve Zend\View, so that it can be used as a standalone library. In this scenario Expressive app would not depend on TemplateRendererInterface, but on Unfortunately Zend\View 3.0 is likely not going to happen soon, that's why I felt adding new method is better for current needs.

weierophinney commented 8 years ago

@mtymek I see your point. But I've also demonstrated that it currently can work with another implementation already, with no changes (it will work fine in phly-expressive-mustache, for instance). This means that a consumer can safely switch between these two implementations without changing the code consuming them. A few minor changes to the ArrayParametersTrait can make it seamless across all implementations, and I'm going to submit an additional PR against zend-expressive to make that happen.

In terms of this:

Another issue: you can now set template name in two ways, by using $model->setTemplate(), and by passing it as a first parameter. This can lead to confusion as well.

What would be confusing would be if we honored the template name set in the view model, when specifying a template name when calling render(), which is why the approach in #4 always calls setTemplate() with the template name provided during render(). As such, it has consistent behavior currently.

Regarding improving zend-view to act better as a standalone library, I'm not quite sure what you're getting at. It already is. Expressive is trying to be agnostic of the library used, and only provide interfaces, instead of relying on interfaces from specific implementations, which is why we didn't use Zend\View\Renderer\RendererInterface as the basis. Perhaps you can clarify what you mean? (It also looks like there may have been an incomplete thought between "but on Unfortunately" in your last paragraph.) (Also: zend-view v3 is imminent. It's just not a large change, but rather updates to work with the v3 versions of zend-servicemanager and zend-eventmanager.)

My feeling is that if you want to rely on features of a specific template engine, inject it instead of a TemplateRendererInterface implementation. In other words, if you're going to rely on features from a concrete implementation of the interface, just use the underlying implementation in the first place. The interfaces exist to allow you to switch implementations. You can only do so if you limit your usage to the methods specifically supported by the interface. That also means you should be careful about the arguments you provide the interface methods, to ensure consistent results across implementations.

mtymek commented 8 years ago

Thanks for extensive explanation!

About ZendView - only part of its features can be used directly. You cannot simply composer require zendframework/zend-view, inject Zend\View into your class and start using nested view models for instance. If you want to render trees (or use different pluggable renderers), you have to write your wrapper around PhpRenderer - same as you did in this repository. Because of how Zend\View triggers events, it is difficult to be used outside ZF2 app. I hit this problem a few times already (when writing my e-mail rendering module for ZF2, when working on a ZF-based microframework, finally with Expressive). Ideally ZendView would work in all its glory without any extra wrapper. BWT, ZendView 2 is complex and has its issues that cannot be solved without at least small BC breaks - see what I submitted in zend-view repo today :)

What you wrote about the contract makes sense - it's not perfect approach, but I guess we have to be realistic here :)

RalfEggert commented 8 years ago

Great work! Well, I can live with the render() method to handle ViewModel instances as well.

Just a small idea. Would this also work with JsonModel, FeedModel and ConsoleModel as well? I guess people will try it...

weierophinney commented 8 years ago

@mtymek — I've opened zendframework/zend-expressive#185 to address the view model conversion, which will make usage of render() seamless across all implementations when using ModelInterface implementations (which answers the question from @RalfEggert as well).

I've done a ton of benchmarking recently, and, honestly, Plates is far faster than zend-view at this point, and more simply incorporates all must-have features from zend-view, including the ability to serialize multiple formats (though it does not have the ability to update the response headers for things like content-type). Unless we can untangle the complexity of zend-view significantly, I foresee zend-view becoming a legacy engine in the not-too-distant future. (I currently have no ideas around that, either, other than a full rewrite, which then begs the question of migration strategies and whether or not it might not be better to join forces to assist other implementations.)

weierophinney commented 7 years ago

Closed with zendframework/zend-expressive#185.