Holger wrote:Ah nice that you keep on working on Pfw_XH
.
I have to – all this copy&paste leads to such problems as
https://github.com/cmb69/twocents_xh/issues/48 (most likely 5 individual plugins are affected).
Holger wrote:BTW: is / was the code from Exchange_XH up to date?
Up-to-date regarding the view concept? Not really. Besides my rather old drafts, I've developed three view concepts which are relatively closely related. The first one was that of Pfw_XH 0.1.0, where view data have been injected into the templates as local variables (unescaped) or as view properties (escaped). I didn't like this manual escaping, so in several other plugins (amongst them Exchange_XH) I've refined this to inject the data as view properties (unescaped) and view methods (escaped). The templates could have been checked with a linter (actually, I've developed a draft) so that all echoing went to a call to a $this method, which guaranteed proper escaping. (A drawback was that callables didn't work anymore, but this feature was of somewhat limited use, anyway.) When turning back to Pfw_XH I've noticed that injecting view data as members of View was prone to BC issues (if I'd ever wanted to introduce a new property or method, even a private one, existing views/templates could cause havoc). Therefore I've decided (current Pfw_XH master) to inject the data as local variables, but wrapped in objects so automatic escaping would be possible. The additional benefit: no need to call $this->escape() anymore, which could amount to quite some additional code. However, I'm not really sure whether this view concept is actually viable – there are severe restrictions on what is possible in a template now.
cmb wrote:In fact: I had some problems to create the "Cache-Info" view.
As you can see in CacheService.php - cacheInfo(), I create some HTML in that method. That's not correct if I'm right (or at least against the concept). And, to output the html, I have to use $view->info = new HtmlString($cache->cacheInfo()); in MainAdminController.php.
Ah, I see. A simple improvement could be done by moving the $html stuff at the end of the method into a view/template, and let the method return the View object, like so:
Code: Select all
public function cacheInfo() {
// create $files as before
$view = new View('cache-info');
$view->count = $count;
$view->bytes = $bytes;
$view->files = new HtmlString($files);
return $view;
}
MainAdminController::defaultAction() would not have to be modified. That would still require the HtmlString, but most of the simple HTML could be moved into a template.
A next step could be to tackle $files. Instead of precomputing $files, the respective DirectoryIterator could be passed to the template, where another loop could be made:
Code: Select all
<?php foreach ($this->files as $file):?>
<?php if (!$fileInfo->isDot() && $fileInfo->isFile()):?>
<?=$this->escape($this->file->getFilenameName())?><br>
<?php endif?>
<?php endforeach?>
To avoid the additional if clause in the template (this would not be "allowed" in the new view concept, because it's too hard to read in a template file, in my opinion), you could build up $files like before, but instead of constructing string a simple array of the filenames would be constructed, and that would be passed to the template.
Another option would be to create a view helper class for this view/template, and to pass an instance of it to the template, so in the template you'd could access $helper->getFiles(), $helper->getSize() and $helper->getCount() (or similar). Seems to be overkill here, but still a valid option. And it might simplify unit testing.
Also
consider to use
generators instead of arrays. I've rarely used them so far, since they require PHP 5.5.0, but they appear to be a really nice feature. Therefore I've added explicit support for them in the new view concept (well, actually there is general support for Iterators, but I don't like to manually create Iterator classes, at least not until this could be done with
anonymous classes, which would require PHP 7, though).
Lack of support for anonymous classes is also the reason why I wouldn't recommend to use a
FilterIterator instead of the if clause of the DirectoryIterator loop. Having anonymous classes would allow to write:
Code: Select all
$files = (new class(new \DirectoryIterator('.')) extends FilterIterator {
public function accept() {
return !$this->current()->isDot() && $this->current()->isFile();
}
});
That might be even more succinct than a respective generator:
Code: Select all
function files() {
foreach (new \DirectoryIterator('.') as $fileInfo) {
if (!$fileInfo->isDot() && $fileInfo->isFile()) {
yield $fileInfo;
}
}
}
Holger wrote:I do not know if I understood all the details but I can tell you that coding this way is much easier when it comes to generate HTML output!
If you conclude that this concept simplifies the generation of HTML output, I think that you have understood the idea.