[TASK] Improve overall recycler performance 02/56102/11
authorAndreas Fernandez <a.fernandez@scripting-base.de>
Sun, 11 Mar 2018 21:09:06 +0000 (22:09 +0100)
committerJan Helke <typo3@helke.de>
Fri, 1 Jun 2018 16:05:34 +0000 (18:05 +0200)
To improve the overall performance of the recycler, these things are done:

- Improve how permissions are checked for each record
  Instead of running multiple SQL requests per record, the check now
  instantly stops if the user is either an admin, or has no permission
  to modify a certain table.

- Drop sorting of records by page tree structure
  The records get sorted by the page tree structure, to mime the tree in
  a flat view. However, this feature is rather useless and also
  considered buggy in a huge record set.

Resolves: #84711
Releases: master, 8.7
Change-Id: I0c5177546489ce2a0ba84435fed3879267a5a871
Reviewed-on: https://review.typo3.org/56102
Reviewed-by: Anja Leichsenring <aleichsenring@ab-softlab.de>
Tested-by: Anja Leichsenring <aleichsenring@ab-softlab.de>
Tested-by: TYPO3com <no-reply@typo3.com>
Reviewed-by: Mona Muzaffar <mona.muzaffar@gmx.de>
Tested-by: Mona Muzaffar <mona.muzaffar@gmx.de>
Reviewed-by: Jan Helke <typo3@helke.de>
Tested-by: Jan Helke <typo3@helke.de>
typo3/sysext/recycler/Classes/Controller/DeletedRecordsController.php
typo3/sysext/recycler/Classes/Controller/RecyclerAjaxController.php
typo3/sysext/recycler/Classes/Domain/Model/DeletedRecords.php
typo3/sysext/recycler/Classes/Domain/Model/Tables.php
typo3/sysext/recycler/Classes/Utility/RecyclerUtility.php
typo3/sysext/recycler/Resources/Private/Partials/RecordsTable/DeletedRecord.html
typo3/sysext/recycler/Tests/Functional/Recycle/Pages/DataSet/Assertion/deletedPage-3_4_5_7.xml
typo3/sysext/recycler/Tests/Unit/Domain/Model/DeletedRecordsTest.php [deleted file]

index d6554d3..aacbcf3 100644 (file)
@@ -50,12 +50,10 @@ class DeletedRecordsController
      * Transforms the rows for the deleted records
      *
      * @param array $deletedRowsArray Array with table as key and array with all deleted rows
-     * @param int $totalDeleted Number of deleted records in total
      * @return array JSON array
      */
-    public function transform($deletedRowsArray, $totalDeleted)
+    public function transform($deletedRowsArray)
     {
-        $total = 0;
         $jsonArray = [
             'rows' => []
         ];
@@ -65,11 +63,9 @@ class DeletedRecordsController
             $iconFactory = GeneralUtility::makeInstance(IconFactory::class);
 
             foreach ($deletedRowsArray as $table => $rows) {
-                $total += count($deletedRowsArray[$table]);
                 foreach ($rows as $row) {
                     $pageTitle = $this->getPageTitle((int)$row['pid']);
                     $backendUserName = $this->getBackendUser((int)$row[$GLOBALS['TCA'][$table]['ctrl']['cruser_id']]);
-
                     $userIdWhoDeleted = $this->getUserWhoDeleted($table, (int)$row['uid']);
 
                     $jsonArray['rows'][] = [
@@ -92,7 +88,6 @@ class DeletedRecordsController
                 }
             }
         }
-        $jsonArray['total'] = $totalDeleted;
         return $jsonArray;
     }
 
index 57eb2de..690e873 100644 (file)
@@ -97,7 +97,7 @@ class RecyclerAjaxController
 
                 /* @var $controller DeletedRecordsController */
                 $controller = GeneralUtility::makeInstance(DeletedRecordsController::class);
-                $recordsArray = $controller->transform($deletedRowsArray, $totalDeleted);
+                $recordsArray = $controller->transform($deletedRowsArray);
 
                 $allowDelete = $this->getBackendUser()->isAdmin()
                     ?: (bool)($this->getBackendUser()->getTSConfig()['mod.']['recycler.']['allowDelete'] ?? false);
@@ -105,10 +105,9 @@ class RecyclerAjaxController
                 $view->setTemplatePathAndFilename($extPath . 'Resources/Private/Templates/Ajax/RecordsTable.html');
                 $view->assign('records', $recordsArray['rows']);
                 $view->assign('allowDelete', $allowDelete);
-                $view->assign('total', $recordsArray['total']);
                 $content = [
                     'rows' => $view->render(),
-                    'totalItems' => $recordsArray['total']
+                    'totalItems' => $totalDeleted
                 ];
                 break;
             case 'undoRecords':
index d543913..dac6e7c 100644 (file)
@@ -243,8 +243,6 @@ class DeletedRecords
 
             if ($recordsToCheck !== false) {
                 $this->checkRecordAccess($table, $recordsToCheck);
-                $pidList = $this->getTreeList($id, $depth);
-                $this->sortDeletedRowsByPidList($pidList);
             }
         }
         $this->label[$table] = $tcaCtrl['label'];
@@ -341,27 +339,6 @@ 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
      ************************************************************/
index e4a59f4..3c1cfff 100644 (file)
@@ -36,6 +36,7 @@ class Tables
         $lang = $this->getLanguageService();
         $tables = [];
         $connection = GeneralUtility::makeInstance(ConnectionPool::class);
+
         foreach (RecyclerUtility::getModifyableTables() as $tableName) {
             $deletedField = RecyclerUtility::getDeletedField($tableName);
             if ($deletedField) {
index c575305..1fe5231 100644 (file)
@@ -34,13 +34,21 @@ class RecyclerUtility
      * as well as the table access rights of the user.
      *
      * @param string $table The table to check access for
-     * @param string $row Record array
+     * @param array $row Record array
      * @return bool Returns TRUE is the user has access, or FALSE if not
      */
     public static function checkAccess($table, $row)
     {
         $backendUser = static::getBackendUser();
 
+        if ($backendUser->isAdmin()) {
+            return true;
+        }
+
+        if (!$backendUser->check('tables_modify', $table)) {
+            return false;
+        }
+
         // Checking if the user has permissions? (Only working as a precaution, because the final permission check is always down in TCE. But it's good to notify the user on beforehand...)
         // First, resetting flags.
         $hasAccess = false;
@@ -60,9 +68,6 @@ class RecyclerUtility
                 $hasAccess = $backendUser->recordEditAccessInternals($table, $calcPRec);
             }
         }
-        if (!$backendUser->check('tables_modify', $table)) {
-            $hasAccess = false;
-        }
         return $hasAccess;
     }
 
index 2f84bfa..7dc56e0 100644 (file)
@@ -30,7 +30,7 @@
        </td>
 </tr>
 <tr class="collapse" id="{record.table}_{record.uid}">
-       <td colspan="6">
+       <td colspan="7">
                <table class="table">
                        <thead>
                                <tr>
index 4de2198..4b20657 100644 (file)
                <perms_everybody>15</perms_everybody>
        </pages>
        <pages>
-               <uid>7</uid>
-               <pid>1</pid>
-               <title>Dummy 1-7 (deleted)</title>
-               <doktype>1</doktype>
-               <deleted>1</deleted>
-               <perms_everybody>0</perms_everybody>
-       </pages>
-       <pages>
                <uid>5</uid>
                <pid>4</pid>
                <title>Dummy 1-4-5 (deleted)</title>
                <deleted>1</deleted>
                <perms_everybody>15</perms_everybody>
        </pages>
+
+       <pages>
+               <uid>7</uid>
+               <pid>1</pid>
+               <title>Dummy 1-7 (deleted)</title>
+               <doktype>1</doktype>
+               <deleted>1</deleted>
+               <perms_everybody>0</perms_everybody>
+       </pages>
 </dataset>
 
diff --git a/typo3/sysext/recycler/Tests/Unit/Domain/Model/DeletedRecordsTest.php b/typo3/sysext/recycler/Tests/Unit/Domain/Model/DeletedRecordsTest.php
deleted file mode 100644 (file)
index d5ce5fd..0000000
+++ /dev/null
@@ -1,73 +0,0 @@
-<?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());
-    }
-}