[TASK] Improve overall recycler performance 13/57113/2
authorAndreas Fernandez <a.fernandez@scripting-base.de>
Sun, 11 Mar 2018 21:09:06 +0000 (22:09 +0100)
committerOliver Hader <oliver.hader@typo3.org>
Mon, 11 Jun 2018 11:04:18 +0000 (13:04 +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/57113
Tested-by: TYPO3com <no-reply@typo3.com>
Reviewed-by: Andreas Fernandez <a.fernandez@scripting-base.de>
Tested-by: Andreas Fernandez <a.fernandez@scripting-base.de>
Reviewed-by: Mathias Brodala <mbrodala@pagemachine.de>
Tested-by: Mathias Brodala <mbrodala@pagemachine.de>
Reviewed-by: Oliver Hader <oliver.hader@typo3.org>
Tested-by: Oliver Hader <oliver.hader@typo3.org>
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 769a2de..e471f94 100644 (file)
@@ -47,12 +47,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' => []
         ];
@@ -62,7 +60,6 @@ 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']);
                     $backendUser = BackendUtility::getRecord('be_users', $row[$GLOBALS['TCA'][$table]['ctrl']['cruser_id']], 'username', '', false);
@@ -84,7 +81,6 @@ class DeletedRecordsController
                 }
             }
         }
-        $jsonArray['total'] = $totalDeleted;
         return $jsonArray;
     }
 
index 759055b..94066c3 100644 (file)
@@ -98,7 +98,7 @@ class RecyclerAjaxController
 
                 /* @var $controller DeletedRecordsController */
                 $controller = GeneralUtility::makeInstance(DeletedRecordsController::class);
-                $recordsArray = $controller->transform($deletedRowsArray, $totalDeleted);
+                $recordsArray = $controller->transform($deletedRowsArray);
 
                 $modTS = $this->getBackendUser()->getTSConfig('mod.recycler');
                 $allowDelete = $this->getBackendUser()->isAdmin() ? true : (bool)$modTS['properties']['allowDelete'];
@@ -106,10 +106,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 82cc010..9f488e4 100644 (file)
@@ -244,8 +244,6 @@ class DeletedRecords
 
             if ($recordsToCheck !== false) {
                 $this->checkRecordAccess($table, $recordsToCheck);
-                $pidList = $this->getTreeList($id, $depth);
-                $this->sortDeletedRowsByPidList($pidList);
             }
         }
         $this->label[$table] = $tcaCtrl['label'];
@@ -342,27 +340,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 7f57627..2dd1e66 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 d5819ce..7c65c17 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 c42383d..4da2131 100644 (file)
@@ -29,7 +29,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());
-    }
-}