Fix removal of still used files

Classes/Service/Cleanup/Files.php

@@ -135,44 +135,13 @@ private function markFileReferencesDeletedIfForeignRecordIsMissing(): void
 
  private function deleteFilesWithoutProperReference(): void
  {
- $queryBuilder = $this->connectionPool->getQueryBuilderForTable('sys_file');
- $queryBuilder->getRestrictions()->removeAll();
- $queryBuilder
- ->select('file.identifier', 'file.storage', 'file.uid')
- ->addSelectLiteral('SUM(' . $queryBuilder->expr()->eq('reference.deleted', 1) . ') AS deleted_sum')
- ->from('sys_file', 'file')
- ->leftJoin(
- 'file',
- 'sys_file_reference',
- 'reference',
- 'reference.uid_local = file.uid'
- )
- ->where($queryBuilder->expr()->like(
- 'reference.tablenames',
- $queryBuilder->createNamedParameter('tx_events_domain_model_%')
- ))
- ->orWhere($queryBuilder->expr()->eq(
- 'reference.tablenames',
- $queryBuilder->createNamedParameter('')
- ))
- ->groupBy('file.uid')
- ->having(
- $queryBuilder->expr()->eq(
- 'deleted_sum',
- $queryBuilder->expr()->count('*')
- )
- )
- ;
- /** @var array{int: array{storage: int, identifier: string, uid: int}} $filesToDelete */
- $filesToDelete = $queryBuilder->execute()->fetchAll();
+ $filesToDelete = $this->filterPotentialFilesToDelete($this->getPotentialFilesToDelete());
 
- $uidsToRemove = [];
- foreach ($filesToDelete as $fileToDelete) {
- $this->deleteFromFal((int)$fileToDelete['storage'], (string)$fileToDelete['identifier']);
- $uidsToRemove[] = (int)$fileToDelete['uid'];
+ foreach ($filesToDelete as $file) {
+ $this->deleteFromFal((int)$file['storage'], (string)$file['identifier']);
  }
 
- $this->deleteFromDb(...$uidsToRemove);
+ $this->deleteFromDb(...array_keys($filesToDelete));
  }
 
  private function deleteFromFal(int $storageUid, string $filePath): void
@@ -228,4 +197,79 @@ private function deleteReferences(): void
  ;
  $queryBuilder->execute();
  }
+
+ /**
+ * @return array<int, array{storage: int, identifier: string}> Index is file uid.
+ */
+ private function getPotentialFilesToDelete(): array
+ {
+ $queryBuilder = $this->connectionPool->getQueryBuilderForTable('sys_file');
+ $queryBuilder->getRestrictions()->removeAll();
+ $queryBuilder
+ ->select('file.uid', 'file.storage', 'file.identifier')
+ ->from('sys_file', 'file')
+ ->leftJoin(
+ 'file',
+ 'sys_file_reference',
+ 'reference',
+ 'reference.uid_local = file.uid'
+ )
+ ->where($queryBuilder->expr()->like(
+ 'reference.tablenames',
+ $queryBuilder->createNamedParameter('tx_events_domain_model_%')
+ ))
+ ->orWhere($queryBuilder->expr()->eq(
+ 'reference.tablenames',
+ $queryBuilder->createNamedParameter('')
+ ))
+ ->groupBy('file.uid')
+ ;
+
+ return $queryBuilder->execute()->fetchAllAssociativeIndexed();
+ }
+
+ /**
+ * @param array<int, array{storage: int, identifier: string}> $files
+ * @return array<int, array{storage: int, identifier: string}> Index is file uid.
+ */
+ private function filterPotentialFilesToDelete(array $files): array
+ {
+ $filesToDelete = [];
+ $filesToKeep = [];
+
+ $queryBuilder = $this->connectionPool->getQueryBuilderForTable('sys_file');
+ $queryBuilder->getRestrictions()->removeAll();
+ $queryBuilder
+ ->select('*')
+ ->from('sys_file_reference', 'reference')
+ ->where($queryBuilder->expr()->in(
+ 'uid_local',
+ $queryBuilder->createNamedParameter(array_keys($files), Connection::PARAM_INT_ARRAY)
+ ))
+ ;
+
+ foreach ($queryBuilder->execute() as $reference) {
+ $file = [];
+ $fileUid = (int) $reference['uid_local'];
+
+ if (
+ (
+ str_starts_with($reference['tablenames'], 'tx_events_domain_model_')
+ || $reference['tablenames'] === ''
+ ) && $reference['deleted'] == 1
+ ) {
+ $file = $files[$fileUid] ?? [];
+ } else {
+ $filesToKeep[$fileUid] = $fileUid;
+ }
+
+ if ($file === []) {
+ continue;
+ }
+
+ $filesToDelete[$fileUid] = $file;
+ }
+
+ return array_diff_key($filesToDelete, $filesToKeep);
+ }
 }

Documentation/Changelog/3.4.0.rst

@@ -64,6 +64,11 @@ Fixes
  The time range where this can happen is now reduced as slugs for each event and
  date is generated right after saving each of them.
 
+* Improve deletion of files and their relations.
+ The used database query didn't work as expected and could result in data loss.
+ There are now two database queries and the logic is moved to PHP.
+ Furthermore, the test cases were extended with another situation.
+
 Tasks
 -----
 

Tests/Functional/Cleanup/Fixtures/RemovePastTestDatabase.php

@@ -7,6 +7,7 @@
  'uid' => '1',
  'title' => 'Root page',
  'slug' => '1',
+ 'media' => '1',
  ],
  [
  'pid' => '1',
@@ -184,6 +185,26 @@
  'identifier_hash' => '475768e491580fb8b74ed36c2b1aaf619ca5e11d',
  'folder_hash' => 'b4ab666a114d9905a50606d1837b74d952dfd90f',
  ],
+ [
+ 'uid' => '6',
+ 'pid' => '0',
+ 'tstamp' => '1371467047',
+ 'type' => '2',
+ 'storage' => '1',
+ 'identifier' => '/user_uploads/example-events-image-used-somewhere-else.gif',
+ 'extension' => 'gif',
+ 'mime_type' => 'image/gif',
+ 'name' => 'example-events-image-used-somewhere-else.gif',
+ 'sha1' => '359ae0fb420fe8afe1a8b8bc5e46d75090a826b9',
+ 'size' => '637',
+ 'creation_date' => '1370877201',
+ 'modification_date' => '1369407629',
+ 'last_indexed' => '0',
+ 'missing' => '0',
+ 'metadata' => '0',
+ 'identifier_hash' => '475768e491580fb8b74ed36c2b1aaf619ca5e11d',
+ 'folder_hash' => 'b4ab666a114d9905a50606d1837b74d952dfd90f',
+ ],
  ],
  'sys_file_metadata' => [
  [
@@ -226,6 +247,14 @@
  'cruser_id' => '1',
  'file' => '5',
  ],
+ [
+ 'uid' => '6',
+ 'pid' => '0',
+ 'tstamp' => '1371467047',
+ 'crdate' => '1371467047',
+ 'cruser_id' => '1',
+ 'file' => '6',
+ ],
  ],
  'sys_file_reference' => [
  [
@@ -324,6 +353,38 @@
  'sorting_foreign' => '0',
  'table_local' => 'sys_file',
  ],
+ [
+ 'uid' => '7',
+ 'pid' => '0',
+ 'tstamp' => '1373537480',
+ 'crdate' => '1371484347',
+ 'cruser_id' => '1',
+ 'deleted' => '0',
+ 'hidden' => '0',
+ 'sys_language_uid' => '0',
+ 'uid_local' => '6',
+ 'uid_foreign' => '0',
+ 'tablenames' => 'pages',
+ 'fieldname' => 'media',
+ 'sorting_foreign' => '0',
+ 'table_local' => 'sys_file',
+ ],
+ [
+ 'uid' => '8',
+ 'pid' => '0',
+ 'tstamp' => '1373537480',
+ 'crdate' => '1371484347',
+ 'cruser_id' => '1',
+ 'deleted' => '0',
+ 'hidden' => '0',
+ 'sys_language_uid' => '0',
+ 'uid_local' => '6',
+ 'uid_foreign' => '1',
+ 'tablenames' => 'tx_events_domain_model_event',
+ 'fieldname' => 'images',
+ 'sorting_foreign' => '2',
+ 'table_local' => 'sys_file',
+ ],
  ],
  'tx_events_domain_model_region' => [
  [

Tests/Functional/Cleanup/RemovePastTest.php

@@ -70,26 +70,27 @@ public function removesPastData(): void
  );
 
  self::assertCount(
- 3,
+ 4,
  $this->getAllRecords('sys_file'),
  'Unexpected number of sys_file records.'
  );
  self::assertCount(
- 3,
+ 4,
  $this->getAllRecords('sys_file_reference'),
  'Unexpected number of sys_file_reference records.'
  );
  self::assertCount(
- 3,
+ 4,
  $this->getAllRecords('sys_file_metadata'),
  'Unexpected number of sys_file_metadata records.'
  );
 
  $files = GeneralUtility::getFilesInDir('fileadmin/user_uploads');
  self::assertIsArray($files, 'Failed to retrieve files from filesystem.');
- self::assertCount(3, $files, 'Unexpected number of files in filesystem.');
- self::assertSame('example-for-future-event.gif', array_values($files)[0], 'Unexpected file in filesystem.');
- self::assertSame('example-for-partner.gif', array_values($files)[1], 'Unexpected file in filesystem.');
- self::assertSame('example-to-keep.gif', array_values($files)[2], 'Unexpected file in filesystem.');
+ self::assertCount(4, $files, 'Unexpected number of files in filesystem.');
+ self::assertSame('example-events-image-used-somewhere-else.gif', array_values($files)[0], 'Unexpected file in filesystem.');
+ self::assertSame('example-for-future-event.gif', array_values($files)[1], 'Unexpected file in filesystem.');
+ self::assertSame('example-for-partner.gif', array_values($files)[2], 'Unexpected file in filesystem.');
+ self::assertSame('example-to-keep.gif', array_values($files)[3], 'Unexpected file in filesystem.');
  }
 }

phpstan-baseline.neon

@@ -45,6 +45,26 @@ parameters:
  count: 1
  path: Classes/Service/Cleanup/Files.php
 
+ -
+ message: "#^Argument of an invalid type Doctrine\\\\DBAL\\\\Result\\|int supplied for foreach, only iterables are supported\\.$#"
+ count: 1
+ path: Classes/Service/Cleanup/Files.php
+
+ -
+ message: "#^Argument of an invalid type Doctrine\\\\DBAL\\\\Driver\\\\ResultStatement\\|int supplied for foreach, only iterables are supported\\.$#"
+ count: 1
+ path: Classes/Service/Cleanup/Files.php
+
+ -
+ message: "#^Cannot call method fetchAllAssociativeIndexed\\(\\) on Doctrine\\\\DBAL\\\\Result\\|int\\.$#"
+ count: 1
+ path: Classes/Service/Cleanup/Files.php
+
+ -
+ message: "#^Method Wrm\\\\Events\\\\Service\\\\Cleanup\\\\Files\\:\\:getPotentialFilesToDelete\\(\\) should return array\\<int, array\\{storage\\: int, identifier\\: string\\}\\> but returns array\\<int\\|string, array\\<string, mixed\\>\\>\\.$#"
+ count: 1
+ path: Classes/Service/Cleanup/Files.php
+
  -
  message: "#^Method Wrm\\\\Events\\\\Service\\\\DestinationDataImportService\\\\CategoriesAssignment\\:\\:getCategories\\(\\) should return TYPO3\\\\CMS\\\\Extbase\\\\Persistence\\\\ObjectStorage\\<Wrm\\\\Events\\\\Domain\\\\Model\\\\Category\\> but returns TYPO3\\\\CMS\\\\Extbase\\\\Persistence\\\\ObjectStorage\\<mixed\\>\\.$#"
  count: 2
@@ -65,6 +85,11 @@ parameters:
  count: 1
  path: Classes/Service/DestinationDataImportService/FilesAssignment.php
 
+ -
+ message: "#^Cannot call method fetchAllAssociativeIndexed\\(\\) on Doctrine\\\\DBAL\\\\Driver\\\\ResultStatement\\|int\\.$#"
+ count: 1
+ path: Classes/Service/Cleanup/Files.php
+
  -
  message: "#^Argument of an invalid type Doctrine\\\\DBAL\\\\Result\\|int supplied for foreach, only iterables are supported\\.$#"
  count: 1