[BUGFIX] Refactor record querying in deep nested structures in recycler 72/55472/11
authorAndreas Fernandez <a.fernandez@scripting-base.de>
Sat, 27 Jan 2018 23:14:11 +0000 (00:14 +0100)
committerChristian Kuhn <lolli@schwarzbu.ch>
Tue, 20 Feb 2018 19:31:44 +0000 (20:31 +0100)
This patch refactors how the recycler queries records in deep page
structures.
Instead of resolving each level of the page tree on demand and executing
the complex logic of creating query builders and paging all over again,
a list of page ids is generated once and stored in the Caching Framework
for the current request. This list is then used in an multiple `IN()`
statements (depending on the DBMS engine used).

Also, some wrong type hints and a wrong language file reference are
fixed.

Resolves: #83702
Releases: master, 8.7
Change-Id: Ie3314b5a9209fb5585f95a70c16b35639951c197
Reviewed-on: https://review.typo3.org/55472
Reviewed-by: Mathias Schreiber <mathias.schreiber@typo3.com>
Tested-by: Mathias Schreiber <mathias.schreiber@typo3.com>
Tested-by: TYPO3com <no-reply@typo3.com>
Reviewed-by: Christian Kuhn <lolli@schwarzbu.ch>
Tested-by: Christian Kuhn <lolli@schwarzbu.ch>
typo3/sysext/recycler/Classes/Controller/DeletedRecordsController.php
typo3/sysext/recycler/Classes/Domain/Model/DeletedRecords.php
typo3/sysext/recycler/Classes/Domain/Model/Tables.php
typo3/sysext/recycler/Tests/Unit/Domain/Model/DeletedRecordsTest.php [new file with mode: 0644]

index b3f777c..d6554d3 100644 (file)
@@ -16,6 +16,7 @@ namespace TYPO3\CMS\Recycler\Controller;
  */
 
 use TYPO3\CMS\Backend\Utility\BackendUtility;
+use TYPO3\CMS\Core\Cache\CacheManager;
 use TYPO3\CMS\Core\Database\ConnectionPool;
 use TYPO3\CMS\Core\DataHandling\DataHandler;
 use TYPO3\CMS\Core\History\RecordHistoryStore;
@@ -50,7 +51,7 @@ class DeletedRecordsController
      *
      * @param array $deletedRowsArray Array with table as key and array with all deleted rows
      * @param int $totalDeleted Number of deleted records in total
-     * @return string JSON array
+     * @return array JSON array
      */
     public function transform($deletedRowsArray, $totalDeleted)
     {
@@ -189,11 +190,11 @@ class DeletedRecordsController
     /**
      * Create and returns an instance of the CacheManager
      *
-     * @return \TYPO3\CMS\Core\Cache\CacheManager
+     * @return CacheManager
      */
     protected function getCacheManager()
     {
-        return GeneralUtility::makeInstance(\TYPO3\CMS\Core\Cache\CacheManager::class);
+        return GeneralUtility::makeInstance(CacheManager::class);
     }
 
     /**
index 28239ea..59c8777 100644 (file)
@@ -14,10 +14,17 @@ namespace TYPO3\CMS\Recycler\Domain\Model;
  * The TYPO3 project - inspiring people to share!
  */
 
+use TYPO3\CMS\Core\Authentication\BackendUserAuthentication;
+use TYPO3\CMS\Core\Cache\CacheManager;
+use TYPO3\CMS\Core\Cache\Frontend\FrontendInterface;
+use TYPO3\CMS\Core\Database\Connection;
 use TYPO3\CMS\Core\Database\ConnectionPool;
+use TYPO3\CMS\Core\Database\Platform\PlatformInformation;
 use TYPO3\CMS\Core\Database\Query\QueryBuilder;
+use TYPO3\CMS\Core\Database\Query\QueryHelper;
 use TYPO3\CMS\Core\Database\Query\Restriction\BackendWorkspaceRestriction;
 use TYPO3\CMS\Core\DataHandling\DataHandler;
+use TYPO3\CMS\Core\Type\Bitmask\Permission;
 use TYPO3\CMS\Core\Utility\GeneralUtility;
 use TYPO3\CMS\Core\Utility\MathUtility;
 use TYPO3\CMS\Recycler\Utility\RecyclerUtility;
@@ -95,7 +102,7 @@ class DeletedRecords
                 $this->setData($id, $table, $depth, $filter);
             }
         } else {
-            foreach (array_keys($GLOBALS['TCA']) as $tableKey) {
+            foreach (RecyclerUtility::getModifyableTables() as $tableKey) {
                 // only go into this table if the limit allows it
                 if ($this->limit !== '') {
                     $parts = GeneralUtility::intExplode(',', $this->limit, true);
@@ -140,20 +147,20 @@ class DeletedRecords
      */
     protected function setData($id, $table, $depth, $filter)
     {
-        if (!array_key_exists('delete', $GLOBALS['TCA'][$table]['ctrl'])) {
+        $deletedField = RecyclerUtility::getDeletedField($table);
+        if (!$deletedField) {
             return;
         }
 
         $id = (int)$id;
         $tcaCtrl = $GLOBALS['TCA'][$table]['ctrl'];
-        $deletedField = RecyclerUtility::getDeletedField($table);
         $firstResult = 0;
         $maxResults = 0;
 
         // get the limit
         if (!empty($this->limit)) {
             // count the number of deleted records for this pid
-            $queryBuilder = $this->getFilteredQueryBuilder($table, $id, $filter);
+            $queryBuilder = $this->getFilteredQueryBuilder($table, $id, $depth, $filter);
             $queryBuilder->getRestrictions()->removeAll();
 
             $deletedCount = (int)$queryBuilder
@@ -179,7 +186,6 @@ class DeletedRecords
                 $this->limit = implode(',', [$offset, $rowCount]);
                 // do NOT query this depth; limit also does not need to be set, we set it anyways
                 $allowQuery = false;
-                $allowDepth = true;
             } else {
                 // the offset for the temporary limit has to remain like the original offset
                 // in case the original offset was just crossed by the amount of deleted records
@@ -198,8 +204,6 @@ class DeletedRecords
                     $maxResults = $rowCount;
                     // set the limit's row count to 0
                     $this->limit = implode(',', [$newOffset, 0]);
-                    // do not go into new depth
-                    $allowDepth = false;
                 } else {
                     // if the result now is <= limit's row count
                     // use the result as the temporary limit
@@ -209,26 +213,16 @@ class DeletedRecords
                     $newCount = $rowCount - $absResult;
                     // store the new result in the limit's row count
                     $this->limit = implode(',', [$newOffset, $newCount]);
-                    // if the new row count is > 0
-                    if ($newCount > 0) {
-                        // go into new depth
-                        $allowDepth = true;
-                    } else {
-                        // if the new row count is <= 0 (only =0 makes sense though)
-                        // do not go into new depth
-                        $allowDepth = false;
-                    }
                 }
                 // allow query for this depth
                 $allowQuery = true;
             }
         } else {
-            $allowDepth = true;
             $allowQuery = true;
         }
         // query for actual deleted records
         if ($allowQuery) {
-            $queryBuilder = $this->getFilteredQueryBuilder($table, $id, $filter);
+            $queryBuilder = $this->getFilteredQueryBuilder($table, $id, $depth, $filter);
             if ($firstResult) {
                 $queryBuilder->setFirstResult($firstResult);
             }
@@ -239,10 +233,6 @@ class DeletedRecords
                 ->from($table)
                 ->andWhere(
                     $queryBuilder->expr()->eq(
-                        'pid',
-                        $queryBuilder->createNamedParameter($id, \PDO::PARAM_INT)
-                    ),
-                    $queryBuilder->expr()->eq(
                         $deletedField,
                         $queryBuilder->createNamedParameter(1, \PDO::PARAM_INT)
                     )
@@ -253,31 +243,8 @@ class DeletedRecords
 
             if ($recordsToCheck !== false) {
                 $this->checkRecordAccess($table, $recordsToCheck);
-            }
-        }
-        // go into depth
-        if ($allowDepth && $depth >= 1) {
-            // check recursively for elements beneath this page
-            $queryBuilder = GeneralUtility::makeInstance(ConnectionPool::class)->getQueryBuilderForTable('pages');
-            $queryBuilder->getRestrictions()->removeAll();
-            $resPages = $queryBuilder
-                ->select('uid')
-                ->from('pages')
-                ->where($queryBuilder->expr()->eq('pid', $queryBuilder->createNamedParameter($id, \PDO::PARAM_INT)))
-                ->orderBy('sorting')
-                ->execute();
-
-            while ($row = $resPages->fetch()) {
-                $this->setData($row['uid'], $table, $depth - 1, $filter);
-                // some records might have been added, check if we still have the limit for further queries
-                if (!empty($this->limit)) {
-                    $parts = GeneralUtility::intExplode(',', $this->limit, true);
-                    // abort loop if LIMIT 0,0
-                    if ($parts[0] === 0 && $parts[1] === 0) {
-                        $resPages->closeCursor();
-                        break;
-                    }
-                }
+                $pidList = $this->getTreeList($id, $depth);
+                $this->sortDeletedRowsByPidList($pidList);
             }
         }
         $this->label[$table] = $tcaCtrl['label'];
@@ -289,12 +256,13 @@ class DeletedRecords
      *
      * @param string $table
      * @param int $pid
+     * @param int $depth
      * @param string $filter
      * @return \TYPO3\CMS\Core\Database\Query\QueryBuilder
-     * @throws \InvalidArgumentException
      */
-    protected function getFilteredQueryBuilder(string $table, int $pid, string $filter): QueryBuilder
+    protected function getFilteredQueryBuilder(string $table, int $pid, int $depth, string $filter): QueryBuilder
     {
+        $pidList = $this->getTreeList($pid, $depth);
         $queryBuilder = GeneralUtility::makeInstance(ConnectionPool::class)->getQueryBuilderForTable($table);
         $queryBuilder->getRestrictions()
             ->removeAll()
@@ -325,9 +293,19 @@ class DeletedRecords
             }
         }
 
+        $maxBindParameters = PlatformInformation::getMaxBindParameters($queryBuilder->getConnection()->getDatabasePlatform());
+        $pidConstraints = [];
+        foreach (array_chunk($pidList, $maxBindParameters - 10) as $chunk) {
+            $pidConstraints[] = $queryBuilder->expr()->in(
+                'pid',
+                $queryBuilder->createNamedParameter($chunk, Connection::PARAM_INT_ARRAY)
+            );
+        }
         $queryBuilder->where(
-            $queryBuilder->expr()->eq('pid', $queryBuilder->createNamedParameter($pid, \PDO::PARAM_INT)),
-            $filterConstraint
+            $queryBuilder->expr()->andX(
+                $filterConstraint,
+                $queryBuilder->expr()->orX(...$pidConstraints)
+            )
         );
 
         return $queryBuilder;
@@ -341,6 +319,7 @@ class DeletedRecords
      */
     protected function checkRecordAccess($table, array $rows)
     {
+        $deleteField = '';
         if ($table === 'pages') {
             // The "checkAccess" method validates access to the passed table/rows. When access to
             // a page record gets validated it is necessary to disable the "delete" field temporarily
@@ -362,6 +341,27 @@ class DeletedRecords
         }
     }
 
+    /**
+     * @param array $pidList
+     */
+    protected function sortDeletedRowsByPidList(array $pidList)
+    {
+        foreach ($this->deletedRows as $table => $rows) {
+            // Reset array of deleted rows for current table
+            $this->deletedRows[$table] = [];
+
+            // Get rows for current pid
+            foreach ($pidList as $pid) {
+                $rowsForCurrentPid = array_filter($rows, function ($row) use ($pid) {
+                    return (int)$row['pid'] === (int)$pid;
+                });
+
+                // Append sorted records to the array again
+                $this->deletedRows[$table] = array_merge($this->deletedRows[$table], $rowsForCurrentPid);
+            }
+        }
+    }
+
     /************************************************************
      * DELETE FUNCTIONS
      ************************************************************/
@@ -376,7 +376,7 @@ class DeletedRecords
         if (is_array($recordsArray)) {
             /** @var $tce DataHandler **/
             $tce = GeneralUtility::makeInstance(DataHandler::class);
-            $tce->start('', '');
+            $tce->start([], []);
             $tce->disableDeleteClause();
             foreach ($recordsArray as $record) {
                 list($table, $uid) = explode(':', $record);
@@ -513,4 +513,83 @@ class DeletedRecords
     {
         return $this->table;
     }
+
+    /**
+     * Get tree list
+     *
+     * @param int $id
+     * @param int $depth
+     * @param int $begin
+     * @return array
+     */
+    protected function getTreeList(int $id, int $depth, int $begin = 0): array
+    {
+        $cache = $this->getCache();
+        $identifier = md5($id . '_' . $depth . '_' . $begin);
+        $pageTree = $cache->get($identifier);
+        if ($pageTree === false) {
+            $pageTree = $this->resolveTree($id, $depth, $begin, $this->getBackendUser()->getPagePermsClause(Permission::PAGE_SHOW));
+            $cache->set($identifier, $pageTree);
+        }
+
+        return $pageTree;
+    }
+
+    /**
+     * @param $id
+     * @param int $depth
+     * @param int $begin
+     * @param string $permsClause
+     * @return array
+     */
+    protected function resolveTree(int $id, int $depth, int $begin = 0, string $permsClause = ''): array
+    {
+        $depth = (int)$depth;
+        $begin = (int)$begin;
+        $id = abs((int)$id);
+        $theList = [];
+        if ($begin === 0) {
+            $theList[] = $id;
+        }
+        if ($depth > 0) {
+            $queryBuilder = GeneralUtility::makeInstance(ConnectionPool::class)->getQueryBuilderForTable('pages');
+            $queryBuilder->getRestrictions()->removeAll()->add(GeneralUtility::makeInstance(BackendWorkspaceRestriction::class));
+            $statement = $queryBuilder->select('uid')
+                ->from('pages')
+                ->where(
+                    $queryBuilder->expr()->eq('pid', $queryBuilder->createNamedParameter($id, \PDO::PARAM_INT)),
+                    QueryHelper::stripLogicalOperatorPrefix($permsClause)
+                )
+                ->execute();
+            while ($row = $statement->fetch()) {
+                if ($begin <= 0) {
+                    $theList[] = $row['uid'];
+                }
+                if ($depth > 1) {
+                    $theList = array_merge($theList, $this->resolveTree($row['uid'], $depth - 1, $begin - 1, $permsClause));
+                }
+            }
+        }
+        return $theList;
+    }
+
+    /**
+     * Gets an instance of the memory cache.
+     *
+     * @return FrontendInterface
+     */
+    protected function getCache(): FrontendInterface
+    {
+        return GeneralUtility::makeInstance(CacheManager::class)->getCache('cache_runtime');
+    }
+
+    /**
+     * Returns the BackendUser
+     *
+     * @return BackendUserAuthentication
+     */
+    protected function getBackendUser(): BackendUserAuthentication
+    {
+        return $GLOBALS['BE_USER'];
+    }
 }
index c7e91ea..e4a59f4 100644 (file)
@@ -28,7 +28,7 @@ class Tables
      *
      * @param int $startUid UID from selected page
      * @param int $depth How many levels recursive
-     * @return string The tables to be displayed
+     * @return array The tables to be displayed
      */
     public function getTables($startUid, $depth = 0)
     {
@@ -64,7 +64,7 @@ class Tables
                             $tables[] = [
                                 $tableName,
                                 $deletedRecordsInTable,
-                                $lang->sL($GLOBALS['TCA'][$tableName]['ctrl']['title'])
+                                $lang->sL($GLOBALS['TCA'][$tableName]['ctrl']['title'] ?? $tableName)
                             ];
                         }
                     }
@@ -75,7 +75,7 @@ class Tables
         array_unshift($jsonArray, [
             '',
             $deletedRecordsTotal,
-            $lang->sL('LLL:EXT:recycler/mod1/locallang.xlf:label_allrecordtypes')
+            $lang->sL('LLL:EXT:recycler/Resources/Private/Language/locallang.xlf:label_allrecordtypes')
         ]);
         return $jsonArray;
     }
diff --git a/typo3/sysext/recycler/Tests/Unit/Domain/Model/DeletedRecordsTest.php b/typo3/sysext/recycler/Tests/Unit/Domain/Model/DeletedRecordsTest.php
new file mode 100644 (file)
index 0000000..d5ce5fd
--- /dev/null
@@ -0,0 +1,73 @@
+<?php
+declare(strict_types = 1);
+namespace TYPO3\CMS\Recycler\Tests\Unit\Domain\Model;
+
+/*
+ * This file is part of the TYPO3 CMS project.
+ *
+ * It is free software; you can redistribute it and/or modify it under
+ * the terms of the GNU General Public License, either version 2
+ * of the License, or any later version.
+ *
+ * For the full copyright and license information, please read the
+ * LICENSE.txt file that was distributed with this source code.
+ *
+ * The TYPO3 project - inspiring people to share!
+ */
+
+use TYPO3\CMS\Recycler\Domain\Model\DeletedRecords;
+use TYPO3\TestingFramework\Core\Unit\UnitTestCase;
+
+/**
+ * Test case
+ */
+class DeletedRecordsTest extends UnitTestCase
+{
+    /**
+     * @test
+     */
+    public function recordsOfMultipleTablesAreSortedByPid()
+    {
+        $deletedRowsData = [
+            'pages' => [
+                ['uid' => 1, 'pid' => 1],
+                ['uid' => 2, 'pid' => 2],
+                ['uid' => 3, 'pid' => 4],
+                ['uid' => 4, 'pid' => 7],
+            ],
+            'sys_template' => [
+                ['uid' => 1, 'pid' => 9],
+                ['uid' => 2, 'pid' => 10],
+                ['uid' => 3, 'pid' => 1],
+            ],
+            'tt_content' => [
+                ['uid' => 1, 'pid' => 7],
+                ['uid' => 2, 'pid' => 1],
+            ]
+        ];
+
+        $expectedRows = [
+            'pages' => [
+                ['uid' => 1, 'pid' => 1],
+                ['uid' => 2, 'pid' => 2],
+                ['uid' => 4, 'pid' => 7],
+                ['uid' => 3, 'pid' => 4],
+            ],
+            'sys_template' => [
+                ['uid' => 3, 'pid' => 1],
+                ['uid' => 2, 'pid' => 10],
+                ['uid' => 1, 'pid' => 9],
+            ],
+            'tt_content' => [
+                ['uid' => 2, 'pid' => 1],
+                ['uid' => 1, 'pid' => 7],
+            ]
+        ];
+
+        /** @var \PHPUnit_Framework_MockObject_MockObject|\TYPO3\TestingFramework\Core\AccessibleObjectInterface|DeletedRecords $subject */
+        $subject = $this->getAccessibleMock(DeletedRecords::class, ['dummy']);
+        $subject->_set('deletedRows', $deletedRowsData);
+        $subject->_call('sortDeletedRowsByPidList', [1, 2, 7, 4, 10, 9]);
+        static::assertEquals($expectedRows, $subject->getDeletedRows());
+    }
+}