Address matomo-review medium findings for blob-row cap

- Bump version to 5.10.0-b1
- Centralise the MEDIUMBLOB-list + flag-clear logic in
  ArchiveBlobColumnType::recheckAndUpdateFlag() and have the CLI command
  delegate to it, removing the duplicated write path.
- Make ArchiveBlobColumnType::isMediumBlob() return true on empty/missing
  INFORMATION_SCHEMA results, matching the documented fail-safe contract.
- Add ArchiveBlobColumnType::CONFIG_KEY constant and use it everywhere the
  flag is read or written, preventing silent key drift.
- Narrow the archive_blob table discovery LIKE pattern to the configured
  table prefix.
- Use DatabaseConfig instead of Config class

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Author: sgiehl

core/ArchiveProcessor/ArchiveBlobRowCap.php

@@ -9,7 +9,7 @@
 
 namespace Piwik\ArchiveProcessor;
 
-use Piwik\Config;
+use Piwik\Config\DatabaseConfig;
 use Piwik\DataAccess\ArchiveBlobColumnType;
 
 /**
@@ -54,7 +54,7 @@ final class ArchiveBlobRowCap
      */
     public static function isCapPossiblyNeeded(): bool
     {
-        return (int) (Config::getInstance()->database['archive_blob_tables_may_contain_mediumblob'] ?? 0) !== 0;
+        return (int)(DatabaseConfig::getConfigValue(ArchiveBlobColumnType::CONFIG_KEY) ?? 0) !== 0;
     }
 
     /**
@@ -91,7 +91,7 @@ public static function capMaxSubtableRows(?int $configuredMax, string $tableName
     private static function applyCap(?int $configuredMax, string $tableName): ?int
     {
         // Fast-path: skip all I/O when the flag is not set (fresh installs).
-        $flag = (int) (Config::getInstance()->database['archive_blob_tables_may_contain_mediumblob'] ?? 0);
+        $flag = (int)(DatabaseConfig::getConfigValue(ArchiveBlobColumnType::CONFIG_KEY) ?? 0);
         if ($flag === 0) {
             return $configuredMax;
         }

core/DataAccess/ArchiveBlobColumnType.php

@@ -9,6 +9,7 @@
 
 namespace Piwik\DataAccess;
 
+use Piwik\Common;
 use Piwik\Config;
 use Piwik\Container\StaticContainer;
 use Piwik\Db;
@@ -27,6 +28,12 @@
  */
 final class ArchiveBlobColumnType
 {
+    /**
+     * The [database] config key used to signal that archive_blob tables may still contain
+     * MEDIUMBLOB `value` columns (set by the 5.10.0-b1 migration on existing installs).
+     */
+    public const CONFIG_KEY = 'archive_blob_tables_may_contain_mediumblob';
+
     /**
      * Per-request cache: table name → isMediumBlob result.
      *
@@ -54,9 +61,21 @@ public static function isMediumBlob(string $tableName): bool
                 [$tableName, 'value']
             );
 
-            // MySQL returns the type in lowercase ('mediumblob', 'longblob'); use
-            // case-insensitive comparison so we are not surprised by unusual drivers.
-            $result = (strtolower((string) $columnType) === 'mediumblob');
+            $normalized = strtolower((string) $columnType);
+            if ($normalized === '') {
+                // INFORMATION_SCHEMA returned no row: the table or column is missing.
+                // This is unexpected (callers create the table before calling us) and
+                // indicates a race condition or schema corruption.  Apply the cap conservatively.
+                StaticContainer::get(LoggerInterface::class)->warning(
+                    'ArchiveBlobColumnType: INFORMATION_SCHEMA returned no row for table {table}; applying cap conservatively.',
+                    ['table' => $tableName]
+                );
+                $result = true;
+            } else {
+                // MySQL returns the type in lowercase ('mediumblob', 'longblob'); use
+                // case-insensitive comparison so we are not surprised by unusual drivers.
+                $result = ($normalized === 'mediumblob');
+            }
         } catch (\Exception $e) {
             StaticContainer::get(LoggerInterface::class)->warning(
                 'ArchiveBlobColumnType: could not determine column type for table {table}: {exception}',
@@ -81,7 +100,8 @@ public static function clearCache(): void
     /**
      * Checks whether any archive_blob_* tables in the current schema still use MEDIUMBLOB for
      * their `value` column.  If none remain, the
-     * `[database] archive_blob_tables_may_contain_mediumblob` config flag is removed.
+     * `[database] archive_blob_tables_may_contain_mediumblob` ({@see CONFIG_KEY}) config flag is
+     * removed.
      *
      * If the flag is not set (0 / unset) this method returns immediately without any I/O so that
      * fresh installs pay zero runtime cost during updates.
@@ -90,47 +110,57 @@ public static function clearCache(): void
      *  - `core/Updater.php` after each component update finishes and at the end of a full batch.
      *  - `plugins/CoreUpdater/Commands/RecheckArchiveBlobTypes.php` as an on-demand CLI tool.
      *
-     * @return bool  `true` when MEDIUMBLOB tables were found (flag left as-is or not set),
-     *               `false` when flag was unset because no MEDIUMBLOB tables remain.
+     * @return string[]  Names of archive_blob tables that still use MEDIUMBLOB.
+     *                   An empty array means no MEDIUMBLOB tables remain and the flag was cleared.
+     *                   A non-empty array means the flag was left as-is.
+     *                   Returns an empty array (without I/O) when the flag was not set.
      */
-    public static function recheckAndUpdateFlag(): bool
+    public static function recheckAndUpdateFlag(): array
     {
-        $flag = (int) (Config::getInstance()->database['archive_blob_tables_may_contain_mediumblob'] ?? 0);
+        $flag = (int) (Config::getInstance()->database[self::CONFIG_KEY] ?? 0);
         if ($flag === 0) {
             // Nothing to do on fresh installs or after the flag was already cleared.
-            return false;
+            return [];
         }
 
         $mediumBlobTables = self::getMediumBlobArchiveTables();
         if (empty($mediumBlobTables)) {
             // All tables have been migrated (or none existed). Remove the flag.
             $config = Config::getInstance();
             $database = $config->database;
-            unset($database['archive_blob_tables_may_contain_mediumblob']);
+            unset($database[self::CONFIG_KEY]);
             $config->database = $database;
             $config->forceSave();
-            return false;
         }
 
-        return true;
+        return $mediumBlobTables;
     }
 
     /**
      * Returns the names of all archive_blob_* tables in the current schema whose `value` column
-     * is MEDIUMBLOB.
+     * is MEDIUMBLOB.  Only tables whose name begins with the configured Matomo table prefix are
+     * inspected, so tables belonging to other Matomo instances (or unrelated tables that happen to
+     * contain "archive_blob_" in their name) are never returned.
      *
      * @return string[]
      */
     public static function getMediumBlobArchiveTables(): array
     {
+        // Build a prefix-anchored LIKE pattern, e.g. "matomo_archive\_blob\_%".
+        // LIKE-escape any '%' or '_' in the prefix itself so a weird table-prefix cannot
+        // accidentally broaden the match.
+        $rawPrefix = Common::prefixTable('archive_blob_');
+        $likePrefix = str_replace(['\\', '%', '_'], ['\\\\', '\\%', '\\_'], $rawPrefix);
+        $likePattern = $likePrefix . '%';
+
         try {
             $rows = Db::fetchAll(
                 "SELECT TABLE_NAME FROM INFORMATION_SCHEMA.COLUMNS
                   WHERE TABLE_SCHEMA = DATABASE()
                     AND TABLE_NAME   LIKE ?
                     AND COLUMN_NAME  = ?
                     AND LOWER(COLUMN_TYPE) = ?",
-                ['%archive\_blob\_%', 'value', 'mediumblob']
+                [$likePattern, 'value', 'mediumblob']
             );
         } catch (\Exception $e) {
             StaticContainer::get(LoggerInterface::class)->warning(

core/Updates/5.10.0-b1.php core/Updates/5.11.0-b2.phpcore/Updates/5.10.0-b1.php renamed to core/Updates/5.11.0-b2.php

@@ -21,7 +21,7 @@
  * Fresh installs (all tables are LONGBLOB) receive no migration so the fast-path short-circuit
  * in {@see \Piwik\ArchiveProcessor\ArchiveBlobRowCap} costs nothing at runtime.
  */
-class Updates_5_10_0_b1 extends Updates
+class Updates_5_11_0_b2 extends Updates
 {
     /**
      * @var MigrationFactory

core/Version.php

@@ -22,7 +22,7 @@ final class Version
      * The current Matomo version.
      * @var string
      */
-    public const VERSION = '5.11.0-b1';
+    public const VERSION = '5.11.0-b2';
 
     public const MAJOR_VERSION = 5;
 

plugins/CoreUpdater/Commands/RecheckArchiveBlobTypes.php

@@ -38,27 +38,22 @@ protected function doExecute(): int
     {
         $output = $this->getOutput();
 
-        $flag = (int) (Config::getInstance()->database['archive_blob_tables_may_contain_mediumblob'] ?? 0);
+        // Short-circuit when the flag is not set (fresh installs, or already cleared).
+        $flag = (int) (Config::getInstance()->database[ArchiveBlobColumnType::CONFIG_KEY] ?? 0);
         if ($flag === 0) {
             $output->writeln(
                 'No MEDIUMBLOB archive_blob tables possible (flag not set); nothing to do.'
             );
             return self::SUCCESS;
         }
 
-        $mediumBlobTables = ArchiveBlobColumnType::getMediumBlobArchiveTables();
+        // Delegate all detection and flag-clearing logic to the single canonical implementation.
+        $mediumBlobTables = ArchiveBlobColumnType::recheckAndUpdateFlag();
 
         if (empty($mediumBlobTables)) {
-            // All tables have been migrated. Remove the flag.
-            $config = Config::getInstance();
-            $database = $config->database;
-            unset($database['archive_blob_tables_may_contain_mediumblob']);
-            $config->database = $database;
-            $config->forceSave();
-
             $output->writeln(
                 'No MEDIUMBLOB archive_blob tables found. ' .
-                'The archive_blob_tables_may_contain_mediumblob flag has been removed from config.ini.php. ' .
+                'The ' . ArchiveBlobColumnType::CONFIG_KEY . ' flag has been removed from config.ini.php. ' .
                 'Archive row-limit cap will no longer be applied.'
             );
         } else {
@@ -70,7 +65,7 @@ protected function doExecute(): int
                 $output->writeln('  - ' . $table);
             }
             $output->writeln(
-                'The archive_blob_tables_may_contain_mediumblob flag remains set. ' .
+                'The ' . ArchiveBlobColumnType::CONFIG_KEY . ' flag remains set. ' .
                 'To migrate, run ALTER TABLE on the listed tables to change the `value` column to LONGBLOB, ' .
                 'then re-run this command.'
             );

tests/PHPUnit/Integration/DataAccess/ArchiveBlobColumnTypeTest.php

@@ -21,8 +21,10 @@
  */
 class ArchiveBlobColumnTypeTest extends IntegrationTestCase
 {
-    private const TEST_TABLE_MEDIUM = 'archive_blob_test_medium';
-    private const TEST_TABLE_LONG   = 'archive_blob_test_long';
+    private const TEST_TABLE_MEDIUM            = 'archive_blob_test_medium';
+    private const TEST_TABLE_LONG             = 'archive_blob_test_long';
+    /** Raw (un-prefixed) name for a table that belongs to a different Matomo instance. */
+    private const TEST_TABLE_FOREIGN_MEDIUM   = 'other_archive_blob_foreign_medium';
 
     public function setUp(): void
     {
@@ -64,11 +66,10 @@ public function testIsMediumBlobReturnsTrueForNonExistentTableFailSafe(): void
     {
         // A table that does not exist → INFORMATION_SCHEMA returns nothing → fail-safe returns true.
         $tableName = Common::prefixTable('archive_blob_9999_99');
-        // Result is false because INFORMATION_SCHEMA returns null/empty string, not 'mediumblob'.
-        // The fail-safe only triggers on exceptions; an empty result means the table is not
-        // MEDIUMBLOB (it doesn't exist at all). Confirm no exception is thrown.
+        // An empty/null result from INFORMATION_SCHEMA is treated as a fail-safe condition:
+        // the cap is applied conservatively rather than risking a silent 16 MB truncation.
         $result = ArchiveBlobColumnType::isMediumBlob($tableName);
-        self::assertFalse($result);
+        self::assertTrue($result);
     }
 
     // -----------------------------------------------------------------------
@@ -133,13 +134,37 @@ public function testGetMediumBlobArchiveTablesReturnsEmptyWhenNoneMediumBlob():
         self::assertNotContains($prefixedLong, $tables);
     }
 
+    public function testGetMediumBlobArchiveTablesIgnoresTablesWithDifferentPrefix(): void
+    {
+        // Create a MEDIUMBLOB table whose raw name contains "archive_blob_" but does NOT carry the
+        // Matomo table prefix (simulates a table from a different Matomo instance sharing the same
+        // DB schema).
+        $this->createRawTable(self::TEST_TABLE_FOREIGN_MEDIUM, 'MEDIUMBLOB');
+
+        $tables = ArchiveBlobColumnType::getMediumBlobArchiveTables();
+
+        self::assertNotContains(
+            self::TEST_TABLE_FOREIGN_MEDIUM,
+            $tables,
+            'getMediumBlobArchiveTables() must not return tables from a different table prefix'
+        );
+    }
+
     // -----------------------------------------------------------------------
     // Helpers
     // -----------------------------------------------------------------------
 
     private function createTestTable(string $tableBaseName, string $blobType): void
     {
         $tableName = Common::prefixTable($tableBaseName);
+        $this->createRawTable($tableName, $blobType);
+    }
+
+    /**
+     * Creates a table using the exact (already-resolved) name passed — no prefix is added.
+     */
+    private function createRawTable(string $tableName, string $blobType): void
+    {
         Db::exec(sprintf(
             'CREATE TABLE IF NOT EXISTS `%s` (
                 `idarchive`  INT UNSIGNED     NOT NULL,
@@ -166,5 +191,12 @@ private function dropTestTables(): void
                 // Ignore errors during cleanup.
             }
         }
+
+        // Drop the foreign table (stored under its raw name, not prefixed).
+        try {
+            Db::exec('DROP TABLE IF EXISTS `' . self::TEST_TABLE_FOREIGN_MEDIUM . '`');
+        } catch (\Exception $e) {
+            // Ignore errors during cleanup.
+        }
     }
 }

…ntegration/Updates/Updates5100b1Test.php …ntegration/Updates/Updates5110b2Test.phptests/PHPUnit/Integration/Updates/Updates5100b1Test.php renamed to tests/PHPUnit/Integration/Updates/Updates5110b2Test.php tests/PHPUnit/Integration/Updates/Updates5100b1Test.php renamed to tests/PHPUnit/Integration/Updates/Updates5110b2Test.php

@@ -18,18 +18,18 @@
 use Piwik\Updater\Migration\Db\Factory as DbFactory;
 use Piwik\Updater\Migration\Factory as MigrationFactory;
 use Piwik\Updater\Migration\Plugin\Factory as PluginFactory;
-use Piwik\Updates\Updates_5_10_0_b1;
+use Piwik\Updates\Updates_5_11_0_b2;
 
-require_once PIWIK_INCLUDE_PATH . '/core/Updates/5.10.0-b1.php';
+require_once PIWIK_INCLUDE_PATH . '/core/Updates/5.11.0-b2.php';
 
 /**
- * @group Updates5100b1Test
+ * @group Updates5110b2Test
  * @group Core
  */
-class Updates5100b1Test extends IntegrationTestCase
+class Updates5110B2Test extends IntegrationTestCase
 {
-    private const TEST_TABLE_MEDIUM = 'archive_blob_test_5100_medium';
-    private const TEST_TABLE_LONG   = 'archive_blob_test_5100_long';
+    private const TEST_TABLE_MEDIUM = 'archive_blob_test_5110_medium';
+    private const TEST_TABLE_LONG = 'archive_blob_test_5110_long';
 
     public function setUp(): void
     {
@@ -78,14 +78,14 @@ public function testGetMigrationsReturnsEmptyWhenNoArchiveBlobTablesExist(): voi
     // Helpers
     // -----------------------------------------------------------------------
 
-    private function buildUpdate(): Updates_5_10_0_b1
+    private function buildUpdate(): Updates_5_11_0_b2
     {
         $migrationFactory = new MigrationFactory(
             $this->getMockBuilder(DbFactory::class)->disableOriginalConstructor()->getMock(),
             $this->getMockBuilder(PluginFactory::class)->disableOriginalConstructor()->getMock(),
             new ConfigFactory()
         );
-        return new Updates_5_10_0_b1($migrationFactory);
+        return new Updates_5_11_0_b2($migrationFactory);
     }
 
     private function createTestTable(string $tableBaseName, string $blobType): void

tests/PHPUnit/Unit/ArchiveProcessor/ArchiveBlobRowCapTest.php

@@ -177,6 +177,7 @@ public function testCapMaxRowsDBErrorReturnsCap(): void
     private function setFlag(int $value): void
     {
         $config = Config::getInstance();
+        /** @var array<string, scalar> $database */
         $database = $config->database;
         if ($value === 0) {
             unset($database['archive_blob_tables_may_contain_mediumblob']);