-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Description
In Controllers we often instantiate a report directly such as in https://github.com/piwik/piwik/blob/2.13.0-rc3/plugins/VisitorInterest/Controller.php#L22-L25 , also related reports are currently instantiated directly eg in https://github.com/piwik/piwik/blob/2.13.0-rc3/plugins/Actions/Reports/GetPageTitles.php#L83-L86 . For example new GetNumberOfVisitsPerVisitDuration()
.
This is not good as another plugin could overwrite this report instance and for example provide a different name or display it in a different way.
class MyReport extends GetNumberOfVisitsPerVisitDuration
{
protected function init()
{
parent::init();
$this->module = 'VisitorInterest';
$this->action = 'getNumberOfVisitsPerVisitDuration';
$this->name = 'My custom name';
}
public function configureView($view)
{
// display differently based on custom needs
}
}
Instead a factory should be used at all times. Eg Report::factory('VisitorInterest', 'getNumberOfVisitsPerVisitDuration');
.
This could be also done via DI but for this we'd need to change Plugin\Manager::find(Multi)Components
.
Using the factory will be required for #7131 and #4734 . The Report::factory($module, $action, $idsite|$type)
(or Report::builder()
) will instantiate a report and based on the type of the site (website, mobileapp, ...) not return the report instance or change properties of the report instance (eg overwrite the report name). Instead of using one method we might want to create eg a ReportRepository
and a ReportBuilder
as reports itself should be not aware of Sites and Types.
I do not really care how it is solved. It will be just important that we create report instances via one method that can decide whether we're allowed to return the report and can change the report instance (eg $report->setName('MobileAppName');
). For now we do not care about idSites
and types
in this issue. It is only a preparing so that we can refactor later. Therefore we could just go with factory($module, $action) for now