From 5476d001b44d4c6c825640b9f2d0ee492292a141 Mon Sep 17 00:00:00 2001 From: Matthias Vogel Date: Wed, 12 Aug 2026 14:36:00 +0200 Subject: [PATCH] [FEATURE] Combine related sys_history entries on save Merge separate Visual Editor saves made within 60 seconds into one history entry so editors see the effective change instead of intermediate autosaves. Delay autosaves for one seconds to batch short edit bursts, and index recent history entries so combining remains efficient as the history grows. Changes that result in no effective difference are removed from the history. related #124 --- .../Controller/PersistenceController.php | 13 +- Classes/Service/DataHandlerService.php | 5 +- Classes/SysHistory/SysHistoryCombiner.php | 170 ++++++++++++ Classes/SysHistory/SysHistoryRepository.php | 61 +++++ .../SysHistoryRepositoryInterface.php | 23 ++ .../Backend/components/ve-auto-save-toggle.js | 2 +- .../RecordingSysHistoryRepository.php | 51 ++++ .../SysHistory/SysHistoryCombinerTest.php | 243 ++++++++++++++++++ ext_tables.sql | 3 + 9 files changed, 567 insertions(+), 4 deletions(-) create mode 100644 Classes/SysHistory/SysHistoryCombiner.php create mode 100644 Classes/SysHistory/SysHistoryRepository.php create mode 100644 Classes/SysHistory/SysHistoryRepositoryInterface.php create mode 100644 Tests/Unit/SysHistory/RecordingSysHistoryRepository.php create mode 100644 Tests/Unit/SysHistory/SysHistoryCombinerTest.php create mode 100644 ext_tables.sql diff --git a/Classes/Backend/Controller/PersistenceController.php b/Classes/Backend/Controller/PersistenceController.php index 66b2d2e..cca32c7 100644 --- a/Classes/Backend/Controller/PersistenceController.php +++ b/Classes/Backend/Controller/PersistenceController.php @@ -8,8 +8,11 @@ use Psr\Http\Message\ServerRequestInterface; use RuntimeException; use TYPO3\CMS\Backend\Attribute\AsController; +use TYPO3\CMS\Core\Crypto\Random; +use TYPO3\CMS\Core\DataHandling\Model\CorrelationId; use TYPO3\CMS\Core\Http\JsonResponse; use TYPO3\CMS\VisualEditor\Service\DataHandlerService; +use TYPO3\CMS\VisualEditor\SysHistory\SysHistoryCombiner; use function array_keys; use function implode; @@ -20,6 +23,8 @@ { public function __construct( private DataHandlerService $dataHandlerService, + private SysHistoryCombiner $sysHistoryCombiner, + private Random $randomGenerator, ) { } @@ -44,12 +49,16 @@ public function saveAction(ServerRequestInterface $request): ResponseInterface } $GLOBALS['TYPO3_REQUEST'] = $request; - $errorLog = $this->dataHandlerService->run($data, []); + $correlationId = CorrelationId::forScope($this->randomGenerator->generateRandomBase64String(32)) + ->withAspects(SysHistoryCombiner::CORRELATION_ASPECT); + $errorLog = $this->dataHandlerService->run($data, [], $correlationId); foreach ($cmdArray as $cmd) { - $errorLog = [...$errorLog, ...$this->dataHandlerService->run([], $cmd)]; + $errorLog = [...$errorLog, ...$this->dataHandlerService->run([], $cmd, $correlationId)]; } + $this->sysHistoryCombiner->combine(); + if ($errorLog) { return new JsonResponse(['success' => false, 'errorLog' => $errorLog], 500); } diff --git a/Classes/Service/DataHandlerService.php b/Classes/Service/DataHandlerService.php index cb8d494..216e014 100644 --- a/Classes/Service/DataHandlerService.php +++ b/Classes/Service/DataHandlerService.php @@ -6,6 +6,7 @@ use RuntimeException; use TYPO3\CMS\Core\DataHandling\DataHandler; +use TYPO3\CMS\Core\DataHandling\Model\CorrelationId; use TYPO3\CMS\Core\Schema\TcaSchemaFactory; use TYPO3\CMS\Core\Utility\GeneralUtility; @@ -26,15 +27,17 @@ public function __construct( * * @return list */ - public function run(array $data, array $cmd): array + public function run(array $data, array $cmd, CorrelationId $correlationId): array { $this->validateData($data); $this->validateCmd($cmd); $dataHandler = GeneralUtility::makeInstance(DataHandler::class); // never use DataHandler over DI!! $dataHandler->start($data, $cmd); + $dataHandler->setCorrelationId($correlationId); $dataHandler->process_datamap(); $dataHandler->process_cmdmap(); + return $dataHandler->errorLog; } diff --git a/Classes/SysHistory/SysHistoryCombiner.php b/Classes/SysHistory/SysHistoryCombiner.php new file mode 100644 index 0000000..5c6cb76 --- /dev/null +++ b/Classes/SysHistory/SysHistoryCombiner.php @@ -0,0 +1,170 @@ +getMappedRows($timeToCombine); + foreach ($mapped as $rowsToCombine) { + $toBeDeletedUids = [...$toBeDeletedUids, ...$this->combineRows($rowsToCombine)]; + } + + $this->sysHistoryRepository->deleteEntries($toBeDeletedUids); + } + + /** + * @return array>> + */ + private function getMappedRows(int $timeToCombine): array + { + $mapped = []; + $groupedPerRecord = []; + foreach ($this->sysHistoryRepository->fetchSysHistory($timeToCombine) as $row) { + $key = implode('-', [ + $row['recuid'], + $row['tablename'], + $row['workspace'], + ]); + $groupedPerRecord[$key] ??= []; + $groupedPerRecord[$key][] = $row; + } + + foreach ($groupedPerRecord as $parentKey => $rows) { + $lastKey = null; + $counter = 0; + foreach ($rows as $row) { + if (!$this->isVisualEditorModification($row)) { + $lastKey = null; + $counter++; + continue; + } + + $key = implode('-', [ + $row['actiontype'], + $row['usertype'], + $row['userid'], + $row['originaluserid'], + ]); + if ($key !== $lastKey) { + $lastKey = $key; + $counter++; + } + + $mapped[$counter . '-' . $parentKey . '-' . $key][] = $row; + } + } + + return $mapped; + } + + /** + * @param array $row + */ + private function isVisualEditorModification(array $row): bool + { + if ((int)$row['actiontype'] !== RecordHistoryStore::ACTION_MODIFY) { + return false; + } + + try { + return CorrelationId::fromString((string)$row['correlation_id'])->getAspects() === [self::CORRELATION_ASPECT]; + } catch (InvalidArgumentException) { + return false; + } + } + + /** + * @param list> $rowsToCombine + * @return list + */ + private function combineRows(array $rowsToCombine): array + { + if (count($rowsToCombine) <= 1) { + return []; + } + + // first is the Oldest, last is the Newest + $first = $rowsToCombine[0]; + $last = $rowsToCombine[array_key_last($rowsToCombine)]; + $newHistoryData = json_decode($first['history_data'], true, flags: JSON_THROW_ON_ERROR); + foreach (array_slice($rowsToCombine, 1) as $rowToCombine) { + $currentHistoryData = json_decode($rowToCombine['history_data'], true, flags: JSON_THROW_ON_ERROR); + + $newHistoryData['oldRecord'] = [ + ...$currentHistoryData['oldRecord'], + ...$newHistoryData['oldRecord'], + ]; + $newHistoryData['newRecord'] = [ + ...$newHistoryData['newRecord'], + ...$currentHistoryData['newRecord'], + ]; + } + + // if one field is the same, we can remove it from the diff, because it is not a change + $newHistoryData = $this->reduce($newHistoryData); + + // if you change the text and than change it back, the resulting diff will be empty, so we can just delete all history entries + if ($this->isTheSame($newHistoryData['oldRecord'], $newHistoryData['newRecord'])) { + return array_map(intval(...), array_column($rowsToCombine, 'uid')); + } + + $this->sysHistoryRepository->updateHistoryData((int)$last['uid'], $newHistoryData); + + return array_map(intval(...), array_column(array_slice($rowsToCombine, 0, -1), 'uid')); + } + + /** + * @param array $oldRecord + * @param array $newRecord + */ + private function isTheSame(array $oldRecord, array $newRecord): bool + { + unset($oldRecord['l18n_diffsource'], $newRecord['l18n_diffsource']); + + $normalizeScalarValue = static fn(mixed $value): mixed => is_scalar($value) ? (string)$value : $value; + $oldRecord = array_map($normalizeScalarValue, $oldRecord); + $newRecord = array_map($normalizeScalarValue, $newRecord); + + return $oldRecord === $newRecord; + } + + /** + * @param array{oldRecord: array, newRecord: array} $newHistoryData + * @return array{oldRecord: array, newRecord: array} + */ + private function reduce(array $newHistoryData): array + { + foreach ($newHistoryData['oldRecord'] as $field => $value) { + if (isset($newHistoryData['newRecord'][$field]) && $newHistoryData['newRecord'][$field] === $value) { + unset($newHistoryData['oldRecord'][$field], $newHistoryData['newRecord'][$field]); + } + } + + return $newHistoryData; + } +} diff --git a/Classes/SysHistory/SysHistoryRepository.php b/Classes/SysHistory/SysHistoryRepository.php new file mode 100644 index 0000000..129efd4 --- /dev/null +++ b/Classes/SysHistory/SysHistoryRepository.php @@ -0,0 +1,61 @@ +> + */ + public function fetchSysHistory(int $timeToFetch): array + { + $queryBuilder = $this->connectionPool->getQueryBuilderForTable('sys_history'); + $result = $queryBuilder + ->select('*') + ->from('sys_history') + ->where($queryBuilder->expr()->gte('tstamp', (new DateTimeImmutable())->getTimestamp() - $timeToFetch)) + ->orderBy('uid', 'ASC') + ->executeQuery(); + return $result->fetchAllAssociative(); + } + + /** + * @param list $toBeDeletedUids + */ + public function deleteEntries(array $toBeDeletedUids): void + { + if ($toBeDeletedUids) { + $queryBuilder = $this->connectionPool->getQueryBuilderForTable('sys_history'); + $queryBuilder + ->delete('sys_history') + ->where($queryBuilder->expr()->in('uid', array_map(intval(...), $toBeDeletedUids))) + ->executeStatement(); + } + } + + /** + * @param array $newHistoryData + */ + public function updateHistoryData(int $uid, array $newHistoryData): void + { + $this->connectionPool->getConnectionForTable('sys_history')->update( + 'sys_history', + ['history_data' => json_encode($newHistoryData, JSON_THROW_ON_ERROR)], + ['uid' => $uid], + ); + } +} diff --git a/Classes/SysHistory/SysHistoryRepositoryInterface.php b/Classes/SysHistory/SysHistoryRepositoryInterface.php new file mode 100644 index 0000000..b560d1a --- /dev/null +++ b/Classes/SysHistory/SysHistoryRepositoryInterface.php @@ -0,0 +1,23 @@ +> + */ + public function fetchSysHistory(int $timeToFetch): array; + + /** + * @param list $toBeDeletedUids + */ + public function deleteEntries(array $toBeDeletedUids): void; + + /** + * @param array $newHistoryData + */ + public function updateHistoryData(int $uid, array $newHistoryData): void; +} diff --git a/Resources/Public/JavaScript/Backend/components/ve-auto-save-toggle.js b/Resources/Public/JavaScript/Backend/components/ve-auto-save-toggle.js index e80a10c..62877e6 100644 --- a/Resources/Public/JavaScript/Backend/components/ve-auto-save-toggle.js +++ b/Resources/Public/JavaScript/Backend/components/ve-auto-save-toggle.js @@ -49,7 +49,7 @@ export class VeAutoSaveToggle extends LitElement { super.connectedCallback(); if (!this.disposeUpdateEditorStateListener) { - this.disposeUpdateEditorStateListener = onMessageDebounced('updateEditorState', this.#onEditorStateMessage.bind(this), 300); + this.disposeUpdateEditorStateListener = onMessageDebounced('updateEditorState', this.#onEditorStateMessage.bind(this), 1000); } this.addEventListener('click', this.onClick); diff --git a/Tests/Unit/SysHistory/RecordingSysHistoryRepository.php b/Tests/Unit/SysHistory/RecordingSysHistoryRepository.php new file mode 100644 index 0000000..6a1414a --- /dev/null +++ b/Tests/Unit/SysHistory/RecordingSysHistoryRepository.php @@ -0,0 +1,51 @@ +> */ + public array $recordedDeleteEntries = []; + + /** @var array> */ + public array $recordedUpdateHistoryData = []; + + /** + * @param list> $sysHistoryData + */ + public function __construct(private readonly array $sysHistoryData = []) + { + } + + /** + * @return list> + */ + public function fetchSysHistory(int $timeToFetch): array + { + return $this->sysHistoryData; + } + + /** + * @param list $toBeDeletedUids + */ + public function deleteEntries(array $toBeDeletedUids): void + { + $this->recordedDeleteEntries[] = $toBeDeletedUids; + } + + /** + * @param array $newHistoryData + */ + public function updateHistoryData(int $uid, array $newHistoryData): void + { + $this->recordedUpdateHistoryData[$uid] = $newHistoryData; + } +} diff --git a/Tests/Unit/SysHistory/SysHistoryCombinerTest.php b/Tests/Unit/SysHistory/SysHistoryCombinerTest.php new file mode 100644 index 0000000..01472d3 --- /dev/null +++ b/Tests/Unit/SysHistory/SysHistoryCombinerTest.php @@ -0,0 +1,243 @@ +> $sysHistoryData + * @param list> $recordedDeleteEntries + * @param array> $recordedUpdateHistoryData + */ + #[Test] + #[DataProvider('provideCombineData')] + public function combine( + array $sysHistoryData, + array $recordedDeleteEntries = [], + array $recordedUpdateHistoryData = [], + int $timeToCombine = 60 + ): void { + $recordings = new RecordingSysHistoryRepository($sysHistoryData); + $combiner = new SysHistoryCombiner($recordings); + $combiner->combine($timeToCombine); + self::assertSame($recordedDeleteEntries, $recordings->recordedDeleteEntries, 'Delete entries do not match'); + self::assertSame($recordedUpdateHistoryData, $recordings->recordedUpdateHistoryData, 'Update history data does not match'); + } + + public static function provideCombineData(): Generator + { + yield 'no data' => [ + 'sysHistoryData' => [], + 'recordedDeleteEntries' => [[]], + 'recordedUpdateHistoryData' => [], + ]; + yield 'simple' => [ + 'sysHistoryData' => [ + self::createRow(1, 'A', 'B'), + self::createRow(2, 'B', 'C'), + ], + 'recordedDeleteEntries' => [[1]], + 'recordedUpdateHistoryData' => [ + 2 => self::createResult('A', 'C'), + ], + ]; + yield 'visual editor correlations combine across scopes' => [ + 'sysHistoryData' => [ + self::createRow(1, 'A', 'B', correlationId: '0400$scope-1:subject/visual-editor'), + self::createRow(2, 'B', 'C', correlationId: '0400$scope-2:subject/visual-editor'), + ], + 'recordedDeleteEntries' => [[1]], + 'recordedUpdateHistoryData' => [ + 2 => self::createResult('A', 'C'), + ], + ]; + yield 'non visual editor correlations are rollback boundaries' => [ + 'sysHistoryData' => [ + self::createRow(1, 'A', 'B', correlationId: '0400$scope-1:subject/redirects/slug'), + self::createRow(2, 'B', 'C', correlationId: '0400$scope-2:subject/redirects/slug'), + ], + 'recordedDeleteEntries' => [[]], + 'recordedUpdateHistoryData' => [], + ]; + yield 'non visual editor correlations separate visual editor modifications' => [ + 'sysHistoryData' => [ + self::createRow(1, 'A', 'B', correlationId: '0400$scope-1:subject/visual-editor'), + self::createRow(2, 'B', 'B', correlationId: '0400$scope-2:subject/visual-editor/redirects/slug'), + self::createRow(3, 'B', 'C', correlationId: '0400$scope-3:subject/visual-editor'), + ], + 'recordedDeleteEntries' => [[]], + 'recordedUpdateHistoryData' => [], + ]; + yield 'change to and back with type missmatch' => [ + 'sysHistoryData' => [ + self::createRow(1, 1, 2), + self::createRow(2, 2, '1'), + ], + 'recordedDeleteEntries' => [[1, 2]], + 'recordedUpdateHistoryData' => [], + ]; + yield 'structured values are not treated as identical' => [ + 'sysHistoryData' => [ + self::createRow(1, ['a'], ['b']), + self::createRow(2, ['b'], ['c']), + ], + 'recordedDeleteEntries' => [[1]], + 'recordedUpdateHistoryData' => [ + 2 => self::createResult(['a'], ['c']), + ], + ]; + yield 'null and empty strings are not treated as identical' => [ + 'sysHistoryData' => [ + self::createRow(1, null, 'value'), + self::createRow(2, 'value', ''), + ], + 'recordedDeleteEntries' => [[1]], + 'recordedUpdateHistoryData' => [ + 2 => self::createResult(null, ''), + ], + ]; + yield 'different Users' => [ + 'sysHistoryData' => [ + self::createRow(1, 'A', 'B', 2), + self::createRow(2, 'B', 'C', 3), + ], + 'recordedDeleteEntries' => [[]], + 'recordedUpdateHistoryData' => [], + ]; + yield 'different Users still combining' => [ + 'sysHistoryData' => [ + self::createRow(1, 'A', 'B', 2), + self::createRow(2, 'B', 'C', 2), + self::createRow(3, 'C', 'D', 3), + ], + 'recordedDeleteEntries' => [[1]], + 'recordedUpdateHistoryData' => [ + 2 => self::createResult('A', 'C'), + ], + ]; + yield 'not combining over other users entries' => [ + 'sysHistoryData' => [ + self::createRow(1, 'A', 'B', userid: 2), + self::createRow(2, 'B', 'C', userid: 2), + self::createRow(3, 'C', 'D', userid: 3), + self::createRow(4, 'D', 'E', userid: 2), + self::createRow(5, 'E', 'F', userid: 2), + ], + 'recordedDeleteEntries' => [[1, 4]], + 'recordedUpdateHistoryData' => [ + 2 => self::createResult('A', 'C'), + 5 => self::createResult('D', 'F'), + ], + ]; + yield 'not combining over intervening actions' => [ + 'sysHistoryData' => [ + self::createRow(1, 'A', 'B'), + self::createRow(2, 'B', 'B', actiontype: 3), + self::createRow(3, 'B', 'C'), + ], + 'recordedDeleteEntries' => [[]], + 'recordedUpdateHistoryData' => [], + ]; + yield 'combining over another record by the same user' => [ + 'sysHistoryData' => [ + self::createRow(1, 'A', 'B', recuid: 9), + self::createRow(2, 'A', 'B', recuid: 10), + self::createRow(3, 'B', 'C', recuid: 9), + ], + 'recordedDeleteEntries' => [[1]], + 'recordedUpdateHistoryData' => [ + 3 => self::createResult('A', 'C'), + ], + ]; + yield 'combining over another workspace by the same user' => [ + 'sysHistoryData' => [ + self::createRow(1, 'A', 'B', workspace: 0), + self::createRow(2, 'A', 'B', workspace: 2), + self::createRow(3, 'B', 'C', workspace: 0), + ], + 'recordedDeleteEntries' => [[1]], + 'recordedUpdateHistoryData' => [ + 3 => self::createResult('A', 'C', workspace: 0), + ], + ]; + yield 'not combining over other users entries second user also combines' => [ + 'sysHistoryData' => [ + self::createRow(1, 'A', 'B', userid: 2), + self::createRow(2, 'B', 'C', userid: 2), + self::createRow(3, 'C', 'D', userid: 3), + self::createRow(4, 'D', 'E', userid: 3), + self::createRow(5, 'E', 'F', userid: 2), + ], + 'recordedDeleteEntries' => [[1, 3]], + 'recordedUpdateHistoryData' => [ + 2 => self::createResult('A', 'C'), + 4 => self::createResult('C', 'E'), + ], + ]; + yield 'lot of changes' => [ + 'sysHistoryData' => [ + self::createRow(1, 'A', 'B'), + self::createRow(2, 'B', 'C'), + self::createRow(3, 'C', 'D'), + self::createRow(4, 'D', 'E'), + self::createRow(5, 'E', 'F'), + ], + 'recordedDeleteEntries' => [[1, 2, 3, 4]], + 'recordedUpdateHistoryData' => [ + 5 => self::createResult('A', 'F'), + ], + ]; + } + + /** + * @return array + */ + public static function createRow(int $uid, mixed $oldValue, mixed $newValue, int $userid = 1, int $recuid = 9, int $workspace = 3, int $actiontype = 2, string $correlationId = '0400$scope:subject/visual-editor'): array + { + return [ + 'uid' => $uid, + 'tstamp' => $uid, + 'actiontype' => $actiontype, + 'usertype' => 'BE', + 'userid' => $userid, + 'originaluserid' => 0, + 'recuid' => $recuid, + 'tablename' => 'tt_content', + 'history_data' => json_encode([ + 'oldRecord' => [ + 'header' => $oldValue, + ], + 'newRecord' => [ + 'header' => $newValue, + ], + 'workspace' => $workspace, + ]), + 'workspace' => $workspace, + 'correlation_id' => $correlationId, + ]; + } + + /** + * @return array{oldRecord: array{header: mixed}, newRecord: array{header: mixed}, workspace: int} + */ + public static function createResult(mixed $oldValue, mixed $newValue, int $workspace = 3): array + { + return [ + 'oldRecord' => [ + 'header' => $oldValue, + ], + 'newRecord' => [ + 'header' => $newValue, + ], + 'workspace' => $workspace, + ]; + } +} diff --git a/ext_tables.sql b/ext_tables.sql new file mode 100644 index 0000000..189c0d3 --- /dev/null +++ b/ext_tables.sql @@ -0,0 +1,3 @@ +CREATE TABLE sys_history ( + KEY visual_editor_tstamp_uid (tstamp,uid) +);