Let controllers specify what type a view to be rendered is #14309
Conversation
…not always assumed based on controller type.
| @@ -576,11 +583,16 @@ protected function setMaxDateView(Date $maxDate, $view) | |||
| * Will exit on error. | |||
| * | |||
| * @param View $view | |||
| * @param string|null $viewType 'basic' or 'admin'. If null, set based on the type of controller. | |||
There was a problem hiding this comment.
if someone extends this controller, and passes admin it wouldn't work as expected right?
There was a problem hiding this comment.
Are you suggesting if admin is passed, we should only allow it if $this instanceof ControllerAdmin? Or something else?
There was a problem hiding this comment.
I just meant might need to be updated in the docs ?
The check with instanceof might be useful though and prevent false expectations if admin is passed. This way a developer would directly notice to extend different class.
There was a problem hiding this comment.
It would sort of work as expected if 'admin' is passed, since setBasicVariablesView() is overridden in ControllerAdmin. I think there would be many more variables set (and possibly an exception if there's no idsite or something like that).
We could simply keep it undocumented except for renderTemplate(), doesn't really need to be set anywhere else I think.
There was a problem hiding this comment.
Or should we just move setBasicVariablesNoneAdminView etc to plugin\Controller and make it work even when it extends only Plugin\Controller and not ControllerAdmin? just a random thought, no preference in any direction here
There was a problem hiding this comment.
The problem is with:
class SitesManagerController extends Plugin\AdminController {
public function myNormalAction() {
return $this->renderTemplate('bar.twig');
}
}
Where ControllerAdmin::setBasicVariablesView() calls ControllerAdmin::setBasicVariablesAdminView(), when we don't want it to.
There was a problem hiding this comment.
Or are you just talking about that specific case? I'm not sure if we should allow it, ie, should normal controllers be allowed to render admin pages?
There was a problem hiding this comment.
should normal controllers be allowed to render admin pages?
No preference. It's just thatrenderTemplate()inPlugin\Controllerhas this parameter and it's documented to setadminoptionally. So I would expect it to work even whenPlugin\Controlleris extended. We can also throw an exception in this case though.
There was a problem hiding this comment.
Will throw an exception. We could change $viewType to $isBasicView or something, but then a dev could supply false.
This isn't an ideal solution, but I'm not too sure of another easy/quick one.
|
@diosmosis it seems this is actually breaking API: https://travis-ci.org/matomo-org/matomo/jobs/515032508#L925 The method I will look through the detailed code changes later again. |
core/Plugin/Controller.php
Outdated
| } elseif ((empty($this->site) || empty($this->idSite)) | ||
| && $viewType == 'admin' | ||
| ) { | ||
| $this->setBasicVariablesView($view, $viewType); |
There was a problem hiding this comment.
@diosmosis wouldn't we also call this when using viewType=view in some cases? basic variables does different things than general variables?
There was a problem hiding this comment.
Yes seems so, I'm not sure why I made this change... I'll have to test again
mattab
added
the
c: Platform
Currently when normal non-admin views are rendered in admin controllers, it is assumed they are admin views. Settings for admin views are thus set, which can cause problems for normal views.
For example, say a matomo allows embedding pages, but not settings pages, then a user loads a site with no data. The
siteWithoutData()action is invoked, and it is assumed to be an admin controller action, sosetBasicVariablesAdminView()is called. This method assumes the view is a settings page so it will disable embedding pages sinceenable_framed_settingsis not enabled.This PR fixes this by allowing the view type to be specified to
renderTemplate()and other methods. It's a bit odd looking, but it's hard to find a way to fix this properly given how many variables get set.