Ideas for cleaning up views: view models, smarty blocks and template inheritance
x00
MVP
I haven't put his as a suggestion on the core github, becuase I have not formalised this and would like more feedback. It is something you could employ yourself in your addons.
Views are a weak area of vanilla becuase there is still a lot of logic interleaved with the markup. The markup is inserted in various ways (e.g wrap and other render functions, echo'd concatenated text an variables, etc). I remember Mark cerca Vanilla 1 begin dissatisfied with how mangled the views were, and this hasn't really changed to this day. The MVC with the best of intentions still doesn't limit the view logic idealism aside, and helper if anything complicate matters further.
Borrowing the view model idea form MVVM you can have a pseudo view model to extend the controller data at the render stage which takes the bulk the view specific logic outside of the templates, then you can have a separate smarty template with mostly markup.
Smarty has somethign called blocks which allow you to break view into manageable chunks these can be latter extended, in a way that doesn't require replacing the whole view, but instead focusing on the required. Even reordering of blocks can be achieve.
https://www.smarty.net/docs/en/language.function.block.tpl
https://www.smarty.net/docs/en/advanced.features.template.inheritance.tpl
First a basic example of what I'm taking about. Currently applications/vanilla/views/post.php looks like this:
<?php if (!defined('APPLICATION')) exit();
$CancelUrl = $this->data('_CancelUrl');
if (!$CancelUrl) {
$CancelUrl = '/discussions';
if (c('Vanilla.Categories.Use') && is_object($this->Category)) {
$CancelUrl = '/categories/'.urlencode($this->Category->UrlCode);
}
}
?>
<div id="DiscussionForm" class="FormTitleWrapper DiscussionForm">
<?php
if ($this->deliveryType() == DELIVERY_TYPE_ALL) {
echo wrap($this->data('Title'), 'h1', ['class' => 'H']);
}
echo '<div class="FormWrapper">';
echo $this->Form->open();
echo $this->Form->errors();
$this->fireEvent('BeforeFormInputs');
if ($this->ShowCategorySelector === true) {
$options = ['Value' => val('CategoryID', $this->Category), 'IncludeNull' => true];
if ($this->Context) {
$options['Context'] = $this->Context;
}
$discussionType = property_exists($this, 'Type') ? $this->Type : $this->data('Type');
if ($discussionType) {
$options['DiscussionType'] = $discussionType;
}
echo '<div class="P">';
echo '<div class="Category">';
echo $this->Form->label('Category', 'CategoryID'), ' ';
echo $this->Form->categoryDropDown('CategoryID', $options);
echo '</div>';
echo '</div>';
}
echo '<div class="P">';
echo $this->Form->label('Discussion Title', 'Name');
echo wrap($this->Form->textBox('Name', ['maxlength' => 100, 'class' => 'InputBox BigInput', 'spellcheck' => 'true']), 'div', ['class' => 'TextBoxWrapper']);
echo '</div>';
$this->fireEvent('BeforeBodyInput');
echo '<div class="P">';
echo $this->Form->bodyBox('Body', ['Table' => 'Discussion', 'FileUpload' => true]);
echo '</div>';
$Options = '';
// If the user has any of the following permissions (regardless of junction), show the options.
if (Gdn::session()->checkPermission('Vanilla.Discussions.Announce')) {
$Options .= '<li>'.checkOrRadio('Announce', 'Announce', $this->data('_AnnounceOptions')).'</li>';
}
$this->EventArguments['Options'] = &$Options;
$this->fireEvent('DiscussionFormOptions');
if ($Options != '') {
echo '<div class="P">';
echo '<ul class="List Inline PostOptions">'.$Options.'</ul>';
echo '</div>';
}
$this->fireEvent('AfterDiscussionFormOptions');
echo '<div class="Buttons">';
$this->fireEvent('BeforeFormButtons');
echo $this->Form->button((property_exists($this, 'Discussion')) ? 'Save' : 'Post Discussion', ['class' => 'Button Primary DiscussionButton']);
if (!property_exists($this, 'Discussion') || !is_object($this->Discussion) || (property_exists($this, 'Draft') && is_object($this->Draft))) {
echo $this->Form->button('Save Draft', ['class' => 'Button DraftButton']);
}
echo $this->Form->button('Preview', ['class' => 'Button PreviewButton']);
echo ' '.anchor(t('Edit'), '#', 'Button WriteButton Hidden')."\n";
$this->fireEvent('AfterFormButtons');
echo anchor(t('Cancel'), $CancelUrl, 'Button Cancel');
echo '</div>';
echo $this->Form->close();
echo '</div>';
?>
</div>
If you strip out the markup and instead extend data you can get something like this
<?php if (!defined('APPLICATION')) exit();
$cancelUrl = $this->data('_CancelUrl');
if (!$cancelUrl) {
$cancelUrl = '/discussions';
if (c('Vanilla.Categories.Use') && is_object($this->Category)) {
$cancelUrl = '/categories/'.urlencode($this->Category->UrlCode);
}
}
if ($this->deliveryType() == DELIVERY_TYPE_ALL) {
$this->setData('view.title', $this->data('Title'));
}
$this->setData('form.open', $this->Form->open());
$this->setData('form.errors', $this->Form->errors());
if ($this->ShowCategorySelector === true) {
$options = ['Value' => val('CategoryID', $this->Category), 'IncludeNull' => true];
if ($this->Context) {
$options['Context'] = $this->Context;
}
$discussionType = property_exists($this, 'Type') ? $this->Type : $this->data('Type');
if ($discussionType) {
$options['DiscussionType'] = $discussionType;
}
$this->setData(
'form.categorySelectorLabel',
$this->Form->label('Category', 'CategoryID')
);
$this->setData(
'form.categorySelectorDropdown',
$this->Form->categoryDropDown('CategoryID', $options)
);
}
$this->setData(
'form.discussionTitleLabel',
$this->Form->label('Discussion Title', 'Name')
);
$this->setData(
'form.discussionTitleInput',
$this->Form->textBox('Name', ['maxlength' => 100, 'class' => 'InputBox BigInput', 'spellcheck' => 'true'])
);
$this->setData(
'form.bodyBox',
$this->Form->bodyBox('Body', ['Table' => 'Discussion', 'FileUpload' => true])
);
$options = [];
// If the user has any of the following permissions (regardless of junction), show the options.
if (Gdn::session()->checkPermission('Vanilla.Discussions.Announce')) {
$options[] = checkOrRadio('Announce', 'Announce', $this->data('_AnnounceOptions'));
}
$this->EventArguments['Options'] = &$options;
$this->fireEvent('DiscussionFormOptions');
if (count($options)) {
$this->setData('view.showOptions', true);
}
$this->setData(
'form.saveButton',
$this->Form->button((property_exists($this, 'Discussion')) ? 'Save' : 'Post Discussion', ['class' => 'Button Primary DiscussionButton'])
);
if (!property_exists($this, 'Discussion') || !is_object($this->Discussion) || (property_exists($this, 'Draft') && is_object($this->Draft))) {
$this->setData(
'form.draftButton',
$this->Form->button('Save Draft', ['class' => 'Button DraftButton'])
);
}
$this->setData(
'form.previewButton',
$this->Form->button('Preview', ['class' => 'Button PreviewButton'])
);
$this->setData(
'form.editButton',
anchor(t('Edit'), '#', 'Button WriteButton Hidden')
);
$this->setData('form.close', $this->Form->close());
A simple way you can include a template like so (not implicitly extendible):
// smarty view handler automatically applied
echo $this->fetchView('discussion.tpl');
discussion.tpl could look like so
{block "main"}
<div id="DiscussionForm" class="FormTitleWrapper DiscussionForm">
{if isset($view.title)}
{block "title"}
<h1>{$view.title}</h1>
{/blocks}
{/if}
<div class="FormWrapper">
{$form.open}
{block "formErrors"}{$form.errors}{/block}
{event name="beforeFormInputs"}
{block "categorySelector"}
{if isset($view.categorySelectorDropdown)}
<div class="P">
<div class="Category">
{block "categorySelectorInputs"}
{$form.categorySelectorLabel} {$fomr.categorySelectorDropdown}
{block}
</div>
</div>
{/if}
{/block}
{block "name"}
<div class="P">
{block "nameInputs"}
{$form.nameLabel}
{$form.nameInput}
{/block}
</div>
{/block}
{block "body"}
{event name="beforeBodyInput"}
<div class="P">
<div class="TextBoxWrapper">
{block "bodyInput"}
{$form.bodyBox}
}
</div>
</div>
{/block}
{block "postOptions"}
{if isset($view.showOptions)}
<div class="P">
<ul class="List Inline PostOptions">
{foreach item=$option from=$form.postOptions}
{block "postOptionsItem"}<li>{$option}</li>{/block}
{/foreach}
</ul>
</div>
{/if}
{event name="afterDiscussionFormOptions"}
{/block}
{block "buttons"}
<div class="Buttons">
{event name="beforeFormButtons"}
{$form.saveButton}
{$form.previewButton} {$form.editButton}
{event name="afterFormButtons"}
{$form.cancelButton}
</div>
{/block}
{$form.close}
</div>
</div>
{/block}
Why not use some view model class instead? Well you could use a class, but the logic is present in much the same way as it currently is, therefore in terms of ease of conversion this is simpler and perfectly readable, however absolutely you could encapsulate it. The reality is though, this is not a view model in the strictest sense. Must of the data is straight from the controller, or assigned in the smarty class. The custom smarty functions, and the core function smarty has permission to access, reference global object like Gdn and Gdn_Theme where as a view model is supposed to be the the sole go between.
Also it would be tricky to convert to three tier architecture (e.g pushing/pulling the compiled template up to the presentation layer on a separate VM and fetching the view model data on request), as these as these global objects would not be there, and it would not be secure to serialise them. There could however be a way of substituting version of these function on that tier with api methods to access these functions securely, which is kind of in the spirit of view model security principles. However this is a side note, only relevant to those who need three tier architecture (e.g. some US government related institutions/industries are legally required to use 3 tier architecture but there are way you can make 2 tier architecture fit into 3 tier technically speaking without the frawork having changed.
Anyway the example above is fine for addons, but doesn't really cover extending the core apps views.
Overriding views I have always discouraged, becuase it can break in updates. However this can happen for hooks as well. If logic is separated, at the very least and change tot he logic can be made mindfully, and block maintain even if there are markup change.
The key thing is tpl files could be extended rather then over-ridden
Template inheritance in smarty can be done like so
$smarty->display('extends:original.tpl|extended.tpl');
So there could be convention for the view model data and the template files, and the original the extended tempted can be compiled together through this implicit convention
say you wanted to append the title block with a subtitle you could do it like so
{block "title" append}
<h2>{c "My Subtitle"}</h2>
{/block}
It is even possible to change the order of blocks using capture
https://www.smarty.net/docs/en/language.function.capture.tpl
{capture "myCategorySelector"}
{block "categorySelector"}
{$smarty.block.parent}
{/block}
{/capture}
{capture "myName"}
{block "name"}
{$smarty.block.parent}
{/block}
{/capture}
{block "body" prepend}
{$smarty.capture.myName}
{$smarty.capture.myCategorySelector}
{/block}
obviously some though would have to go into how parent views use block in order not to complete noble extendibility. I think that you probably should not have standalone items not inside blocks and the main block should contain only blocks.
grep is your friend.
Comments
Note the only logic used the template are if conditions and loops
grep is your friend.
Two things are true:
As we move towards a more React-based system, you'll find us much more strictly separating these things from our views. The R&D team is currently using Twig to build a new cloud product. While the product in particular won't be seen in open source, I do expect to see many of the underlying pieces & improved architectural ideas to get worked back into core as time goes on. (I'm not suggesting Twig in particular is one of those pieces - it may just be a stepping stone for us.)
I understand that however addon developer have endured pain too, so I think it is not unreasonable for theme developer to adapt as well.
It was never a viable option to use view overrides as it is. then chance of an update breaking was quite likely, so always a maintenance issue, which is why I discouraged view overrides in general. I avoided themes with a lot of view overrides like the plague.
it is good that you are move towards a more dynamic system, such as using react. Context driven is good, rather than hand of god techniques.
The concept of view models and services apply to react, angular, etc.
In an ideal world if you can completely detach the presentation (e.g in theory put in on another server) from the logic layer the is a big plus. MVC doesn't traditionally do this it is a triangular on one tier with maybe the data on another. I do understand the N-tier isn't the be an end all. However at least the idea treating the presentation as and independent thing should be a focus.
A browser cannot be taken seriously as the third tier, because it is not completely under the control of the app, however with mobile aps you could treat the client more so, there are more secure systems where you have three tiers on the same cloud network, though might not needed that unless you are required to have it.
What I would discourage is is some sort of fancy canvas layout UI which might convenient for some, but get in the way for developers. I wouldn't discount anything, but that sort of think make me nervous. A lot of compromise and a lot of work would have to go into such a system, without any more flexibility.
Obviously no matter which way you go themes will have to adapt no different from addons.
grep is your friend.
Yes, but we also can't break every cloud customer's theme in the meantime, nor break every open source install during an update for that matter, and we don't have an infinite staff to update every theme. It's tricky.
Of course this is what I'm preaching, as a company you need should engage developers if you value them so they are prepared for releases. I know you are trying to provide info, but it is in scraps. I don't think always when changes are made, an assessment is made of the potential impact. It is not just about depreciation warnings either it is about the underling architecture and conventions. It might not be noticeable to bit piece developer, but if you have produced a lot of code it is noticeable. Again not criticising change itself. Change is good.
grep is your friend.
We only have visibility of two things: Our own ecosystem, and the general open source community on this site.
Open source places hundreds of constraints on every decision we make. We to the best we can do signal our changes far enough in advance that everyone can change in the same direction as us. That said, every change is painful and surprising when you have any custom code at all. I should know - I have sites stuck on the 2.5 branch because I'm having trouble upgrading PHP.
Our reality is we have very little assistance from open source (but shout out to @R_J and @bleistivt in particular for their excellent issues, audits, and patches). So we don't have a lot of bandwidth for thinking about change roadmaps for open source, nor do we have anyone from the community deeply involved enough to consult on long-term goals. We also can only see the use cases and addons announced on this site, not other secret ecosystems. We're muddling thru as best we can. And we continue to put effort into building our open source community.
I'm open to specific suggestions about how we could have better prepared or signaled changes we made or are making. But "please do more" is a bit frustrating when we feel like we're doing a lot and don't know what you're asking for, really.
When code is re-factored e.g structural and procedural changes occur (e.g. the new addon manager) developers need to understand their impact. Loading of pugins is done differently for example and that is quite different thing from just the new json format.
Developer need time to change their code however are not going make any change unless they have release candidate, and it can be a race against time, especially if they have many addons. Developer aren't involved in a process. I think they should be notified, and notification should be a requirement to use the addon section IMO.
It is too mystifying for new developer to use this framework. Often you have use very different methodologies for simualr things for no particular reason. It might be time against to look at the rage of conventions and methodologies you are using consider the pros and con, and promote some practices. Times have change since mark first wrote his blog posts. At least then if you make changes you have something concrete to inform about. If folk choose to go off piste then it is reasonable expectation that they will have more to figure out. This is assuming that conventions aren't purely idealogical but purposeful.
Honestly you should filter out addon that haven't been updated since the latest update, and group them by update. Yes it will be drastic but it will make more obvious the problem you have with keeping developers engaged and save a lot of confusion with users. Also quite honestly I'm probably not the only one that want to drop my support of certain addons altogether, becuase we can only dedicate item to some, and some are just no longer relevant. Why are vanilla 1 addons still listed?
You find is frustrating but is a question of how valuable as a company you think addon developer are. That means actually investing in a developer program, not appealing to the to the few people that learned how the frawork works through hours of reading through the codebase or shotgun approaches, who might have free time to spare. There aren't many developer with experience, becuase there are so many frameworks to choose from, it wouldn't always appeal even if it is more than suitable.
Maybe consider paying someone like R_J to run the developer program, even if it one day a week. To engage developers so to encourage more active pluign development. It is not that uncommon on other framework for PRs to be funded. Either the client is paying for a PRs development that is relevant through the developers general work, or sometime the client has more explicitly funded a developer to be a general contributor. The later is rarer, but does happen. This is becuase if they are big enough they have made an investment in choosing the framework.
Again it is question of how important you think a developer program is, and the addon market place. If you don't think its is that important then don't bother.
It is all to do with confidence.
grep is your friend.