From 4677dac58f290eb950547b16a4dcf8ea1a1cd7f1 Mon Sep 17 00:00:00 2001 From: Kamil Tekiela Date: Thu, 28 Dec 2023 15:47:01 +0100 Subject: [PATCH] Refactor Indexes::doSaveData() Signed-off-by: Kamil Tekiela --- app/services.php | 2 +- phpstan-baseline.neon | 9 +-- psalm-baseline.xml | 9 +-- .../Table/IndexRenameController.php | 55 ++++++++++++++++- src/Controllers/Table/IndexesController.php | 59 ++++++++++++++++--- src/Table/Indexes.php | 56 ++---------------- src/Table/Table.php | 8 +-- .../Table/IndexRenameControllerTest.php | 2 +- .../Table/IndexesControllerTest.php | 2 +- .../Table/Structure/SpatialControllerTest.php | 8 +-- .../Table/Structure/UniqueControllerTest.php | 8 +-- tests/classes/Table/IndexesTest.php | 30 ++++------ 12 files changed, 142 insertions(+), 106 deletions(-) diff --git a/app/services.php b/app/services.php index 51de1ca115..993d5e7f97 100644 --- a/app/services.php +++ b/app/services.php @@ -193,7 +193,7 @@ return [ ], 'table_indexes' => [ 'class' => Indexes::class, - 'arguments' => ['$response' => '@response', '$template' => '@template', '$dbi' => '@dbi'], + 'arguments' => ['$dbi' => '@dbi'], ], 'table_maintenance' => ['class' => PhpMyAdmin\Table\Maintenance::class, 'arguments' => ['$dbi' => '@dbi']], 'table_search' => ['class' => Search::class, 'arguments' => ['$dbi' => '@dbi']], diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon index 90752741d6..93a34a0013 100644 --- a/phpstan-baseline.neon +++ b/phpstan-baseline.neon @@ -4931,7 +4931,7 @@ parameters: path: src/Controllers/Table/IndexRenameController.php - - message: "#^Parameter \\#7 \\$oldIndexName of method PhpMyAdmin\\\\Table\\\\Indexes\\:\\:doSaveData\\(\\) expects string, mixed given\\.$#" + message: "#^Parameter \\#6 \\$oldIndexName of method PhpMyAdmin\\\\Table\\\\Indexes\\:\\:doSaveData\\(\\) expects string, mixed given\\.$#" count: 1 path: src/Controllers/Table/IndexRenameController.php @@ -15367,7 +15367,7 @@ parameters: - message: "#^Construct empty\\(\\) is not allowed\\. Use more strict comparison\\.$#" - count: 18 + count: 19 path: src/Table/Table.php - @@ -17910,11 +17910,6 @@ parameters: count: 1 path: tests/classes/Table/IndexesTest.php - - - message: "#^Parameter \\#2 \\$haystack of method PHPUnit\\\\Framework\\\\Assert\\:\\:assertStringContainsString\\(\\) expects string, mixed given\\.$#" - count: 1 - path: tests/classes/Table/IndexesTest.php - - message: "#^Call to an undefined method PhpMyAdmin\\\\DatabaseInterface\\:\\:expects\\(\\)\\.$#" count: 4 diff --git a/psalm-baseline.xml b/psalm-baseline.xml index c63e23a1e9..c141de8376 100644 --- a/psalm-baseline.xml +++ b/psalm-baseline.xml @@ -11787,9 +11787,6 @@ true|Message - - return $sqlQuery; - uiprefs =& $_SESSION['tmpval']['table_uiprefs'][$serverId][$this->dbName][$this->name]]]> @@ -15256,6 +15253,9 @@ $value + + clear + @@ -15275,9 +15275,6 @@ Config::getInstance() DatabaseInterface::getInstance() - - - method willReturn diff --git a/src/Controllers/Table/IndexRenameController.php b/src/Controllers/Table/IndexRenameController.php index 6a5d2a9b46..f528102105 100644 --- a/src/Controllers/Table/IndexRenameController.php +++ b/src/Controllers/Table/IndexRenameController.php @@ -5,10 +5,12 @@ declare(strict_types=1); namespace PhpMyAdmin\Controllers\Table; use PhpMyAdmin\Config; +use PhpMyAdmin\Container\ContainerBuilder; use PhpMyAdmin\Controllers\AbstractController; use PhpMyAdmin\Current; use PhpMyAdmin\DatabaseInterface; use PhpMyAdmin\DbTableExists; +use PhpMyAdmin\Html\Generator; use PhpMyAdmin\Http\ServerRequest; use PhpMyAdmin\Identifiers\DatabaseName; use PhpMyAdmin\Identifiers\TableName; @@ -96,16 +98,63 @@ final class IndexRenameController extends AbstractController if (isset($_POST['do_save_data'])) { $oldIndexName = $request->getParsedBodyParam('old_index', ''); - $this->indexes->doSaveData( - $request, + $previewSql = $request->hasBodyParam('preview_sql'); + + $sqlResult = $this->indexes->doSaveData( $index, true, Current::$database, Current::$table, - $request->hasBodyParam('preview_sql'), + $previewSql, $oldIndexName, ); + // If there is a request for SQL previewing. + if ($previewSql) { + $this->response->addJSON( + 'sql_data', + $this->template->render('preview_sql', ['query_data' => $sqlResult]), + ); + + return; + } + + if ($sqlResult instanceof Message) { + $this->response->setRequestStatus(false); + $this->response->addJSON('message', $sqlResult); + + return; + } + + if ($request->isAjax()) { + $message = Message::success( + __('Table %1$s has been altered successfully.'), + ); + $message->addParam(Current::$table); + $this->response->addJSON( + 'message', + Generator::getMessage($message, $sqlResult, 'success'), + ); + + $indexes = Index::getFromTable($this->dbi, Current::$table, Current::$database); + $indexesDuplicates = Index::findDuplicates(Current::$table, Current::$database); + + $this->response->addJSON( + 'index_table', + $this->template->render('indexes', [ + 'url_params' => ['db' => Current::$database, 'table' => Current::$table], + 'indexes' => $indexes, + 'indexes_duplicates' => $indexesDuplicates, + ]), + ); + + return; + } + + /** @var StructureController $controller */ + $controller = ContainerBuilder::getContainer()->get(StructureController::class); + $controller($request); + return; } diff --git a/src/Controllers/Table/IndexesController.php b/src/Controllers/Table/IndexesController.php index ced8656efb..8353823770 100644 --- a/src/Controllers/Table/IndexesController.php +++ b/src/Controllers/Table/IndexesController.php @@ -5,10 +5,12 @@ declare(strict_types=1); namespace PhpMyAdmin\Controllers\Table; use PhpMyAdmin\Config; +use PhpMyAdmin\Container\ContainerBuilder; use PhpMyAdmin\Controllers\AbstractController; use PhpMyAdmin\Current; use PhpMyAdmin\DatabaseInterface; use PhpMyAdmin\DbTableExists; +use PhpMyAdmin\Html\Generator; use PhpMyAdmin\Http\ServerRequest; use PhpMyAdmin\Identifiers\DatabaseName; use PhpMyAdmin\Identifiers\TableName; @@ -100,14 +102,55 @@ class IndexesController extends AbstractController } if (isset($_POST['do_save_data'])) { - $this->indexes->doSaveData( - $request, - $index, - false, - Current::$database, - Current::$table, - $request->hasBodyParam('preview_sql'), - ); + $previewSql = $request->hasBodyParam('preview_sql'); + + $sqlResult = $this->indexes->doSaveData($index, false, Current::$database, Current::$table, $previewSql); + + if ($sqlResult instanceof Message) { + $this->response->setRequestStatus(false); + $this->response->addJSON('message', $sqlResult); + + return; + } + + // If there is a request for SQL previewing. + if ($previewSql) { + $this->response->addJSON( + 'sql_data', + $this->template->render('preview_sql', ['query_data' => $sqlResult]), + ); + + return; + } + + if ($request->isAjax()) { + $message = Message::success( + __('Table %1$s has been altered successfully.'), + ); + $message->addParam(Current::$table); + $this->response->addJSON( + 'message', + Generator::getMessage($message, $sqlResult, 'success'), + ); + + $indexes = Index::getFromTable($this->dbi, Current::$table, Current::$database); + $indexesDuplicates = Index::findDuplicates(Current::$table, Current::$database); + + $this->response->addJSON( + 'index_table', + $this->template->render('indexes', [ + 'url_params' => ['db' => Current::$database, 'table' => Current::$table], + 'indexes' => $indexes, + 'indexes_duplicates' => $indexesDuplicates, + ]), + ); + + return; + } + + /** @var StructureController $controller */ + $controller = ContainerBuilder::getContainer()->get(StructureController::class); + $controller($request); return; } diff --git a/src/Table/Indexes.php b/src/Table/Indexes.php index f882b3350d..8457486e3a 100644 --- a/src/Table/Indexes.php +++ b/src/Table/Indexes.php @@ -4,26 +4,18 @@ declare(strict_types=1); namespace PhpMyAdmin\Table; -use PhpMyAdmin\Container\ContainerBuilder; -use PhpMyAdmin\Controllers\Table\StructureController; use PhpMyAdmin\DatabaseInterface; -use PhpMyAdmin\Html\Generator; -use PhpMyAdmin\Http\ServerRequest; use PhpMyAdmin\Identifiers\DatabaseName; use PhpMyAdmin\Index; use PhpMyAdmin\Message; use PhpMyAdmin\Query\Compatibility; use PhpMyAdmin\Query\Generator as QueryGenerator; -use PhpMyAdmin\ResponseRenderer; -use PhpMyAdmin\Template; use function __; final class Indexes { public function __construct( - protected ResponseRenderer $response, - protected Template $template, private DatabaseInterface $dbi, ) { } @@ -37,14 +29,13 @@ final class Indexes * @param bool $renameMode Rename the Index mode */ public function doSaveData( - ServerRequest $request, Index $index, bool $renameMode, string $db, string $table, bool $previewSql, string $oldIndexName = '', - ): void { + ): string|Message { $error = false; if ($renameMode && Compatibility::isCompatibleRenameIndex($this->dbi->getVersion())) { if ($oldIndexName === 'PRIMARY') { @@ -64,56 +55,21 @@ final class Indexes $index->getName(), ); } else { - $sqlQuery = $this->dbi->getTable($db, $table) - ->getSqlQueryForIndexCreateOrEdit($index, $error); + $sqlQuery = $this->dbi->getTable($db, $table)->getSqlQueryForIndexCreateOrEdit($index, $error); } // If there is a request for SQL previewing. if ($previewSql) { - $this->response->addJSON( - 'sql_data', - $this->template->render('preview_sql', ['query_data' => $sqlQuery]), - ); - - return; + return $sqlQuery; } - if ($error) { - $this->response->setRequestStatus(false); - $this->response->addJSON('message', $error); - - return; + if ($error instanceof Message) { + return $error; } $this->dbi->query($sqlQuery); - if ($request->isAjax()) { - $message = Message::success( - __('Table %1$s has been altered successfully.'), - ); - $message->addParam($table); - $this->response->addJSON( - 'message', - Generator::getMessage($message, $sqlQuery, 'success'), - ); - $indexes = Index::getFromTable($this->dbi, $table, $db); - $indexesDuplicates = Index::findDuplicates($table, $db); - - $this->response->addJSON( - 'index_table', - $this->template->render('indexes', [ - 'url_params' => ['db' => $db, 'table' => $table], - 'indexes' => $indexes, - 'indexes_duplicates' => $indexesDuplicates, - ]), - ); - - return; - } - - /** @var StructureController $controller */ - $controller = ContainerBuilder::getContainer()->get(StructureController::class); - $controller($request); + return $sqlQuery; } public function executeAddIndexSql(string|DatabaseName $db, string $sql): Message diff --git a/src/Table/Table.php b/src/Table/Table.php index b4acea2b30..b8c0b3b92f 100644 --- a/src/Table/Table.php +++ b/src/Table/Table.php @@ -1878,10 +1878,10 @@ class Table implements Stringable /** * Function to get the sql query for index creation or edit * - * @param Index $index current index - * @param bool $error whether error occurred or not + * @param Index $index current index + * @param Message|false $error whether error occurred or not */ - public function getSqlQueryForIndexCreateOrEdit(Index $index, bool &$error): string + public function getSqlQueryForIndexCreateOrEdit(Index $index, Message|false &$error): string { // $sql_query is the one displayed in the query box $sqlQuery = sprintf( @@ -1891,7 +1891,7 @@ class Table implements Stringable ); // Drops the old index - if (! empty($_POST['old_index'])) { + if (isset($_POST['old_index'])) { $oldIndex = is_array($_POST['old_index']) ? $_POST['old_index']['Key_name'] : $_POST['old_index']; if ($oldIndex === 'PRIMARY') { $sqlQuery .= ' DROP PRIMARY KEY,'; diff --git a/tests/classes/Controllers/Table/IndexRenameControllerTest.php b/tests/classes/Controllers/Table/IndexRenameControllerTest.php index 9248da82d7..bbdb70556b 100644 --- a/tests/classes/Controllers/Table/IndexRenameControllerTest.php +++ b/tests/classes/Controllers/Table/IndexRenameControllerTest.php @@ -48,7 +48,7 @@ class IndexRenameControllerTest extends AbstractTestCase $response, $template, $dbi, - new Indexes($response, $template, $dbi), + new Indexes($dbi), new DbTableExists($dbi), ))($request); $this->assertSame($expected, $response->getHTMLResult()); diff --git a/tests/classes/Controllers/Table/IndexesControllerTest.php b/tests/classes/Controllers/Table/IndexesControllerTest.php index bcb3f4a02c..4c946d904f 100644 --- a/tests/classes/Controllers/Table/IndexesControllerTest.php +++ b/tests/classes/Controllers/Table/IndexesControllerTest.php @@ -96,7 +96,7 @@ class IndexesControllerTest extends AbstractTestCase $response, $template, $dbi, - new Indexes($response, $template, $dbi), + new Indexes($dbi), new DbTableExists($dbi), ); diff --git a/tests/classes/Controllers/Table/Structure/SpatialControllerTest.php b/tests/classes/Controllers/Table/Structure/SpatialControllerTest.php index d382166f2b..224ad8ac4f 100644 --- a/tests/classes/Controllers/Table/Structure/SpatialControllerTest.php +++ b/tests/classes/Controllers/Table/Structure/SpatialControllerTest.php @@ -36,7 +36,7 @@ class SpatialControllerTest extends AbstractTestCase $controllerStub = $this->createMock(StructureController::class); $controllerStub->expects($this->once())->method('__invoke')->with($request); - $indexes = new Indexes(new ResponseRenderer(), new Template(), DatabaseInterface::getInstance()); + $indexes = new Indexes(DatabaseInterface::getInstance()); $controller = new SpatialController(new ResponseRenderer(), new Template(), $controllerStub, $indexes); $controller($request); @@ -64,7 +64,7 @@ class SpatialControllerTest extends AbstractTestCase $controllerStub = $this->createMock(StructureController::class); $controllerStub->expects($this->once())->method('__invoke')->with($request); - $indexes = new Indexes(new ResponseRenderer(), new Template(), DatabaseInterface::getInstance()); + $indexes = new Indexes(DatabaseInterface::getInstance()); $controller = new SpatialController(new ResponseRenderer(), new Template(), $controllerStub, $indexes); $controller($request); @@ -90,7 +90,7 @@ class SpatialControllerTest extends AbstractTestCase $controllerStub->expects($this->never())->method('__invoke'); $response = new ResponseRenderer(); - $indexes = new Indexes(new ResponseRenderer(), new Template(), DatabaseInterface::getInstance()); + $indexes = new Indexes(DatabaseInterface::getInstance()); $controller = new SpatialController($response, new Template(), $controllerStub, $indexes); $controller($request); @@ -120,7 +120,7 @@ class SpatialControllerTest extends AbstractTestCase $controllerStub = $this->createMock(StructureController::class); $controllerStub->expects($this->once())->method('__invoke')->with($request); - $indexes = new Indexes(new ResponseRenderer(), new Template(), DatabaseInterface::getInstance()); + $indexes = new Indexes(DatabaseInterface::getInstance()); $controller = new SpatialController(new ResponseRenderer(), new Template(), $controllerStub, $indexes); $controller($request); diff --git a/tests/classes/Controllers/Table/Structure/UniqueControllerTest.php b/tests/classes/Controllers/Table/Structure/UniqueControllerTest.php index 7d8dc3b923..5ec1e554ec 100644 --- a/tests/classes/Controllers/Table/Structure/UniqueControllerTest.php +++ b/tests/classes/Controllers/Table/Structure/UniqueControllerTest.php @@ -36,7 +36,7 @@ class UniqueControllerTest extends AbstractTestCase $controllerStub = $this->createMock(StructureController::class); $controllerStub->expects($this->once())->method('__invoke')->with($request); - $indexes = new Indexes(new ResponseRenderer(), new Template(), DatabaseInterface::getInstance()); + $indexes = new Indexes(DatabaseInterface::getInstance()); $controller = new UniqueController(new ResponseRenderer(), new Template(), $controllerStub, $indexes); $controller($request); @@ -64,7 +64,7 @@ class UniqueControllerTest extends AbstractTestCase $controllerStub = $this->createMock(StructureController::class); $controllerStub->expects($this->once())->method('__invoke')->with($request); - $indexes = new Indexes(new ResponseRenderer(), new Template(), DatabaseInterface::getInstance()); + $indexes = new Indexes(DatabaseInterface::getInstance()); $controller = new UniqueController(new ResponseRenderer(), new Template(), $controllerStub, $indexes); $controller($request); @@ -90,7 +90,7 @@ class UniqueControllerTest extends AbstractTestCase $controllerStub->expects($this->never())->method('__invoke'); $response = new ResponseRenderer(); - $indexes = new Indexes(new ResponseRenderer(), new Template(), DatabaseInterface::getInstance()); + $indexes = new Indexes(DatabaseInterface::getInstance()); $controller = new UniqueController($response, new Template(), $controllerStub, $indexes); $controller($request); @@ -120,7 +120,7 @@ class UniqueControllerTest extends AbstractTestCase $controllerStub = $this->createMock(StructureController::class); $controllerStub->expects($this->once())->method('__invoke')->with($request); - $indexes = new Indexes(new ResponseRenderer(), new Template(), DatabaseInterface::getInstance()); + $indexes = new Indexes(DatabaseInterface::getInstance()); $controller = new UniqueController(new ResponseRenderer(), new Template(), $controllerStub, $indexes); $controller($request); diff --git a/tests/classes/Table/IndexesTest.php b/tests/classes/Table/IndexesTest.php index 5f16a220af..cde9d42d01 100644 --- a/tests/classes/Table/IndexesTest.php +++ b/tests/classes/Table/IndexesTest.php @@ -7,13 +7,10 @@ namespace PhpMyAdmin\Tests\Table; use PhpMyAdmin\Config; use PhpMyAdmin\Current; use PhpMyAdmin\DatabaseInterface; -use PhpMyAdmin\Http\Factory\ServerRequestFactory; use PhpMyAdmin\Index; use PhpMyAdmin\Table\Indexes; use PhpMyAdmin\Table\Table; -use PhpMyAdmin\Template; use PhpMyAdmin\Tests\AbstractTestCase; -use PhpMyAdmin\Tests\Stubs\ResponseRenderer as ResponseStub; use PHPUnit\Framework\Attributes\CoversClass; #[CoversClass(Indexes::class)] @@ -65,25 +62,24 @@ class IndexesTest extends AbstractTestCase $dbi->expects($this->any())->method('getTable') ->willReturn($table); - $response = new ResponseStub(); $index = new Index(); - $indexes = new Indexes($response, new Template(), $dbi); - - $request = ServerRequestFactory::create()->createServerRequest('GET', 'http://example.com/') - ->withQueryParams(['ajax_request' => '1']); + $indexes = new Indexes($dbi); // Preview SQL - $indexes->doSaveData($request, $index, false, Current::$database, Current::$table, true); - $jsonArray = $response->getJSONResult(); - $this->assertArrayHasKey('sql_data', $jsonArray); - $this->assertStringContainsString($sqlQuery, $jsonArray['sql_data']); + $sqlResult = $indexes->doSaveData($index, false, Current::$database, Current::$table, true); + $this->assertIsString($sqlResult); + $this->assertStringContainsString($sqlQuery, $sqlResult); // Alter success - $response->clear(); - $indexes->doSaveData($request, $index, false, Current::$database, Current::$table, false); - $jsonArray = $response->getJSONResult(); - $this->assertArrayHasKey('index_table', $jsonArray); - $this->assertArrayHasKey('message', $jsonArray); + $sqlResult = $indexes->doSaveData($index, false, Current::$database, Current::$table, false); + $this->assertIsString($sqlResult); + $this->assertStringContainsString($sqlQuery, $sqlResult); + + // Error message + // Cannot be tested at the moment. + // $index->setName('PRIMARY'); // Cannot rename any index to primary so the operation should fail + // $indexes->doSaveData($index, false, Current::$database, Current::$table, false); + // $this->assertInstanceOf(Message::class, $sqlResult); } }