[BUGFIX] Use sys_history for workspaces stage changes + comments

Historically, a stage change (publishing or "editing => ready to publish") has been tracked - along with the comment in sys_log with the action=6 (update) and details_nr=30, where then the logs are read again for the workspaces module. Reading from sys_log is a problem as this data is very volatile (there is even a scheduler task to delete old sys_log entries older than e.g. 90 days, so we loose comments). This change now stores the information inside the sys_history table about a stage change, so the history of such an action is preserved, along with the comment. The workspaces module now reads from sys_history (via RecordHistory class) and from sys_log if there are still old comments. The reading from sys_log can then be removed at a later change. Resolves: #97158 Releases: main, 11.5 Change-Id: I055b944462f57a889681a6a236c329af29bb8d80 Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/73878 Tested-by: core-ci <typo3@b13.com> Tested-by: Benni Mack <benni@typo3.org> Reviewed-by: Benni Mack <benni@typo3.org>
TYPO3 · Mar 14, 2022 · fde62fd · fde62fd
1 parent b1cb4f2
commit fde62fd
Show file tree

Hide file tree

Showing 6 changed files with 129 additions and 37 deletions.
diff --git a/Build/Sources/TypeScript/workspaces/Resources/Public/TypeScript/Backend.ts b/Build/Sources/TypeScript/workspaces/Resources/Public/TypeScript/Backend.ts
@@ -117,7 +117,7 @@ class Backend extends Workspaces {
 
       $panel.append(
         $('<div />', {class: 'panel-footer'}).append(
-          $('<span />', {class: 'label label-success'}).text(comment.stage_title),
+          $('<span />', {class: 'label label-success me-2'}).text(comment.previous_stage_title + ' > ' + comment.stage_title),
           $('<span />', {class: 'label label-info'}).text(comment.tstamp),
         ),
       );

diff --git a/typo3/sysext/core/Classes/DataHandling/History/RecordHistoryStore.php b/typo3/sysext/core/Classes/DataHandling/History/RecordHistoryStore.php
@@ -34,6 +34,7 @@ class RecordHistoryStore
     public const ACTION_MOVE = 3;
     public const ACTION_DELETE = 4;
     public const ACTION_UNDELETE = 5;
+    public const ACTION_STAGECHANGE = 6;
 
     public const USER_BACKEND = 'BE';
     public const USER_FRONTEND = 'FE';
@@ -67,8 +68,8 @@ class RecordHistoryStore
     /**
      * @param string $userType
      * @param int|null $userId
-     * @param int $originalUserId
-     * @param int $tstamp
+     * @param int|null $originalUserId
+     * @param int|null $tstamp
      * @param int $workspaceId
      */
     public function __construct(string $userType = self::USER_BACKEND, int $userId = null, int $originalUserId = null, int $tstamp = null, int $workspaceId = 0)
@@ -201,6 +202,24 @@ public function moveRecord(string $table, int $uid, array $payload, CorrelationI
         return $this->getDatabaseConnection()->lastInsertId('sys_history');
     }
 
+    public function changeStageForRecord(string $table, int $uid, array $payload, CorrelationId $correlationId = null): string
+    {
+        $data = [
+            'actiontype' => self::ACTION_STAGECHANGE,
+            'usertype' => $this->userType,
+            'userid' => $this->userId,
+            'originaluserid' => $this->originalUserId,
+            'tablename' => $table,
+            'recuid' => $uid,
+            'tstamp' => $this->tstamp,
+            'history_data' => json_encode($payload),
+            'workspace' => $this->workspaceId,
+            'correlation_id' => (string)$this->createCorrelationId($table, $uid, $correlationId),
+        ];
+        $this->getDatabaseConnection()->insert('sys_history', $data);
+        return $this->getDatabaseConnection()->lastInsertId('sys_history');
+    }
+
     protected function createCorrelationId(string $tableName, int $uid, ?CorrelationId $correlationId): CorrelationId
     {
         if ($correlationId !== null && $correlationId->getSubject() !== null) {

diff --git a/typo3/sysext/workspaces/Classes/Controller/Remote/RemoteServer.php b/typo3/sysext/workspaces/Classes/Controller/Remote/RemoteServer.php
@@ -278,10 +278,12 @@ public function getRowDetails($parameter)
                 $hookObject->modifyDifferenceArray($parameter, $diffReturnArray, $liveReturnArray, $diffUtility);
             }
         }
-        $commentsForRecord = $this->getCommentsForRecord($parameter->uid, $parameter->table);
 
         $historyService = GeneralUtility::makeInstance(HistoryService::class);
         $history = $historyService->getHistory($parameter->table, $parameter->t3ver_oid);
+        $stageChanges = $historyService->getStageChanges($parameter->table, (int)$parameter->t3ver_oid);
+        $stageChangesFromSysLog = $this->getStageChangesFromSysLog($parameter->table, (int)$parameter->t3ver_oid);
+        $commentsForRecord = $this->getCommentsForRecord($stageChanges, $stageChangesFromSysLog);
 
         if ($this->stagesService->isPrevStageAllowedForUser($parameter->stage)) {
             $prevStage = $this->stagesService->getPrevStage($parameter->stage);
@@ -396,18 +398,59 @@ protected function prepareFileReferenceDifferences(array $liveFileReferences, ar
     }
 
     /**
-     * Gets an array with all sys_log entries and their comments for the given record uid and table
+     * Prepares all comments of the stage change history entries for returning the JSON structure
      *
-     * @param int $uid uid of changed element to search for in log
-     * @param string $table Name of the record's table
+     * @param array $historyEntries
+     * @param array $additionalChangesFromLog this is not in use since 2022 anymore, and can be removed in TYPO3 v13.0 the latest.
      * @return array
      */
-    public function getCommentsForRecord($uid, $table)
+    protected function getCommentsForRecord(array $historyEntries, array $additionalChangesFromLog): array
+    {
+        $allStageChanges = [];
+        /** @var Avatar $avatar */
+        $avatar = GeneralUtility::makeInstance(Avatar::class);
+
+        foreach ($historyEntries as $entry) {
+            $preparedEntry = [];
+            $beUserRecord = BackendUtility::getRecord('be_users', $entry['userid']);
+            $preparedEntry['stage_title'] = htmlspecialchars($this->stagesService->getStageTitle($entry['history_data']['next']));
+            $preparedEntry['previous_stage_title'] = htmlspecialchars($this->stagesService->getStageTitle($entry['history_data']['current']));
+            $preparedEntry['user_uid'] = (int)$entry['userid'];
+            $preparedEntry['user_username'] = is_array($beUserRecord) ? htmlspecialchars($beUserRecord['username']) : '';
+            $preparedEntry['tstamp'] = htmlspecialchars(BackendUtility::datetime($entry['tstamp']));
+            $preparedEntry['user_comment'] = nl2br(htmlspecialchars($entry['history_data']['comment']));
+            $preparedEntry['user_avatar'] = $beUserRecord ? $avatar->render($beUserRecord) : '';
+            $allStageChanges[] = $preparedEntry;
+        }
+
+        // see if there are more
+        foreach ($additionalChangesFromLog as $sysLogRow) {
+            $sysLogEntry = [];
+            $data = $this->unserializeLogData($sysLogRow['log_data'] ?? '');
+            $beUserRecord = BackendUtility::getRecord('be_users', $sysLogRow['userid']);
+            $sysLogEntry['stage_title'] = htmlspecialchars($this->stagesService->getStageTitle($data['stage']));
+            $sysLogEntry['previous_stage_title'] = '';
+            $sysLogEntry['user_uid'] = (int)$sysLogRow['userid'];
+            $sysLogEntry['user_username'] = is_array($beUserRecord) ? htmlspecialchars($beUserRecord['username']) : '';
+            $sysLogEntry['tstamp'] = htmlspecialchars(BackendUtility::datetime($sysLogRow['tstamp']));
+            $sysLogEntry['user_comment'] = nl2br(htmlspecialchars($data['comment']));
+            $sysLogEntry['user_avatar'] = $avatar->render($beUserRecord);
+            $allStageChanges[] = $sysLogEntry;
+        }
+
+        // There might be "old" sys_log entries, so they need to be checked as well
+        return $allStageChanges;
+    }
+
+    /**
+     * Find all stage changes from sys_log that do not have a historyId. Can safely be removed in future TYPO3
+     * versions as this fallback layer only makes sense in TYPO3 v11 when old records want to have a history.
+     */
+    protected function getStageChangesFromSysLog(string $table, int $uid): array
     {
-        $sysLogReturnArray = [];
         $queryBuilder = GeneralUtility::makeInstance(ConnectionPool::class)->getQueryBuilderForTable('sys_log');
 
-        $result = $queryBuilder
+        return $queryBuilder
             ->select('log_data', 'tstamp', 'userid')
             ->from('sys_log')
             ->where(
@@ -429,24 +472,8 @@ public function getCommentsForRecord($uid, $table)
                 )
             )
             ->orderBy('tstamp', 'DESC')
-            ->executeQuery();
-
-        /** @var Avatar $avatar */
-        $avatar = GeneralUtility::makeInstance(Avatar::class);
-
-        while ($sysLogRow = $result->fetchAssociative()) {
-            $sysLogEntry = [];
-            $data = $this->unserializeLogData($sysLogRow['log_data'] ?? '');
-            $beUserRecord = BackendUtility::getRecord('be_users', $sysLogRow['userid']);
-            $sysLogEntry['stage_title'] = htmlspecialchars($this->stagesService->getStageTitle($data['stage']));
-            $sysLogEntry['user_uid'] = (int)$sysLogRow['userid'];
-            $sysLogEntry['user_username'] = is_array($beUserRecord) ? htmlspecialchars($beUserRecord['username']) : '';
-            $sysLogEntry['tstamp'] = htmlspecialchars(BackendUtility::datetime($sysLogRow['tstamp']));
-            $sysLogEntry['user_comment'] = nl2br(htmlspecialchars($data['comment']));
-            $sysLogEntry['user_avatar'] = $avatar->render($beUserRecord);
-            $sysLogReturnArray[] = $sysLogEntry;
-        }
-        return $sysLogReturnArray;
+            ->executeQuery()
+            ->fetchAllAssociative();
     }
 
     protected function getBackendUser(): BackendUserAuthentication

diff --git a/typo3/sysext/workspaces/Classes/Hook/DataHandlerHook.php b/typo3/sysext/workspaces/Classes/Hook/DataHandlerHook.php
@@ -19,6 +19,7 @@
 use Doctrine\DBAL\Platforms\PostgreSQL94Platform as PostgreSQLPlatform;
 use Doctrine\DBAL\Platforms\SQLServer2012Platform as SQLServerPlatform;
 use TYPO3\CMS\Backend\Utility\BackendUtility;
+use TYPO3\CMS\Core\Authentication\BackendUserAuthentication;
 use TYPO3\CMS\Core\Cache\CacheManager;
 use TYPO3\CMS\Core\Context\Context;
 use TYPO3\CMS\Core\Context\WorkspaceAspect;
@@ -27,6 +28,7 @@
 use TYPO3\CMS\Core\Database\Query\Restriction\DeletedRestriction;
 use TYPO3\CMS\Core\Database\RelationHandler;
 use TYPO3\CMS\Core\DataHandling\DataHandler;
+use TYPO3\CMS\Core\DataHandling\History\RecordHistoryStore;
 use TYPO3\CMS\Core\Localization\LanguageService;
 use TYPO3\CMS\Core\SysLog\Action\Database as DatabaseAction;
 use TYPO3\CMS\Core\SysLog\Error as SystemLogErrorClassification;
@@ -487,8 +489,10 @@ protected function version_setStage($table, $id, $stageId, string $comment, Data
             $dataHandler->log($table, $id, DatabaseAction::VERSIONIZE, 0, SystemLogErrorClassification::USER_ERROR, 'Attempt to set stage for record failed: {reason}', -1, ['reason' => $errorCode]);
         } elseif ($dataHandler->checkRecordUpdateAccess($table, $id)) {
             $workspaceInfo = $dataHandler->BE_USER->checkWorkspace($record['t3ver_wsid']);
+            $workspaceId = (int)$workspaceInfo['uid'];
+            $currentStage = (int)$record['t3ver_stage'];
             // check if the user is allowed to the current stage, so it's also allowed to send to next stage
-            if ($dataHandler->BE_USER->workspaceCheckStageForCurrent($record['t3ver_stage'])) {
+            if ($dataHandler->BE_USER->workspaceCheckStageForCurrent($currentStage)) {
                 // Set stage of record:
                 GeneralUtility::makeInstance(ConnectionPool::class)
                     ->getConnectionForTable($table)
@@ -505,8 +509,9 @@ protected function version_setStage($table, $id, $stageId, string $comment, Data
                     $pid = $propertyArray['pid'];
                     $dataHandler->log($table, $id, DatabaseAction::VERSIONIZE, 0, SystemLogErrorClassification::MESSAGE, 'Stage for record was changed to {stage}. Comment was: "{comment}"', -1, ['stage' =>  $stageId, 'comment' => mb_substr($comment, 0, 100)], $dataHandler->eventPid($table, $id, $pid));
                 }
-                // TEMPORARY, except 6-30 as action/detail number which is observed elsewhere!
-                $dataHandler->log($table, $id, DatabaseAction::UPDATE, 0, SystemLogErrorClassification::MESSAGE, 'Stage raised to {stage}', 30, ['comment' => $comment, 'stage' => $stageId]);
+                // Write the stage change to history
+                $historyStore = $this->getRecordHistoryStore($workspaceId, $dataHandler->BE_USER);
+                $historyStore->changeStageForRecord($table, (int)$id, ['current' => $currentStage, 'next' => $stageId, 'comment' => $comment]);
                 if ((int)$workspaceInfo['stagechg_notification'] > 0) {
                     $this->notificationEmailInfo[$workspaceInfo['uid'] . ':' . $stageId . ':' . $comment]['shared'] = [$workspaceInfo, $stageId, $comment];
                     $this->notificationEmailInfo[$workspaceInfo['uid'] . ':' . $stageId . ':' . $comment]['elements'][] = [$table, $id];
@@ -587,12 +592,13 @@ protected function version_swap($table, $id, $swapWith, DataHandler $dataHandler
             return;
         }
         $workspaceId = (int)$swapVersion['t3ver_wsid'];
+        $currentStage = (int)$swapVersion['t3ver_stage'];
         if (!$dataHandler->BE_USER->workspacePublishAccess($workspaceId)) {
             $dataHandler->log($table, (int)$id, DatabaseAction::PUBLISH, 0, SystemLogErrorClassification::USER_ERROR, 'User could not publish records from workspace #{workspace}', -1, ['workspace' => $workspaceId]);
             return;
         }
         $wsAccess = $dataHandler->BE_USER->checkWorkspace($workspaceId);
-        if (!($workspaceId <= 0 || !($wsAccess['publish_access'] & 1) || (int)$swapVersion['t3ver_stage'] === StagesService::STAGE_PUBLISH_ID)) {
+        if (!($workspaceId <= 0 || !($wsAccess['publish_access'] & 1) || $currentStage === StagesService::STAGE_PUBLISH_ID)) {
             $dataHandler->log($table, (int)$id, DatabaseAction::PUBLISH, 0, SystemLogErrorClassification::USER_ERROR, 'Records in workspace #{workspace} can only be published when in "Publish" stage.', -1, ['workspace' => $workspaceId]);
             return;
         }
@@ -767,7 +773,9 @@ protected function version_swap($table, $id, $swapWith, DataHandler $dataHandler
                 $pid = $propArr['pid'];
                 $dataHandler->log($table, $id, DatabaseAction::VERSIONIZE, 0, SystemLogErrorClassification::MESSAGE, 'Stage for record was changed to ' . $stageId . '. Comment was: "' . substr($comment, 0, 100) . '"', -1, [], $dataHandler->eventPid($table, $id, $pid));
             }
-            $dataHandler->log($table, $id, DatabaseAction::UPDATE, 0, SystemLogErrorClassification::MESSAGE, 'Published', 30, ['comment' => $comment, 'stage' => $stageId]);
+            // Write the stage change to the history
+            $historyStore = $this->getRecordHistoryStore((int)$wsAccess['uid'], $dataHandler->BE_USER);
+            $historyStore->changeStageForRecord($table, (int)$id, ['current' => $currentStage, 'next' => StagesService::STAGE_PUBLISH_EXECUTE_ID, 'comment' => $comment]);
 
             // Clear cache:
             $dataHandler->registerRecordIdForPageCacheClearing($table, $id);
@@ -989,8 +997,10 @@ protected function publishNewRecord(string $table, array $newRecordInWorkspace,
         $this->notificationEmailInfo[$notificationEmailInfoKey]['elements'][] = [$table, $id];
         $this->notificationEmailInfo[$notificationEmailInfoKey]['recipients'] = $notificationAlternativeRecipients;
         // Write to log with stageId -20 (STAGE_PUBLISH_EXECUTE_ID)
-        $dataHandler->log($table, $id, DatabaseAction::VERSIONIZE, 0, SystemLogErrorClassification::MESSAGE, 'Stage for record was changed to {stage}. Comment was: "{comment}"', -1, ['stage' => (int)$stageId, 'comment' => substr($comment, 0, 100)], $dataHandler->eventPid($table, $id, $newRecordInWorkspace['pid']));
-        $dataHandler->log($table, $id, DatabaseAction::UPDATE, 0, SystemLogErrorClassification::MESSAGE, 'Published', 30, ['comment' => $comment, 'stage' => $stageId]);
+        $dataHandler->log($table, $id, DatabaseAction::VERSIONIZE, 0, SystemLogErrorClassification::MESSAGE, 'Stage for record was changed to {stage}. Comment was: "{comment}"', -1, ['stage' => $stageId, 'comment' => substr($comment, 0, 100)], $dataHandler->eventPid($table, $id, $newRecordInWorkspace['pid']));
+        // Write the stage change to the history (usually this is done in updateDB in DataHandler, but we do a manual SQL change)
+        $historyStore = $this->getRecordHistoryStore((int)$wsAccess['uid'], $dataHandler->BE_USER);
+        $historyStore->changeStageForRecord($table, $id, ['current' => (int)$newRecordInWorkspace['t3ver_stage'], 'next' => StagesService::STAGE_PUBLISH_EXECUTE_ID, 'comment' => $comment]);
 
         // Clear cache
         $dataHandler->registerRecordIdForPageCacheClearing($table, $id);
@@ -1541,6 +1551,25 @@ protected function softOrHardDeleteSingleRecord(string $table, int $uid): void
         }
     }
 
+    /**
+     * Makes an instance for RecordHistoryStore. This is needed as DataHandler would usually trigger the setHistory()
+     * but has no support for tracking "stage change" information.
+     *
+     * So we have to do this manually. Usually a $dataHandler->updateDB() could do this, but we use raw update statements
+     * here in workspaces for the time being, mostly because we also want to add "comment"
+     */
+    protected function getRecordHistoryStore(int $workspaceId, BackendUserAuthentication $user): RecordHistoryStore
+    {
+        return GeneralUtility::makeInstance(
+            RecordHistoryStore::class,
+            RecordHistoryStore::USER_BACKEND,
+            (int)$user->user['uid'],
+            $user->getOriginalUserIdWhenInSwitchUserMode(),
+            $GLOBALS['EXEC_TIME'],
+            $workspaceId
+        );
+    }
+
     /**
      * @return RelationHandler
      */

diff --git a/typo3/sysext/workspaces/Classes/Service/HistoryService.php b/typo3/sysext/workspaces/Classes/Service/HistoryService.php
@@ -18,6 +18,7 @@
 use TYPO3\CMS\Backend\Backend\Avatar\Avatar;
 use TYPO3\CMS\Backend\History\RecordHistory;
 use TYPO3\CMS\Backend\Utility\BackendUtility;
+use TYPO3\CMS\Core\DataHandling\History\RecordHistoryStore;
 use TYPO3\CMS\Core\Localization\LanguageService;
 use TYPO3\CMS\Core\SingletonInterface;
 use TYPO3\CMS\Core\Utility\DiffUtility;
@@ -62,7 +63,10 @@ public function getHistory($table, $id)
     {
         $history = [];
         $i = 0;
-        foreach ((array)$this->getHistoryEntries($table, $id) as $entry) {
+        foreach ($this->getHistoryEntries($table, $id) as $entry) {
+            if ((int)($entry['actiontype'] ?? 0) === RecordHistoryStore::ACTION_STAGECHANGE) {
+                continue;
+            }
             if ($i++ > 20) {
                 break;
             }
@@ -71,6 +75,19 @@ public function getHistory($table, $id)
         return $history;
     }
 
+    public function getStageChanges(string $table, int $id): array
+    {
+        $stageChanges = [];
+        foreach ($this->getHistoryEntries($table, $id) as $entry) {
+            if ((int)($entry['actiontype'] ?? 0) !== RecordHistoryStore::ACTION_STAGECHANGE) {
+                continue;
+            }
+            $stageChanges[] = $entry;
+        }
+
+        return $stageChanges;
+    }
+
     /**
      * Gets the human readable representation of one
      * record history entry.

diff --git a/typo3/sysext/workspaces/Resources/Public/JavaScript/Backend.js b/typo3/sysext/workspaces/Resources/Public/JavaScript/Backend.js