Adds PHPStan, [no_release]

.git-hooks-matomo/pre-push

@@ -0,0 +1,106 @@
+#!/bin/bash
+
+# This hook is called with the following parameters:
+#
+# $1 -- Name of the remote to which the push is being done
+# $2 -- URL to which the push is being done
+#
+# If pushing without using a named remote those arguments will be equal.
+#
+# Information about the commits which are being pushed is supplied as lines to
+# the standard input in the form:
+#
+#   <local ref> <local oid> <remote ref> <remote oid>
+
+
+
+### Check we're running in the context of a plugin and get helpful dir variables ###
+
+REPO_DIR="$(git rev-parse --show-toplevel)"
+echo "Running pre-commit hook in repo: $REPO_DIR"
+
+if [[ "$REPO_DIR" =~ /plugins/(.*) ]]; then
+    PLUGIN_PATH="plugins/${BASH_REMATCH[1]}/"
+else
+    echo "Not a plugin, not running any further checks"
+    exit 1
+fi
+MATOMO_DIR=$(echo "$REPO_DIR" | sed -E 's|/plugins/.*$||')
+
+
+
+### Figure out how to run PHPStan - ddev or not. ###
+
+COMMAND=""
+# Use local PHP if setup
+if command -v php >/dev/null 2>&1; then
+    if [ -f "${MATOMO_DIR}/vendor/bin/phpstan" ]; then
+      COMMAND="${MATOMO_DIR}/vendor/bin/phpstan"
+      PLUGIN_PATH=''
+    fi
+elif command -v ddev >/dev/null 2>&1; then
+  # Use ddev if setup (overridding local setup)
+  if [ -d "$MATOMO_DIR/.ddev" ]; then
+    cd "$MATOMO_DIR" || exit 1
+    if ddev status 2>&1 > /dev/null; then
+      COMMAND="ddev exec phpstan"
+    fi
+  fi
+fi
+# If no command, exit
+if [[ -z "$COMMAND" ]]; then
+  echo "No way to run phpstan found."
+  exit 1
+fi
+
+
+
+# Basic setup
+cd "$REPO_DIR"
+STATUS=0
+
+
+
+
+### Run PHPStan on newly created files. ###
+
+PHPSTAN_CREATED_CONFIG=phpstan/phpstan.created.neon
+MAIN_BRANCH='5.x-dev'
+if [[ -f "$PHPSTAN_CREATED_CONFIG" ]]; then
+  CHANGED_FILES=$(git diff --name-only ${MAIN_BRANCH} --diff-filter=A | grep '\.php$' || true)
+  if [ -z "$CHANGED_FILES" ]; then
+    echo "No created PHP files"
+  else
+    echo "Running PHPstan at a very high level on new files"
+    CHANGED_FILES=`echo "$CHANGED_FILES" | sed -e 's/^\(.*\)$/"\1"/' | xargs -I{} echo "${PLUGIN_PATH}{}"`
+    echo "$CHANGED_FILES" | xargs $COMMAND analyse -c ${PLUGIN_PATH}${PHPSTAN_CREATED_CONFIG} || STATUS=1
+  fi
+fi
+
+
+
+### Run PHPStan on modified files. ###
+PHPSTAN_MODIFIED_CONFIG=phpstan/phpstan.modified.neon
+if [[ -f "$PHPSTAN_MODIFIED_CONFIG" ]]; then
+  CHANGED_FILES=$(git diff --name-only ${MAIN_BRANCH} --diff-filter=CM | grep '\.php$' || true)
+  if [ -z "$CHANGED_FILES" ]; then
+    echo "No changed PHP files"
+  else
+    echo "Running PHPstan on modified files"
+    CHANGED_FILES=`echo "$CHANGED_FILES" | sed -e 's/^\(.*\)$/"\1"/' | xargs -I{} echo "${PLUGIN_PATH}{}"`
+    echo "$CHANGED_FILES" | xargs $COMMAND analyse -c ${PLUGIN_PATH}${PHPSTAN_MODIFIED_CONFIG} || STATUS=1
+  fi
+fi
+
+# Don't bother running the full check, as we check changes files already, and
+# can assume that the unchanged files don't need rechecking.
+#
+# Github will check this anyway.
+#
+# PHPSTAN_BASE_CONFIG=phpstan.neon
+# if [[ -f "$PHPSTAN_BASE_CONFIG" ]]; then
+#     echo "Running PHPstan at a base level on all plugin files"
+#   $COMMAND analyse -c ${PLUGIN_PATH}/${PHPSTAN_BASE_CONFIG} || STATUS=1
+# fi
+
+exit $STATUS

.github/workflows/phpstan.yml

@@ -0,0 +1,83 @@
+name: PHPStan check
+
+on: pull_request
+
+permissions:
+  actions: read
+  checks: read
+  contents: read
+  deployments: none
+  issues: read
+  packages: none
+  pull-requests: read
+  repository-projects: none
+  security-events: none
+  statuses: read
+
+env:
+  PLUGIN_NAME: CustomAlerts
+
+jobs:
+  phpstan:
+    name: PHPStan
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v4
+        with:
+          lfs: false
+          persist-credentials: false
+      - name: Setup PHP
+        uses: shivammathur/setup-php@v2
+        with:
+          php-version: '7.2'
+
+      - name: Check out github-action-tests repository
+        uses: actions/checkout@v4
+        with:
+          repository: matomo-org/github-action-tests
+          ref: main
+          path: github-action-tests
+
+      - name: checkout matomo for plugin builds
+        shell: bash
+        run: ${{ github.workspace }}/github-action-tests/scripts/bash/checkout_matomo.sh
+        env:
+          PLUGIN_NAME: ${{ env.PLUGIN_NAME }}
+          WORKSPACE: ${{ github.workspace }}
+          ACTION_PATH: ${{ github.workspace }}/github-action-tests
+          MATOMO_TEST_TARGET: maximum_supported_matomo
+
+      - name: prepare setup
+        shell: bash
+        run: |
+          cd ${{ github.workspace }}/matomo
+          echo -e "composer install"
+          composer install --ignore-platform-reqs
+
+      - name: checkout additional plugins
+        if: ${{ env.DEPENDENT_PLUGINS != '' }}
+        shell: bash
+        working-directory: ${{ github.workspace }}/matomo
+        run: ${{ github.workspace }}/github-action-tests/scripts/bash/checkout_dependent_plugins.sh
+
+        env:
+          GITHUB_USER_TOKEN: ${{ secrets.TESTS_ACCESS_TOKEN || secrets.GITHUB_TOKEN }}
+
+      - name: "Restore result cache"
+        uses: actions/cache/restore@v4
+        with:
+          path: /tmp/phpstan # same as in phpstan.neon
+          key: "phpstan-result-cache-${{ github.run_id }}"
+          restore-keys: |
+            phpstan-result-cache-
+
+      - name: PHPStan whole repo
+        id: phpstan-all
+        run: cd ${{ github.workspace }}/matomo && composer run phpstan -- -vvv -c plugins/${{ env.PLUGIN_NAME }}/phpstan.neon
+
+      - name: "Save result cache"
+        uses: actions/cache/save@v4
+        if: ${{ !cancelled() }}
+        with:
+          path: /tmp/phpstan # same as in phpstan.neon
+          key: "phpstan-result-cache-${{ github.run_id }}"

API.php

@@ -271,7 +271,7 @@ public function deleteAlert($idAlert)
     /**
      * Get triggered alerts.
      *
-     * @param int[] idSites
+     * @param int[] $idSites
      *
      * @return array
      */

Controller.php

@@ -118,7 +118,7 @@ public function historyTriggeredAlerts()
 
         $idSites = $this->getSiteIdsHavingAccess();
         $alerts  = API::getInstance()->getTriggeredAlerts($idSites);
-        array_slice($alerts, 0, 100);
+        $alerts = array_slice($alerts, 0, 100);
         $alerts = array_reverse($alerts);
 
         $view->alertsFormatted = $this->formatAlerts($alerts, 'html_extended');
@@ -280,7 +280,7 @@ private function addBasicCreateAndEditVariables($view, $alert)
         $view->supportsSMS      = $this->supportsPlugin('MobileMessaging');
         $supportsSlack          = $this->supportsPlugin('Slack');
         $isSlackOAuthTokenAdded = false;
-        if ($supportsSlack) {
+        if ($supportsSlack && class_exists(\Piwik\Plugins\Slack\SystemSettings::class)) {
             $slackSettings = StaticContainer::get(\Piwik\Plugins\Slack\SystemSettings::class);
             $isSlackOAuthTokenAdded = !empty($slackSettings->slackOauthToken->getValue());
         }

CustomAlerts.php

@@ -41,6 +41,7 @@ public function registerEvents()
             'Request.dispatch'                       => 'checkControllerPermission',
             'Translate.getClientSideTranslationKeys' => 'getClientSideTranslationKeys',
             'UsersManager.deleteUser'                => 'deleteAlertsForLogin',
+            'UsersManager.removeSiteAccess'          => 'removeAlertsForUser',
             'SitesManager.deleteSite.end'            => 'deleteAlertsForSite',
             'Db.getTablesInstalled'                  => 'getTablesInstalled',
             'ScheduledTasks.execute'                 => 'startingScheduledTask',
@@ -295,4 +296,36 @@ public function validateReportParameters($parameters, $alertMedium)
             throw new \Exception(Piwik::translate('CustomAlerts_InvalidPhoneNumberReportParameter'));
         }
     }
+
+
+    /**
+     * Remove alerts associated with user
+     *
+     * If an alert is related to multiple sites that aren't in idSites, we
+     * won't delete the alert, just remove the alert_site link and triggers
+     *
+     * @param string $userLogin Username of user who had access removed
+     * @param array<string> $idSites List of website IDs
+     * @return void
+     */
+    public function removeAlertsForUser($userLogin, $idSites): void
+    {
+        if (empty($idSites) || empty($userLogin)) {
+            return;
+        }
+
+        $model = $this->getModel();
+        $alerts = $model->getAlerts($idSites, $userLogin);
+
+        foreach ($alerts as $alert) {
+            $alertId = $alert['idalert'];
+            $alertSites = $alert['id_sites'];
+            if (empty(array_diff($alertSites, $idSites))) {
+                $model->deleteAlert($alertId);
+            } else {
+                $model->deleteTriggeredAlertsForUserAndSites($alertId, $idSites, $userLogin);
+                $model->deleteAlertSitesForSites($alertId, $idSites);
+            }
+        }
+    }
 }

Model.php

@@ -112,7 +112,7 @@ public function getAlerts($idSites, $login = false)
 
     /**
      * @param $idSites
-     * @return array
+     * @return string
      */
     protected function getInnerSiteQuery($idSites)
     {
@@ -141,7 +141,7 @@ private function completeAlerts($alerts)
     private function getDefinedSiteIds($idAlert)
     {
         $sql   = "SELECT idsite FROM " . Common::prefixTable('alert_site') . " WHERE idalert = ?";
-        $sites = Db::fetchAll($sql, $idAlert, \PDO::FETCH_COLUMN);
+        $sites = Db::fetchAll($sql, $idAlert);
 
         $idSites = array();
         foreach ($sites as $site) {
@@ -453,4 +453,26 @@ public function deleteTriggeredAlertsForSite($idSite)
     {
         $this->getDb()->query("DELETE FROM " . Common::prefixTable("alert_triggered") . " WHERE idsite = ?", $idSite);
     }
+
+    public function deleteTriggeredAlertsForUserAndSites($idAlert, $idSites, $login)
+    {
+        $db = $this->getDb();
+        $placeholders = Common::getSqlStringFieldsArray($idSites);
+        $bind = array_merge(array($idAlert, $login), $idSites);
+        $db->query(
+            "DELETE FROM " . Common::prefixTable("alert_triggered") . " WHERE idalert = ? AND login = ? AND idsite in (" . $placeholders . " )",
+            $bind
+        );
+    }
+
+    public function deleteAlertSitesForSites($idAlert, $idSites)
+    {
+        $db = $this->getDb();
+        $placeholders = Common::getSqlStringFieldsArray($idSites);
+        $bind = array_merge(array($idAlert), $idSites);
+        $db->query(
+            "DELETE FROM " . Common::prefixTable("alert_site") . " WHERE idalert = ? AND idsite in (" . $placeholders . " )",
+            $bind
+        );
+    }
 }

phpstan.neon

@@ -0,0 +1,21 @@
+parameters:
+    level: 1
+    phpVersion: 70200
+    tmpDir: /tmp/phpstan/CustomAlerts/main
+    paths:
+        - .
+    excludePaths:
+        - tests/*
+        - github-action-tests
+    bootstrapFiles:
+        - ../../bootstrap-phpstan.php
+    universalObjectCratesClasses:
+        - Piwik\Config
+        - Piwik\View
+        - Piwik\ViewDataTable\Config
+    scanDirectories:
+        # ../../ does not actually seem to give us anything
+        # that ../plugins/ does not, but including it for
+        # completeness.  It does not seem to slow down performance.
+        - .
+

phpstan/phpstan.created.neon

@@ -0,0 +1,5 @@
+includes:
+    - ../phpstan.neon
+parameters:
+    level: 5
+    tmpDir: /tmp/phpstan/CustomAlerts/created

phpstan/phpstan.modified.neon

@@ -0,0 +1,5 @@
+includes:
+    - ../phpstan.neon
+parameters:
+    level: 5
+    tmpDir: /tmp/phpstan/CustomAlerts/modified

tests/Integration/CustomAlertsTest.php

@@ -200,6 +200,26 @@ public function test_deleteAlertsForLogin()
         $this->assertEquals('Initial6', $alerts[2]['name']);
     }
 
+    public function test_removeSiteAccessEventRemovesAlertsForSitesAndLogin()
+    {
+        $this->createAlert('DeleteMe', array(), array(1), 'userLogin');
+
+        $this->assertCount(1, $this->model->getAlerts(array(1), 'userLogin'));
+        Piwik::postEvent('UsersManager.removeSiteAccess', array('userLogin', array(1)));
+        $this->assertCount(0, $this->model->getAlerts(array(1), 'userLogin'));
+    }
+
+    public function test_removeSiteAccessEventRemovesSiteAndTriggersButKeepsAlert()
+    {
+        $alert = $this->createAlert('KeepMe', array(), array(1, 2), 'userLogin');
+        $this->model->triggerAlert($alert['idalert'], 1, 99, 48, Date::now()->getDatetime());
+
+        Piwik::postEvent('UsersManager.removeSiteAccess', array('userLogin', array(1)));
+
+        $this->assertNotNull($this->model->getAlert($alert['idalert']));
+        $this->assertCount(0, $this->model->getTriggeredAlerts(array(1), 'userLogin'));
+    }
+
     public function testStartingScheduledTask()
     {
         $this->checkOptionStringValue(true);