Refactor SQL queries: enforce parameterized bindings and consistent style

bundles/ApplicationLoggerBundle/src/Handler/ApplicationLoggerDb.php

@@ -59,9 +59,11 @@ public function write(LogRecord $record): void
      */
     public static function getComponents(): array
     {
-        $db = Db::get();
-
-        return $db->fetchFirstColumn('SELECT component FROM ' . self::TABLE_NAME . ' WHERE NOT ISNULL(component) GROUP BY component;');
+        return Db::get()->fetchFirstColumn(sprintf(
+            'SELECT component FROM %s WHERE component IS NOT NULL GROUP BY component',
+            self::TABLE_NAME
+            )
+        );
     }
 
     /**
@@ -81,9 +83,12 @@ public static function getPriorities(): array
             'emergency' => 'EMERG',
         ];
 
-        $db = Db::get();
+        $priorityNumbers = Db::get()->fetchFirstColumn(sprintf(
+            'SELECT priority FROM %s WHERE priority IS NOT NULL GROUP BY priority',
+            self::TABLE_NAME
+            )
+        );
 
-        $priorityNumbers = $db->fetchFirstColumn('SELECT priority FROM ' . self::TABLE_NAME . ' WHERE NOT ISNULL(priority) GROUP BY priority;');
         foreach ($priorityNumbers as $priorityNumber) {
             $priorities[$priorityNumber] = $priorityNames[$priorityNumber];
         }

bundles/ApplicationLoggerBundle/src/Maintenance/LogArchiveTask.php

@@ -41,70 +41,98 @@ public function __construct(
 
     public function execute(): void
     {
-        $db = $this->db;
         $storage = Storage::get('application_log');
 
         $date = new DateTime('now');
-        $tablename = ApplicationLoggerDb::TABLE_ARCHIVE_PREFIX.'_'.$date->format('Y').'_'.$date->format('m');
+        $archiveTable = sprintf('%s_%s', ApplicationLoggerDb::TABLE_ARCHIVE_PREFIX, $date->format('Y_m'));
 
         if (!empty($this->config['applicationlog']['archive_alternative_database'])) {
-            $tablename = $db->quoteIdentifier($this->config['applicationlog']['archive_alternative_database']).'.'.$tablename;
+            $archiveTable = sprintf(
+                '%s.%s',
+                $this->db->quoteIdentifier($this->config['applicationlog']['archive_alternative_database']),
+                $archiveTable
+            );
         }
 
-        $archive_threshold = (int) ($this->config['applicationlog']['archive_treshold'] ?? 30);
+        $archiveThreshold = (int) ($this->config['applicationlog']['archive_treshold'] ?? 30);
+        $sourceTable = ApplicationLoggerDb::TABLE_NAME;
+        $cutoff = (new DateTimeImmutable())->modify(sprintf('-%d days', $archiveThreshold))->format('Y-m-d H:i:s');
+        $whereParams = [$cutoff];
 
-        $timestamp = time();
-        $sql = 'SELECT %s FROM '.ApplicationLoggerDb::TABLE_NAME.' WHERE `timestamp` < DATE_SUB(FROM_UNIXTIME('.$timestamp.'), INTERVAL '.$archive_threshold.' DAY)';
-
-        if ($db->fetchOne(sprintf($sql, 'COUNT(*)')) > 0) {
-            $db->executeQuery('CREATE TABLE IF NOT EXISTS '.$tablename." (
-                       id BIGINT(20) NOT NULL,
-                       `pid` INT(11) NULL DEFAULT NULL,
-                       `timestamp` DATETIME NOT NULL,
-                       message VARCHAR(1024),
-                       `priority` ENUM('emergency','alert','critical','error','warning','notice','info','debug') DEFAULT NULL,
-                       fileobject VARCHAR(1024),
-                       info VARCHAR(1024),
-                       component VARCHAR(255),
-                       source VARCHAR(255) NULL DEFAULT NULL,
-                       relatedobject BIGINT(20),
-                       relatedobjecttype ENUM('object', 'document', 'asset'),
-                       maintenanceChecked TINYINT(1)
-                    ) ENGINE = ARCHIVE ROW_FORMAT = DEFAULT;");
-
-            $db->executeQuery('INSERT INTO '.$tablename.' '.sprintf($sql, '*'));
+        $count = $this->db->fetchOne(
+            sprintf('SELECT COUNT(*) FROM %s WHERE `timestamp` < ?', $sourceTable),
+            $whereParams
+        );
 
-            $this->logger->debug('Deleting referenced FileObjects of application_logs which are older than '.$archive_threshold.' days');
+        if ($count > 0) {
+            $this->db->executeStatement(sprintf(
+                "CREATE TABLE IF NOT EXISTS %s (
+                    id BIGINT(20) NOT NULL,
+                    `pid` INT(11) NULL DEFAULT NULL,
+                    `timestamp` DATETIME NOT NULL,
+                    message VARCHAR(1024),
+                    `priority` ENUM('emergency','alert','critical','error','warning','notice','info','debug') DEFAULT NULL,
+                    fileobject VARCHAR(1024),
+                    info VARCHAR(1024),
+                    component VARCHAR(255),
+                    source VARCHAR(255) NULL DEFAULT NULL,
+                    relatedobject BIGINT(20),
+                    relatedobjecttype ENUM('object', 'document', 'asset'),
+                    maintenanceChecked TINYINT(1)
+                ) ENGINE = ARCHIVE ROW_FORMAT = DEFAULT",
+                $archiveTable
+            ));
+
+            $this->db->executeStatement(
+                sprintf('INSERT INTO %s SELECT * FROM %s WHERE `timestamp` < ?', $archiveTable, $sourceTable),
+                $whereParams
+            );
+
+            $this->logger->debug(sprintf(
+                'Deleting referenced FileObjects of application_logs which are older than %d days',
+                $archiveThreshold
+            ));
+
+            $fileObjectPaths = $this->db->fetchAllAssociative(
+                sprintf('SELECT fileobject FROM %s WHERE `timestamp` < ?', $sourceTable),
+                $whereParams
+            );
 
-            $fileObjectPaths = $db->fetchAllAssociative(sprintf($sql, 'fileobject'));
             foreach ($fileObjectPaths as $objectPath) {
                 $filePath = $objectPath['fileobject'];
                 if ($filePath !== null && $storage->fileExists($filePath)) {
                     $storage->delete($filePath);
                 }
             }
 
-            $db->executeQuery('DELETE FROM '.ApplicationLoggerDb::TABLE_NAME.' WHERE `timestamp` < DATE_SUB(FROM_UNIXTIME('.$timestamp.'), INTERVAL '.$archive_threshold.' DAY);');
+            $this->db->executeStatement(
+                sprintf('DELETE FROM %s WHERE `timestamp` < ?', $sourceTable),
+                $whereParams
+            );
         }
 
-        $archiveTables = $db->fetchFirstColumn(
+        $archiveTables = $this->db->fetchFirstColumn(
             'SELECT table_name
                 FROM information_schema.tables
                 WHERE table_schema = ?
                 AND table_name LIKE ?',
             [
-                $this->config['applicationlog']['archive_alternative_database'] ?: $db->getDatabase(),
-                ApplicationLoggerDb::TABLE_ARCHIVE_PREFIX.'_%',
+                $this->config['applicationlog']['archive_alternative_database'] ?: $this->db->getDatabase(),
+                ApplicationLoggerDb::TABLE_ARCHIVE_PREFIX . '_%',
             ]
         );
-        foreach ($archiveTables as $archiveTable) {
-            if (preg_match('/^'.ApplicationLoggerDb::TABLE_ARCHIVE_PREFIX.'_(\d{4})_(\d{2})$/', $archiveTable, $matches)) {
-                $deleteArchiveLogDate = Carbon::createFromFormat('Y/m', $matches[1].'/'.$matches[2]);
-                if ($deleteArchiveLogDate->add(new DateInterval('P'.($this->config['applicationlog']['delete_archive_threshold'] ?? 6).'M')) < new DateTimeImmutable()) {
-                    $db->executeStatement('DROP TABLE IF EXISTS `'.($this->config['applicationlog']['archive_alternative_database'] ?: $db->getDatabase()).'`.'.$archiveTable);
 
-                    $folderName = $deleteArchiveLogDate->format('Y/m');
+        foreach ($archiveTables as $archiveTableName) {
+            if (preg_match('/^' . ApplicationLoggerDb::TABLE_ARCHIVE_PREFIX . '_(\d{4})_(\d{2})$/', $archiveTableName, $matches)) {
+                $deleteArchiveLogDate = Carbon::createFromFormat('Y/m', $matches[1] . '/' . $matches[2]);
+                if ($deleteArchiveLogDate->add(new DateInterval('P' . ($this->config['applicationlog']['delete_archive_threshold'] ?? 6) . 'M')) < new DateTimeImmutable()) {
+                    $this->db->executeStatement(sprintf(
+                        'DROP TABLE IF EXISTS %s.%s',
+                        $this->db->quoteIdentifier($this->config['applicationlog']['archive_alternative_database'] ?: $this->db->getDatabase()),
+                        $this->db->quoteIdentifier($archiveTableName)
+                    ));
 
+                    $folderName = $deleteArchiveLogDate->format('Y/m');
                     if ($storage->directoryExists($folderName)) {
                         $storage->deleteDirectory($folderName);
                     }

bundles/ApplicationLoggerBundle/src/Maintenance/LogMailMaintenanceTask.php

@@ -16,6 +16,7 @@
 
 namespace OpenDxp\Bundle\ApplicationLoggerBundle\Maintenance;
 
+use Doctrine\DBAL\ArrayParameterType;
 use Doctrine\DBAL\Connection;
 use OpenDxp\Bundle\ApplicationLoggerBundle\Handler\ApplicationLoggerDb;
 use OpenDxp\Config;
@@ -43,7 +44,7 @@ public function execute(): void
 
             // getting the enums from priority
             $priorityColumnDefinition = $this->db->fetchAllAssociative(
-                'SHOW COLUMNS FROM ' .ApplicationLoggerDb::TABLE_NAME. " LIKE 'priority'"
+                sprintf("SHOW COLUMNS FROM %s LIKE 'priority'", ApplicationLoggerDb::TABLE_NAME)
             );
 
             // type is the actual enum values
@@ -59,15 +60,14 @@ public function execute(): void
                 $logLevels[] = $enumValue[$i];
             }
 
-            $query = 'SELECT * FROM '
-                . ApplicationLoggerDb::TABLE_NAME
-                . ' WHERE maintenanceChecked IS NULL '
-                . 'AND priority IN('
-                . implode(',', $logLevels)
-                . ') '
-                . 'ORDER BY id DESC';
+            $rows = $this->db->fetchAllAssociative(sprintf(
+                    'SELECT * FROM %s WHERE maintenanceChecked IS NULL AND priority IN (?) ORDER BY id DESC',
+                    ApplicationLoggerDb::TABLE_NAME
+                ),
+                [$logLevels],
+                [ArrayParameterType::STRING]
+            );
 
-            $rows = $this->db->fetchAllAssociative($query);
             $limit = 100;
             $rowsProcessed = 0;
 
@@ -103,12 +103,9 @@ public function execute(): void
         // flag them as checked, regardless if email notifications are enabled or not
         // otherwise, when activating email notifications, you'll receive all log-messages from the past and not
         // since the point when you enabled the notifications
-        $this->db->executeQuery(
-            'UPDATE '
-            . ApplicationLoggerDb::TABLE_NAME
-            . ' SET maintenanceChecked = 1 '
-            . 'WHERE maintenanceChecked != 1 '
-            . 'OR maintenanceChecked IS NULL'
-        );
+        $this->db->executeStatement(sprintf(
+            'UPDATE %s SET maintenanceChecked = 1 WHERE maintenanceChecked != 1 OR maintenanceChecked IS NULL',
+            ApplicationLoggerDb::TABLE_NAME
+        ));
     }
 }

bundles/CoreBundle/src/Command/DeleteClassificationStoreCommand.php

@@ -16,7 +16,7 @@
 
 namespace OpenDxp\Bundle\CoreBundle\Command;
 
-use Exception;
+use InvalidArgumentException;
 use OpenDxp\Cache;
 use OpenDxp\Console\AbstractCommand;
 use OpenDxp\Db;
@@ -37,55 +37,48 @@ class DeleteClassificationStoreCommand extends AbstractCommand
 {
     protected function configure(): void
     {
-        $this
-            ->addArgument('storeId', InputArgument::REQUIRED, 'The store ID to delete')
-        ;
+        $this->addArgument('storeId', InputArgument::REQUIRED, 'The store ID to delete');
     }
 
     protected function execute(InputInterface $input, OutputInterface $output): int
     {
         $storeId = $input->getArgument('storeId');
 
         if (!is_numeric($storeId)) {
-            throw new Exception('Invalid store ID');
+            throw new InvalidArgumentException('Invalid store ID');
         }
 
+        $storeId = (int) $storeId;
         $db = Db::get();
 
-        $tableList = $db->fetchAllAssociative("show tables like 'object_classificationstore_data_%'");
+        $tableList = $db->fetchAllAssociative('SHOW TABLES LIKE "object_classificationstore_data_%"');
         foreach ($tableList as $table) {
             $theTable = current($table);
-            $sql = 'delete from ' . $theTable . ' where keyId In (select id from classificationstore_keys where storeId = ' . $db->quote($storeId) . ')';
-            echo $sql . "\n";
-            $db->executeQuery($sql);
+            $output->writeln(sprintf('Deleting classification store data from <info>%s</info>', $theTable));
+            $db->executeStatement(
+                sprintf('DELETE FROM %s WHERE keyId IN (SELECT id FROM classificationstore_keys WHERE storeId = ?)', $theTable),
+                [$storeId]
+            );
         }
 
-        $tableList = $db->fetchAllAssociative("show tables like 'object_classificationstore_groups_%'");
+        $tableList = $db->fetchAllAssociative('SHOW TABLES LIKE "object_classificationstore_groups_%"');
         foreach ($tableList as $table) {
             $theTable = current($table);
-            $sql = 'delete from ' . $theTable . ' where groupId In (select id from classificationstore_groups where storeId = ' . $db->quote($storeId) . ')';
-            echo $sql . "\n";
-            $db->executeQuery($sql);
+            $output->writeln(sprintf('Deleting classification store groups from <info>%s</info>', $theTable));
+            $db->executeStatement(
+                sprintf('DELETE FROM %s WHERE groupId IN (SELECT id FROM classificationstore_groups WHERE storeId = ?)', $theTable),
+                [$storeId]
+            );
         }
 
-        $sql = 'delete from classificationstore_keys where storeId = ' . $db->quote($storeId);
-        echo $sql . "\n";
-        $db->executeQuery($sql);
-
-        $sql = 'delete from classificationstore_groups where storeId = ' . $db->quote($storeId);
-        echo $sql . "\n";
-        $db->executeQuery($sql);
-
-        $sql = 'delete from classificationstore_collections where storeId = ' . $db->quote($storeId);
-        echo $sql . "\n";
-        $db->executeQuery($sql);
-
-        $sql = 'delete from classificationstore_stores where id = ' . $db->quote($storeId);
-        echo $sql . "\n";
-        $db->executeQuery($sql);
+        $output->writeln('Deleting keys, groups, collections and store record');
+        $db->executeStatement('DELETE FROM classificationstore_keys WHERE storeId = ?', [$storeId]);
+        $db->executeStatement('DELETE FROM classificationstore_groups WHERE storeId = ?', [$storeId]);
+        $db->executeStatement('DELETE FROM classificationstore_collections WHERE storeId = ?', [$storeId]);
+        $db->executeStatement('DELETE FROM classificationstore_stores WHERE id = ?', [$storeId]);
 
         Cache::clearAll();
 
-        return 0;
+        return self::SUCCESS;
     }
-}
+}

bundles/CoreBundle/src/Command/DeleteUnusedLocaleDataCommand.php

@@ -16,6 +16,7 @@
 
 namespace OpenDxp\Bundle\CoreBundle\Command;
 
+use Doctrine\DBAL\ArrayParameterType;
 use OpenDxp\Console\AbstractCommand;
 use OpenDxp\Console\Traits\DryRun;
 use OpenDxp\Db;
@@ -59,11 +60,7 @@ protected function execute(InputInterface $input, OutputInterface $output): int
             $skipLocales = explode(',', $input->getOption('skip-locales'));
         }
 
-        $languageList = [];
         $validLanguages = Tool::getValidLanguages();
-        foreach ($validLanguages as $language) {
-            $languageList[] = $db->quote($language);
-        }
 
         $tables = $db->fetchAllAssociative("SHOW TABLES LIKE 'object\_localized\_data\_%'");
 
@@ -72,20 +69,24 @@ protected function execute(InputInterface $input, OutputInterface $output): int
             $table = current($table);
             $classId = str_replace('object_localized_data_', '', $table);
 
-            $result = $db->fetchAllAssociative('SELECT DISTINCT `language` FROM ' . $table . ' WHERE `language` NOT IN(' . implode(',', $languageList) .')');
+            $result = $db->fetchAllAssociative(
+                sprintf('SELECT DISTINCT `language` FROM %s WHERE `language` NOT IN(?)', $table),
+                [$validLanguages],
+                [ArrayParameterType::STRING]
+            );
             $result = ($result ?: []);
 
             //delete data from object_localized_data_classID tables
             foreach ($result as $res) {
                 $language = $res['language'];
                 if (!ArrayHelper::inArrayCaseInsensitive($language, $skipLocales) && !ArrayHelper::inArrayCaseInsensitive($language, $validLanguages)) {
-                    $sqlDeleteData = 'Delete FROM object_localized_data_' . $classId  . ' WHERE `language` = ' . $db->quote($language);
                     $printLine = true;
+                    $deleteStmt = sprintf('DELETE FROM object_localized_data_%s WHERE `language` = ?', $classId);
                     if (!$this->isDryRun()) {
-                        $output->writeln($sqlDeleteData);
-                        $db->executeQuery($sqlDeleteData);
+                        $output->writeln(sprintf('DELETE FROM object_localized_data_%s WHERE `language` = %s', $classId, $db->quote($language)));
+                        $db->executeStatement($deleteStmt, [$language]);
                     } else {
-                        $output->writeln($this->dryRunMessage($sqlDeleteData));
+                        $output->writeln($this->dryRunMessage(sprintf('DELETE FROM object_localized_data_%s WHERE `language` = %s', $classId, $db->quote($language))));
                     }
                 }
             }
@@ -97,7 +98,7 @@ protected function execute(InputInterface $input, OutputInterface $output): int
                 $existingLanguage = str_replace('object_localized_'.$classId.'_', '', $localizedView);
 
                 if (!ArrayHelper::inArrayCaseInsensitive($existingLanguage, $validLanguages)) {
-                    $sqlDropView = 'DROP VIEW IF EXISTS object_localized_' . $classId . '_' .$existingLanguage;
+                    $sqlDropView = sprintf('DROP VIEW IF EXISTS object_localized_%s_%s', $classId, $existingLanguage);
                     $printLine = true;
 
                     if (!$this->isDryRun()) {
@@ -116,7 +117,7 @@ protected function execute(InputInterface $input, OutputInterface $output): int
                 $existingLanguage = str_replace('object_localized_query_'.$classId.'_', '', $localizedTable);
 
                 if (!ArrayHelper::inArrayCaseInsensitive($existingLanguage, $validLanguages)) {
-                    $sqlDropTable = 'DROP TABLE IF EXISTS object_localized_query_' . $classId . '_' .$existingLanguage;
+                    $sqlDropTable = sprintf('DROP TABLE IF EXISTS object_localized_query_%s_%s', $classId, $existingLanguage);
                     $printLine = true;
 
                     if (!$this->isDryRun()) {

bundles/CoreBundle/src/Command/Document/CleanupCommand.php

@@ -89,7 +89,7 @@ protected function execute(InputInterface $input, OutputInterface $output): int
                 $tableName = 'documents_' . $filteredDocumentType;
 
                 try {
-                    $db->executeQuery('DROP TABLE IF EXISTS ' . $tableName);
+                    $db->executeQuery(sprintf('DROP TABLE IF EXISTS %s', $tableName));
                 } catch (Exception $ex) {
                     $output->writeln(sprintf('Could not drop table %s: %s', $tableName, $ex));
                 }