Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Report classes should not be instantiated directly #7821

Closed
tsteur opened this issue May 4, 2015 · 0 comments
Closed

Report classes should not be instantiated directly #7821

tsteur opened this issue May 4, 2015 · 0 comments
Assignees
Labels
c: Platform For Matomo platform changes that aren't impacting any of our APIs but improve the core itself. Task Indicates an issue is neither a feature nor a bug and it's purely a "technical" change.
Milestone

Comments

@tsteur
Copy link
Member

tsteur commented May 4, 2015

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

@tsteur tsteur added Task Indicates an issue is neither a feature nor a bug and it's purely a "technical" change. c: Platform For Matomo platform changes that aren't impacting any of our APIs but improve the core itself. labels May 4, 2015
@tsteur tsteur added this to the 2.14.0 milestone May 7, 2015
@tsteur tsteur self-assigned this May 7, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: Platform For Matomo platform changes that aren't impacting any of our APIs but improve the core itself. Task Indicates an issue is neither a feature nor a bug and it's purely a "technical" change.
Projects
None yet
Development

No branches or pull requests

1 participant