Commit 5a8171f5 authored by Christian Kuhn's avatar Christian Kuhn
Browse files

[BUGFIX] Update CSV references when discarding record

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/+/72297

Tested-by: core-ci's avatarcore-ci <typo3@b13.com>
Tested-by: Christian Kuhn's avatarChristian Kuhn <lolli@schwarzbu.ch>
Reviewed-by: Christian Kuhn's avatarChristian Kuhn <lolli@schwarzbu.ch>
parent fe086012
......@@ -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.
......
......@@ -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();
......
......@@ -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');
......
......@@ -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,,,,,,
......
......@@ -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,,,,,,
......@@ -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,,,,,,
Supports Markdown
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment