Skip to content

Report classes should not be instantiated directly #7821

@tsteur

Description

@tsteur

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

Metadata

Metadata

Assignees

Labels

TaskIndicates an issue is neither a feature nor a bug and it's purely a "technical" change.c: PlatformFor Matomo platform changes that aren't impacting any of our APIs but improve the core itself.

Type

No type

Projects

No projects

Milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions