Refactor & improve MultiSites plugin (#23065)

* Disable autosanitize for MultiSites API * Add proper type hints accross MultiSites plugin * cs/ws * Replace Common::getRequestVar with Request class methods * some more type hints * simplify table filter * updates expected UI test file * clean up more code & bring code to phpstan lvl 5 * Add tooltips to table header * small code improvements
matomo-org · Feb 26, 2025 · 1505379 · 1505379
1 parent 64e6ea2
commit 1505379
Show file tree

Hide file tree

Showing 23 changed files with 431 additions and 362 deletions.
diff --git a/core/Menu/MenuAbstract.php b/core/Menu/MenuAbstract.php
@@ -91,7 +91,7 @@ protected function getAllMenus()
      * Adds a new entry to the menu.
      *
      * @param string $menuName The menu's category name. Can be a translation token.
-     * @param string $subMenuName The menu item's name. Can be a translation token.
+     * @param null|string $subMenuName The menu item's name. Can be a translation token.
      * @param string|array $url The URL the admin menu entry should link to, or an array of query parameters
      *                          that can be used to build the URL.
      * @param int $order The order hint.

diff --git a/core/Plugin/API.php b/core/Plugin/API.php
@@ -47,6 +47,7 @@ abstract class API
 {
     private static $instances;
 
+    /** @var bool */
     protected $autoSanitizeInputParams = true;
 
     /**

diff --git a/plugins/MultiSites/API.php b/plugins/MultiSites/API.php
diff --git a/plugins/MultiSites/Columns/Metrics/EcommerceOnlyEvolutionMetric.php b/plugins/MultiSites/Columns/Metrics/EcommerceOnlyEvolutionMetric.php
@@ -21,18 +21,19 @@
  */
 class EcommerceOnlyEvolutionMetric extends EvolutionMetric
 {
+    /** @var bool */
     private $isRevenueEvolution;
 
     public function __construct(
         $wrapped,
         DataTable $pastData,
         $evolutionMetricName = false,
-        $quotientPrecision = 0,
+        int $quotientPrecision = 0,
         ?DataTable $currentData = null
     ) {
         parent::__construct($wrapped, $pastData, $evolutionMetricName, $quotientPrecision, $currentData);
 
-        $this->isRevenueEvolution = $this->getName() == 'revenue_evolution';
+        $this->isRevenueEvolution = $this->getName() === 'revenue_evolution';
     }
 
     public function compute(Row $row)

diff --git a/plugins/MultiSites/Controller.php b/plugins/MultiSites/Controller.php
@@ -15,14 +15,13 @@
 use Piwik\Piwik;
 use Piwik\Plugins\Goals\API as GoalsAPI;
 use Piwik\Plugins\SitesManager\API as SitesManagerAPI;
+use Piwik\Request;
 use Piwik\Translation\Translator;
 use Piwik\View;
 
 class Controller extends \Piwik\Plugin\Controller
 {
-    /**
-     * @var Translator
-     */
+    /** @var Translator */
     private $translator;
 
     public function __construct(Translator $translator)
@@ -32,20 +31,20 @@ public function __construct(Translator $translator)
         $this->translator = $translator;
     }
 
-    public function index()
+    public function index(): string
     {
         return $this->getSitesInfo($isWidgetized = false);
     }
 
-    public function standalone()
+    public function standalone(): string
     {
         return $this->getSitesInfo($isWidgetized = true);
     }
 
     /**
      * @throws \Piwik\NoAccessException
      */
-    public function getSitesInfo($isWidgetized = false)
+    protected function getSitesInfo(bool $isWidgetized = false): string
     {
         Piwik::checkUserHasSomeViewAccess();
 
@@ -63,9 +62,11 @@ public function getSitesInfo($isWidgetized = false)
         // if the current date is today, or yesterday,
         // in case the website is set to UTC-12), or today in UTC+14, we refresh the page every 5min
         if (
-            in_array($date, array('today', date('Y-m-d'),
-                                  'yesterday', Date::factory('yesterday')->toString('Y-m-d'),
-                                  Date::factory('now', 'UTC+14')->toString('Y-m-d')))
+            in_array($date, [
+                'today', date('Y-m-d'),
+                'yesterday', Date::factory('yesterday')->toString('Y-m-d'),
+                Date::factory('now', 'UTC+14')->toString('Y-m-d')
+            ])
         ) {
             $view->autoRefreshTodayReport = Config::getInstance()->General['multisites_refresh_after_seconds'];
         }
@@ -77,11 +78,9 @@ public function getSitesInfo($isWidgetized = false)
         return $view->render();
     }
 
-    public function getEvolutionGraph($columns = false)
+    public function getEvolutionGraph(): ?string
     {
-        if (empty($columns)) {
-            $columns = Common::getRequestVar('columns');
-        }
+        $columns = Request::fromRequest()->getStringParameter('columns');
         $api = "API.get";
 
         if ($columns == 'revenue') {

diff --git a/plugins/MultiSites/Dashboard.php b/plugins/MultiSites/Dashboard.php
@@ -26,14 +26,12 @@ class Dashboard
     /** @var DataTable */
     private $sitesByGroup;
 
-    /**
-     * @var int
-     */
+    /** @var int */
     private $numSites = 0;
 
     /**
      * Array of metrics that will be displayed and will be number formatted
-     * @var array
+     * @var array<string>
      */
     private $displayedMetricColumns = [
         'nb_visits', 'nb_pageviews', 'hits', 'nb_actions', 'revenue',
@@ -43,14 +41,14 @@ class Dashboard
     /**
      * @param string $period
      * @param string $date
-     * @param string|false $segment
+     * @param string|null $segment
      */
-    public function __construct($period, $date, $segment)
+    public function __construct(string $period, string $date, ?string $segment)
     {
         $sites = Request::processRequest('MultiSites.getAll', [
             'period' => $period,
             'date' => $date,
-            'segment' => $segment,
+            'segment' => $segment ?? '',
             'enhanced' => '1',
             // NOTE: have to select everything since with queued filters disabled some metrics won't be renamed to
             // their display name, and so showColumns will end up removing those.
@@ -63,7 +61,7 @@ public function __construct($period, $date, $segment)
 
         $sites->deleteRow(DataTable::ID_SUMMARY_ROW);
 
-        /** @var DataTable $pastData */
+        /** @var null|DataTable $pastData */
         $pastData = $sites->getMetadata('pastData');
 
         $sites->filter(function (DataTable $table) use ($pastData) {
@@ -93,16 +91,16 @@ public function __construct($period, $date, $segment)
         $this->setSitesTable($sites);
     }
 
-    public function setSitesTable(DataTable $sites)
+    public function setSitesTable(DataTable $sites): void
     {
         $this->sitesByGroup = $this->moveSitesHavingAGroupIntoSubtables($sites);
         $this->rememberNumberOfSites();
     }
 
-    public function getSites($request, $limit)
+    public function getSites(array $request, int $limit): array
     {
         $request['filter_limit']  = $limit;
-        $request['filter_offset'] = isset($request['filter_offset']) ? $request['filter_offset'] : 0;
+        $request['filter_offset'] = isset($request['filter_offset']) ? (int) $request['filter_offset'] : 0;
 
         $this->makeSitesFlatAndApplyGenericFilters($this->sitesByGroup, $request);
         $sites = $this->convertDataTableToArrayAndApplyQueuedFilters($this->sitesByGroup, $request);
@@ -111,7 +109,7 @@ public function getSites($request, $limit)
         return $sites;
     }
 
-    public function getTotals()
+    public function getTotals(): array
     {
         $totals = [
             'nb_pageviews'          => $this->sitesByGroup->getMetadata('total_nb_pageviews'),
@@ -129,7 +127,7 @@ public function getTotals()
         return $totals;
     }
 
-    private function formatMetrics(&$metrics)
+    private function formatMetrics(array &$metrics): void
     {
         if (\Piwik\Request::fromRequest()->getStringParameter('format_metrics', '0') === '0') {
             return; // do not format metrics if requires unformatted
@@ -149,23 +147,23 @@ private function formatMetrics(&$metrics)
     }
 
 
-    public function getNumSites()
+    public function getNumSites(): int
     {
         return $this->numSites;
     }
 
-    public function search($pattern)
+    public function search(?string $pattern): void
     {
         $this->nestedSearch($this->sitesByGroup, $pattern);
         $this->rememberNumberOfSites();
     }
 
-    private function rememberNumberOfSites()
+    private function rememberNumberOfSites(): void
     {
         $this->numSites = $this->sitesByGroup->getRowsCountRecursive();
     }
 
-    private function nestedSearch(DataTable $sitesByGroup, $pattern)
+    private function nestedSearch(DataTable $sitesByGroup, ?string $pattern): void
     {
         foreach ($sitesByGroup->getRows() as $index => $site) {
             $label = strtolower($site->getColumn('label'));
@@ -189,10 +187,7 @@ private function nestedSearch(DataTable $sitesByGroup, $pattern)
         }
     }
 
-    /**
-     * @return string
-     */
-    public function getLastDate()
+    public function getLastDate(): string
     {
         $lastPeriod = $this->sitesByGroup->getMetadata('last_period_date');
 
@@ -205,7 +200,7 @@ public function getLastDate()
         return $lastPeriod;
     }
 
-    private function convertDataTableToArrayAndApplyQueuedFilters(DataTable $table, $request)
+    private function convertDataTableToArrayAndApplyQueuedFilters(DataTable $table, array $request): array
     {
         $request['serialize'] = 0;
         $request['expanded'] = 0;
@@ -214,10 +209,10 @@ private function convertDataTableToArrayAndApplyQueuedFilters(DataTable $table,
         $request['disable_generic_filters'] = 1;
 
         $responseBuilder = new ResponseBuilder('json', $request);
-        return json_decode($responseBuilder->getResponse($table, 'MultiSites', 'getAll'), true);
+        return json_decode($responseBuilder->getResponse($table, 'MultiSites', 'getAll'), true) ?: [];
     }
 
-    private function moveSitesHavingAGroupIntoSubtables(DataTable $sites)
+    private function moveSitesHavingAGroupIntoSubtables(DataTable $sites): DataTable
     {
         /** @var DataTableSummaryRow[] $groups */
         $groups = [];
@@ -256,7 +251,7 @@ private function moveSitesHavingAGroupIntoSubtables(DataTable $sites)
         return $sitesByGroup;
     }
 
-    private function createGroupSubtable(DataTable $sites)
+    private function createGroupSubtable(DataTable $sites): DataTable
     {
         $table = new DataTable();
         $processedMetrics = $sites->getMetadata(DataTable::EXTRA_PROCESSED_METRICS_METADATA_NAME);
@@ -265,7 +260,7 @@ private function createGroupSubtable(DataTable $sites)
         return $table;
     }
 
-    private function makeCloneOfDataTableSites(DataTable $sites)
+    private function makeCloneOfDataTableSites(DataTable $sites): DataTable
     {
         $sitesByGroup = $sites->getEmptyClone(true);
         // we handle them ourselves for faster performance etc. This way we also avoid to apply them twice.
@@ -306,7 +301,7 @@ private function makeCloneOfDataTableSites(DataTable $sites)
      * @param DataTable $table
      * @param array $request
      */
-    private function makeSitesFlatAndApplyGenericFilters(DataTable $table, $request)
+    private function makeSitesFlatAndApplyGenericFilters(DataTable $table, array $request): void
     {
         // we handle limit here as we have to apply sort filter, then make sites flat, then apply limit filter.
         $filterOffset = $request['filter_offset'];
@@ -318,8 +313,8 @@ private function makeSitesFlatAndApplyGenericFilters(DataTable $table, $request)
         $table->disableFilter('Limit');
 
         // this will apply the sort filter
-        /** @var DataTable $table */
         $genericFilter = new DataTablePostProcessor('MultiSites', 'getAll', $request);
+        /** @var DataTable */
         $table = $genericFilter->applyGenericFilters($table);
 
         // make sure from now on the sites will be no longer sorted, they were already sorted
@@ -329,7 +324,7 @@ private function makeSitesFlatAndApplyGenericFilters(DataTable $table, $request)
         $table->filter('Piwik\Plugins\MultiSites\DataTable\Filter\NestedSitesLimiter', [$filterOffset, $filterLimit]);
     }
 
-    private function enrichValues($sites)
+    private function enrichValues(array $sites): array
     {
         foreach ($sites as &$site) {
             if (!isset($site['idsite'])) {

diff --git a/plugins/MultiSites/DataTable/Filter/NestedSitesLimiter.php b/plugins/MultiSites/DataTable/Filter/NestedSitesLimiter.php
@@ -49,19 +49,19 @@
  */
 class NestedSitesLimiter extends BaseFilter
 {
+    /** @var int */
     private $offset = 0;
+    /** @var int */
     private $limit  = 0;
-    /**
-     * @var Row[]
-     */
-    private $rows   = array();
+    /** @var Row[] */
+    private $rows   = [];
 
     /**
      * Constructor.
      *
      * @param DataTable $table The table to eventually filter.
      */
-    public function __construct($table, $offset, $limit)
+    public function __construct(DataTable $table, int $offset, int $limit)
     {
         parent::__construct($table);
         $this->offset = $offset;
@@ -102,17 +102,17 @@ public function filter($table)
         $table->setRows($this->rows);
     }
 
-    private function hasNumberOfRequestedRowsFound()
+    private function hasNumberOfRequestedRowsFound(): bool
     {
         return count($this->rows) >= $this->limit;
     }
 
-    private function hasRows()
+    private function hasRows(): bool
     {
         return count($this->rows) !== 0;
     }
 
-    private function addRowIfNeeded(Row $row, $numRows)
+    private function addRowIfNeeded(Row $row, int $numRows): void
     {
         $inOffset = $numRows >= $this->offset;
 
@@ -122,9 +122,9 @@ private function addRowIfNeeded(Row $row, $numRows)
     }
 
     /**
-     * @param Row $lastGroupFromPreviousPage
+     * @param null|Row $lastGroupFromPreviousPage
      */
-    private function prependGroupIfFirstSiteBelongsToAGroupButGroupIsMissingInRows($lastGroupFromPreviousPage)
+    private function prependGroupIfFirstSiteBelongsToAGroupButGroupIsMissingInRows(?Row $lastGroupFromPreviousPage): void
     {
         if ($lastGroupFromPreviousPage && !empty($this->rows)) {
             // the result starts with a row that does belong to a group, we make sure to show this group before that site

diff --git a/plugins/MultiSites/Menu.php b/plugins/MultiSites/Menu.php
@@ -9,15 +9,15 @@
 
 namespace Piwik\Plugins\MultiSites;
 
-use Piwik\Common;
 use Piwik\Menu\MenuTop;
 use Piwik\Piwik;
+use Piwik\Request;
 
 class Menu extends \Piwik\Plugin\Menu
 {
     public function configureTopMenu(MenuTop $menu)
     {
-        $idSite = Common::getRequestVar('idSite', 0, 'int');
+        $idSite = Request::fromRequest()->getIntegerParameter('idSite', 0);
 
         $urlParams = $this->urlForActionWithDefaultUserParams('index', ['segment' => false, 'idSite' => $idSite ?: false]);
         $tooltip   = Piwik::translate('MultiSites_TopLinkTooltip');
-Original file line number
+Diff line change
@@ Expand Up / @@ -47,6 +47,7 @@ abstract class API @@
     {
         private static $instances;
+        /** @var bool */
         protected $autoSanitizeInputParams = true;
         /**
@@ Expand Down @@