From 65038c09ede3023c308cab3c8709d945def9c4d7 Mon Sep 17 00:00:00 2001 From: Daniel Siepmann Date: Tue, 20 Jun 2023 10:53:18 +0200 Subject: [PATCH] Fix removal of still used files Simplify SQL queries and move logic to PHP. --- Classes/Service/Cleanup/Files.php | 114 ++++++++++++------ Documentation/Changelog/3.4.0.rst | 5 + .../Fixtures/RemovePastTestDatabase.php | 61 ++++++++++ ...ample-events-image-used-somewhere-else.gif | 0 Tests/Functional/Cleanup/RemovePastTest.php | 15 +-- phpstan-baseline.neon | 25 ++++ 6 files changed, 178 insertions(+), 42 deletions(-) create mode 100644 Tests/Functional/Cleanup/Fixtures/RemovePastTestFileadmin/user_uploads/example-events-image-used-somewhere-else.gif diff --git a/Classes/Service/Cleanup/Files.php b/Classes/Service/Cleanup/Files.php index d1041d4..b90f01b 100644 --- a/Classes/Service/Cleanup/Files.php +++ b/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 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 $files + * @return array 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); + } } diff --git a/Documentation/Changelog/3.4.0.rst b/Documentation/Changelog/3.4.0.rst index 16f586f..32fd3d5 100644 --- a/Documentation/Changelog/3.4.0.rst +++ b/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 ----- diff --git a/Tests/Functional/Cleanup/Fixtures/RemovePastTestDatabase.php b/Tests/Functional/Cleanup/Fixtures/RemovePastTestDatabase.php index 6ac09de..13ce248 100644 --- a/Tests/Functional/Cleanup/Fixtures/RemovePastTestDatabase.php +++ b/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' => [ [ diff --git a/Tests/Functional/Cleanup/Fixtures/RemovePastTestFileadmin/user_uploads/example-events-image-used-somewhere-else.gif b/Tests/Functional/Cleanup/Fixtures/RemovePastTestFileadmin/user_uploads/example-events-image-used-somewhere-else.gif new file mode 100644 index 0000000..e69de29 diff --git a/Tests/Functional/Cleanup/RemovePastTest.php b/Tests/Functional/Cleanup/RemovePastTest.php index bf71b68..3e92355 100644 --- a/Tests/Functional/Cleanup/RemovePastTest.php +++ b/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.'); } } diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon index f17ea91..11c86bf 100644 --- a/phpstan-baseline.neon +++ b/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\\ but returns array\\\\>\\.$#" + count: 1 + path: Classes/Service/Cleanup/Files.php + - message: "#^Method Wrm\\\\Events\\\\Service\\\\DestinationDataImportService\\\\CategoriesAssignment\\:\\:getCategories\\(\\) should return TYPO3\\\\CMS\\\\Extbase\\\\Persistence\\\\ObjectStorage\\ but returns TYPO3\\\\CMS\\\\Extbase\\\\Persistence\\\\ObjectStorage\\\\.$#" 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