From 1e0e4f5f04431bec78319c007e7abfe31bae1429 Mon Sep 17 00:00:00 2001
From: Christian Kuhn <lolli@schwarzbu.ch>
Date: Thu, 25 Nov 2021 15:54:45 +0100
Subject: [PATCH] [BUGFIX] Update CSV references when discarding record
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
When an element is discarded in workspaces that is referenced
by another record in a group CSV field, those referencing
records need to be updated to no longer list the discarded
record.
The patch adds a method to DataHandler discard logic to
take care of that.
This resolves the very last call setting assertCleanReferenceIndex
to false. That test property has been a temporary solution and
is dropped now, tests extending AbstractDataHandlerActionTestCase
can no longer (easily) prevent the tearDown() refindex check and
should never do that again.
Change-Id: Ifd2e5893d2d39b77762920b8b41ab547ed20ecd7
Resolves: #96080
Releases: master, 11.5
Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/72287
Tested-by: core-ci <typo3@b13.com>
Tested-by: Stefan Bürk <stefan@buerk.tech>
Tested-by: Benni Mack <benni@typo3.org>
Tested-by: Christian Kuhn <lolli@schwarzbu.ch>
Reviewed-by: Stefan Bürk <stefan@buerk.tech>
Reviewed-by: Benni Mack <benni@typo3.org>
Reviewed-by: Christian Kuhn <lolli@schwarzbu.ch>
---
.../core/Classes/DataHandling/DataHandler.php | 70 +++++++++++++++++++
.../AbstractDataHandlerActionTestCase.php | 9 +--
.../DataHandling/Group/Discard/ActionTest.php | 2 -
...eContentNCreateRelationNDiscardElement.csv | 4 +-
.../DataSet/deleteContentOfRelation.csv | 10 ++-
.../DataSet/deleteContentOfRelation.csv | 10 ++-
6 files changed, 90 insertions(+), 15 deletions(-)
diff --git a/typo3/sysext/core/Classes/DataHandling/DataHandler.php b/typo3/sysext/core/Classes/DataHandling/DataHandler.php
index 646628f07a09..b11f9227231d 100644
--- a/typo3/sysext/core/Classes/DataHandling/DataHandler.php
+++ b/typo3/sysext/core/Classes/DataHandling/DataHandler.php
@@ -5588,6 +5588,7 @@ class DataHandler implements LoggerAwareInterface
$this->discardLocalizationOverlayRecords($table, $versionRecord);
$this->discardRecordRelations($table, $versionRecord);
+ $this->discardCsvReferencesToRecord($table, $versionRecord);
$this->hardDeleteSingleRecord($table, (int)$versionRecord['uid']);
$this->deletedRecords[$table][] = (int)$versionRecord['uid'];
$this->registerReferenceIndexRowsForDrop($table, (int)$versionRecord['uid'], $userWorkspace);
@@ -5709,6 +5710,75 @@ class DataHandler implements LoggerAwareInterface
}
}
+ /**
+ * When the to-discard record is the target of a CSV group field of another table record,
+ * these records need to be updated to no longer point to the discarded record.
+ *
+ * Those referencing records are not very easy to find with only the to-discard record being available.
+ * The solution used here looks up records referencing the to-discard record by fetching a list of
+ * references from sys_refindex, where the to-discard record is on the right side (ref_* fields)
+ * and in the workspace the to-discard record lives in. The referencing record fields are then updated
+ * to drop the to-discard record from the CSV list.
+ *
+ * Using sys_refindex for this task is a bit risky: This would fail if a DataHandler call
+ * adds a reference to the record and requests discarding the record in one call - the refindex
+ * is always only updated at the very end of a DataHandler call, the logic below wouldn't catch
+ * this since it would be based on an outdated sys_refindex. The scenario however is of little use and
+ * not used in core, so it should be fine.
+ *
+ * @param string $table Table name of this record
+ * @param array $record The record row to handle
+ */
+ protected function discardCsvReferencesToRecord(string $table, array $record): void
+ {
+ // @see test workspaces Group Discard createContentAndCreateElementRelationAndDiscardElement
+ // Records referencing the to-discard record.
+ $queryBuilder = GeneralUtility::makeInstance(ConnectionPool::class)->getQueryBuilderForTable('sys_refindex');
+ $statement = $queryBuilder->select('tablename', 'recuid', 'field')
+ ->from('sys_refindex')
+ ->where(
+ $queryBuilder->expr()->eq('workspace', $queryBuilder->createNamedParameter($record['t3ver_wsid'], \PDO::PARAM_INT)),
+ $queryBuilder->expr()->eq('ref_table', $queryBuilder->createNamedParameter($table)),
+ $queryBuilder->expr()->eq('ref_uid', $queryBuilder->createNamedParameter($record['uid'], \PDO::PARAM_INT))
+ )
+ ->execute();
+ while ($row = $statement->fetchAssociative()) {
+ // For each record referencing the to-discard record, see if it is a CSV group field definition.
+ // If so, update that record to drop both the possible "uid" and "table_name_uid" variants from the list.
+ $fieldTca = $GLOBALS['TCA'][$row['tablename']]['columns'][$row['field']]['config'] ?? [];
+ $groupAllowed = GeneralUtility::trimExplode(',', $fieldTca['allowed'] ?? '', true);
+ // @todo: "select" may be affected too, but it has no coverage to show this, yet?
+ if (($fieldTca['type'] ?? '') === 'group'
+ && empty($fieldTca['MM'])
+ && (in_array('*', $groupAllowed, true) || in_array($table, $groupAllowed, true))
+ ) {
+ // Note it would be possible to a) update multiple records with only one DB call, and b) combine the
+ // select and update to a single update query by doing the CSV manipulation as string function in sql.
+ // That's harder to get right though and probably not *that* beneficial performance-wise since we're
+ // most likely dealing with a very small number of records here anyways. Still, an optimization should
+ // be considered after we drop TCA 'prepend_tname' handling and always rely only on "table_name_uid"
+ // variant for CSV storage.
+
+ // Get that record
+ $recordReferencingDiscardedRecord = BackendUtility::getRecord($row['tablename'], $row['recuid'], $row['field']);
+ if (!$recordReferencingDiscardedRecord) {
+ continue;
+ }
+ // Drop "uid" and "table_name_uid" from list
+ $listOfRelatedRecords = GeneralUtility::trimExplode(',', $recordReferencingDiscardedRecord[$row['field']], true);
+ $listOfRelatedRecordsWithoutDiscardedRecord = array_diff($listOfRelatedRecords, [$record['uid'], $table . '_' . $record['uid']]);
+ if ($listOfRelatedRecords !== $listOfRelatedRecordsWithoutDiscardedRecord) {
+ // Update record if list changed
+ $queryBuilder = GeneralUtility::makeInstance(ConnectionPool::class)->getQueryBuilderForTable($row['tablename']);
+ $queryBuilder->update($row['tablename'])
+ ->set($row['field'], implode(',', $listOfRelatedRecordsWithoutDiscardedRecord))
+ ->where($queryBuilder->expr()->eq('uid', $queryBuilder->createNamedParameter($row['recuid'], \PDO::PARAM_INT)))
+ ->execute();
+ }
+ }
+ }
+ }
+
/**
* When a workspace record row is discarded that has mm relations, existing mm table rows need
* to be deleted. The method performs the delete operation depending on TCA field configuration.
diff --git a/typo3/sysext/core/Tests/Functional/DataHandling/AbstractDataHandlerActionTestCase.php b/typo3/sysext/core/Tests/Functional/DataHandling/AbstractDataHandlerActionTestCase.php
index 43d334806cfa..1c65ffc0e806 100644
--- a/typo3/sysext/core/Tests/Functional/DataHandling/AbstractDataHandlerActionTestCase.php
+++ b/typo3/sysext/core/Tests/Functional/DataHandling/AbstractDataHandlerActionTestCase.php
@@ -39,11 +39,6 @@ abstract class AbstractDataHandlerActionTestCase extends FunctionalTestCase
{
const VALUE_BackendUserId = 1;
- /**
- * @var bool True if assertCleanReferenceIndex() should be called in tearDown(). Set to false only with care.
- */
- protected $assertCleanReferenceIndex = true;
-
/**
* @var string
*/
@@ -130,9 +125,7 @@ abstract class AbstractDataHandlerActionTestCase extends FunctionalTestCase
protected function tearDown(): void
{
$this->assertErrorLogEntries();
- if ($this->assertCleanReferenceIndex) {
- $this->assertCleanReferenceIndex();
- }
+ $this->assertCleanReferenceIndex();
unset($this->actionService);
unset($this->recordIds);
parent::tearDown();
diff --git a/typo3/sysext/workspaces/Tests/Functional/DataHandling/Group/Discard/ActionTest.php b/typo3/sysext/workspaces/Tests/Functional/DataHandling/Group/Discard/ActionTest.php
index cadabab1dff0..3e9066059244 100644
--- a/typo3/sysext/workspaces/Tests/Functional/DataHandling/Group/Discard/ActionTest.php
+++ b/typo3/sysext/workspaces/Tests/Functional/DataHandling/Group/Discard/ActionTest.php
@@ -94,8 +94,6 @@ class ActionTest extends AbstractActionTestCase
*/
public function createContentAndCreateElementRelationAndDiscardElement(): void
{
- // Remove this once the relation part is resolved, see the todo in the CSV
- $this->assertCleanReferenceIndex = false;
$this->createContentAndCreateElementRelation();
$this->actionService->clearWorkspaceRecord(self::TABLE_Element, $this->recordIds['newElementId']);
$this->assertAssertionDataSet('createContentNCreateRelationNDiscardElement');
diff --git a/typo3/sysext/workspaces/Tests/Functional/DataHandling/Group/Discard/DataSet/createContentNCreateRelationNDiscardElement.csv b/typo3/sysext/workspaces/Tests/Functional/DataHandling/Group/Discard/DataSet/createContentNCreateRelationNDiscardElement.csv
index 751d9a36b0d5..eb31b3d50ff0 100644
--- a/typo3/sysext/workspaces/Tests/Functional/DataHandling/Group/Discard/DataSet/createContentNCreateRelationNDiscardElement.csv
+++ b/typo3/sysext/workspaces/Tests/Functional/DataHandling/Group/Discard/DataSet/createContentNCreateRelationNDiscardElement.csv
@@ -19,14 +19,12 @@
,"uid","pid","sorting","deleted","sys_language_uid","l18n_parent","t3ver_wsid","t3ver_state","t3ver_stage","t3ver_oid","header","tx_testdatahandler_group",,,,,
,297,89,256,0,0,0,0,0,0,0,"Regular Element #1","1,2",,,,,
,298,89,512,0,0,0,0,0,0,0,"Regular Element #2","2,3",,,,,
-# @todo: Odd - It could be expected these relations to no longer existing element 4 are discarded too? What happens here with non-ws delete?
-,299,89,128,0,0,0,1,1,0,0,"Testing #1",4,,,,,
+,299,89,128,0,0,0,1,1,0,0,"Testing #1",,,,,,
"tx_testdatahandler_element",,,,,,,,,,,,,,,,,
,"uid","pid","sorting","deleted","sys_language_uid","l10n_parent","t3ver_wsid","t3ver_state","t3ver_stage","t3ver_oid","title",,,,,,
,1,89,256,0,0,0,0,0,0,0,"Element #1",,,,,,
,2,89,512,0,0,0,0,0,0,0,"Element #2",,,,,,
,3,89,768,0,0,0,0,0,0,0,"Element #3",,,,,,
-# @todo: sys_refindex result is broken here. the flag to perform integrity test in tearDown() is disabled for this test.
"sys_refindex",,,,,,,,,,,,,,,,,
,"hash","tablename","recuid","field","flexpointer","softref_key","softref_id","sorting","workspace","ref_table","ref_uid","ref_string",,,,,
,"70386199b2e7d7e3be07ec7b501f411d","tt_content",297,"tx_testdatahandler_group",,,,0,0,"tx_testdatahandler_element",1,,,,,,
diff --git a/typo3/sysext/workspaces/Tests/Functional/DataHandling/ManyToMany/Publish/DataSet/deleteContentOfRelation.csv b/typo3/sysext/workspaces/Tests/Functional/DataHandling/ManyToMany/Publish/DataSet/deleteContentOfRelation.csv
index 85a067de1f03..c78ded3795ca 100644
--- a/typo3/sysext/workspaces/Tests/Functional/DataHandling/ManyToMany/Publish/DataSet/deleteContentOfRelation.csv
+++ b/typo3/sysext/workspaces/Tests/Functional/DataHandling/ManyToMany/Publish/DataSet/deleteContentOfRelation.csv
@@ -31,4 +31,12 @@
,"uid","pid","sorting","deleted","sys_language_uid","l18n_parent","t3ver_wsid","t3ver_state","t3ver_stage","t3ver_oid","header","image","categories",,,,
,297,89,256,0,0,0,0,0,0,0,"Regular Element #1",0,2,,,,
,298,89,512,1,0,0,0,0,0,0,"Regular Element #2",0,2,,,,
-# @todo: sys_refindex checking. temporarily dropped here since mariadb vs. postgres & sqlite are different
+"sys_refindex",,,,,,,,,,,,,,,,,
+,"hash","tablename","recuid","field","flexpointer","softref_key","softref_id","sorting","workspace","ref_table","ref_uid","ref_string",,,,,
+,"3c637501ab9c158daa933643bff8cc43","sys_category",28,"items",,,,0,0,"tt_content",297,,,,,,
+,"e19100d609a435906e16432a41b55c72","sys_category",29,"items",,,,0,0,"tt_content",297,,,,,,
+,"aabda97b66f9b693f30d1faf442b37d6","sys_category",29,"items",,,,1,0,"tt_content",298,,,,,,
+,"b102e2f9b1ed99b14d813363199cb281","sys_category",30,"items",,,,0,0,"tt_content",298,,,,,,
+,"1b70a8e25c22645f7a49a79f57f3cf3f","sys_category",31,"parent",,,,0,0,"sys_category",28,,,,,,
+,"25426f92d44dd2ccf416108462b446e3","sys_workspace",1,"custom_stages",,,,0,0,"sys_workspace_stage",1,,,,,,
+,"01a3ce8c4e3b2bb1aa439dc29081f996","sys_workspace_stage",1,"responsible_persons",,,,0,0,"be_users",3,,,,,,
diff --git a/typo3/sysext/workspaces/Tests/Functional/DataHandling/ManyToMany/PublishAll/DataSet/deleteContentOfRelation.csv b/typo3/sysext/workspaces/Tests/Functional/DataHandling/ManyToMany/PublishAll/DataSet/deleteContentOfRelation.csv
index 85a067de1f03..c78ded3795ca 100644
--- a/typo3/sysext/workspaces/Tests/Functional/DataHandling/ManyToMany/PublishAll/DataSet/deleteContentOfRelation.csv
+++ b/typo3/sysext/workspaces/Tests/Functional/DataHandling/ManyToMany/PublishAll/DataSet/deleteContentOfRelation.csv
@@ -31,4 +31,12 @@
,"uid","pid","sorting","deleted","sys_language_uid","l18n_parent","t3ver_wsid","t3ver_state","t3ver_stage","t3ver_oid","header","image","categories",,,,
,297,89,256,0,0,0,0,0,0,0,"Regular Element #1",0,2,,,,
,298,89,512,1,0,0,0,0,0,0,"Regular Element #2",0,2,,,,
-# @todo: sys_refindex checking. temporarily dropped here since mariadb vs. postgres & sqlite are different
+"sys_refindex",,,,,,,,,,,,,,,,,
+,"hash","tablename","recuid","field","flexpointer","softref_key","softref_id","sorting","workspace","ref_table","ref_uid","ref_string",,,,,
+,"3c637501ab9c158daa933643bff8cc43","sys_category",28,"items",,,,0,0,"tt_content",297,,,,,,
+,"e19100d609a435906e16432a41b55c72","sys_category",29,"items",,,,0,0,"tt_content",297,,,,,,
+,"aabda97b66f9b693f30d1faf442b37d6","sys_category",29,"items",,,,1,0,"tt_content",298,,,,,,
+,"b102e2f9b1ed99b14d813363199cb281","sys_category",30,"items",,,,0,0,"tt_content",298,,,,,,
+,"1b70a8e25c22645f7a49a79f57f3cf3f","sys_category",31,"parent",,,,0,0,"sys_category",28,,,,,,
+,"25426f92d44dd2ccf416108462b446e3","sys_workspace",1,"custom_stages",,,,0,0,"sys_workspace_stage",1,,,,,,
+,"01a3ce8c4e3b2bb1aa439dc29081f996","sys_workspace_stage",1,"responsible_persons",,,,0,0,"be_users",3,,,,,,
--
GitLab