Skip to content

Commit 7459f08

Browse files
MauricioFauthMauricioFauth
authored
Merge pull request #19624 from kamil-tekiela/Fix-column-move
Fix column move
2 parents eb25b88 + e9ab86b commit 7459f08

5 files changed

Lines changed: 91 additions & 25 deletions

File tree

‎phpstan-baseline.neon‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4225,14 +4225,14 @@ parameters:
42254225
path: src/Controllers/Table/Structure/ChangeController.php
42264226

42274227
-
4228-
message: '#^Loose comparison via "\=\=" is not allowed\.$#'
4229-
identifier: equal.notAllowed
4228+
message: '#^PHPDoc tag @var with type int is not subtype of type int\<0, max\>\|false\.$#'
4229+
identifier: varTag.type
42304230
count: 1
42314231
path: src/Controllers/Table/Structure/MoveColumnsController.php
42324232

42334233
-
4234-
message: '#^PHPDoc tag @var with type int is not subtype of type int\<0, max\>\|false\.$#'
4235-
identifier: varTag.type
4234+
message: '#^Parameter \#2 \$moveColumns of method PhpMyAdmin\\Controllers\\Table\\Structure\\MoveColumnsController\:\:generateAlterTableSql\(\) expects list\<string\>, list given\.$#'
4235+
identifier: argument.type
42364236
count: 1
42374237
path: src/Controllers/Table/Structure/MoveColumnsController.php
42384238

‎psalm-baseline.xml‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2808,6 +2808,11 @@
28082808
<code><![CDATA[[$request->getParam('field')]]]></code>
28092809
</MixedArgumentTypeCoercion>
28102810
</file>
2811+
<file src="src/Controllers/Table/Structure/MoveColumnsController.php">
2812+
<MixedArgumentTypeCoercion>
2813+
<code><![CDATA[$moveColumns]]></code>
2814+
</MixedArgumentTypeCoercion>
2815+
</file>
28112816
<file src="src/Controllers/Table/Structure/PartitioningController.php">
28122817
<PossiblyNullArgument>
28132818
<code><![CDATA[$stmt->partitions]]></code>

‎resources/js/src/table/structure.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -405,7 +405,7 @@ AJAX.registerOnload('table/structure.js', function () {
405405
for (var i in data.columns) {
406406
var theColumn = data.columns[i];
407407
var $theRow = $rows
408-
.find('input:checkbox[value=\'' + theColumn + '\']')
408+
.find('input:checkbox[value="' + escapeJsString(theColumn) + '"]')
409409
.closest('tr');
410410
// append the row for this column to the table
411411
$fieldsTable.append($theRow);

‎src/Controllers/Table/Structure/MoveColumnsController.php‎

Lines changed: 13 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,6 @@
2020
use function __;
2121
use function array_diff;
2222
use function array_is_list;
23-
use function array_keys;
2423
use function array_search;
2524
use function array_splice;
2625
use function assert;
@@ -52,8 +51,9 @@ public function __invoke(ServerRequest $request): Response
5251
$createTableSql = $this->dbi->getTable(Current::$database, Current::$table)->showCreate();
5352
$sqlQuery = $this->generateAlterTableSql($createTableSql, $moveColumns);
5453

55-
if ($sqlQuery === null) {
54+
if ($sqlQuery instanceof Message) {
5655
$this->response->setRequestStatus(false);
56+
$this->response->addJSON('message', $sqlQuery);
5757

5858
return $this->response->response();
5959
}
@@ -85,42 +85,39 @@ public function __invoke(ServerRequest $request): Response
8585
return $this->response->response();
8686
}
8787

88-
/**
89-
* @param array<int,mixed> $moveColumns
90-
* @psalm-param list<mixed> $moveColumns
91-
*/
92-
private function generateAlterTableSql(string $createTableSql, array $moveColumns): string|null
88+
/** @param list<string> $moveColumns */
89+
private function generateAlterTableSql(string $createTableSql, array $moveColumns): string|Message
9390
{
9491
$parser = new Parser($createTableSql);
9592
/** @var CreateStatement $statement */
9693
$statement = $parser->statements[0];
9794
/** @var CreateDefinition[] $fields */
9895
$fields = $statement->fields;
9996
$columns = [];
97+
$columnNames = [];
10098
foreach ($fields as $field) {
10199
if ($field->name === null) {
102100
continue;
103101
}
104102

105103
$columns[$field->name] = $field;
104+
$columnNames[] = $field->name;
106105
}
107106

108-
$columnNames = array_keys($columns);
109107
// Ensure the columns from client match the columns from the table
110108
if (
111109
count($columnNames) !== count($moveColumns) ||
112110
array_diff($columnNames, $moveColumns) !== []
113111
) {
114-
return null;
112+
return Message::error(__('The selected columns do not match the columns in the table.'));
115113
}
116114

117115
$changes = [];
118116

119117
// move columns from first to last
120-
/** @psalm-var list<string> $moveColumns */
121118
foreach ($moveColumns as $i => $columnName) {
122119
// is this column already correctly placed?
123-
if ($columnNames[$i] == $columnName) {
120+
if ($columnNames[$i] === $columnName) {
124121
continue;
125122
}
126123

@@ -129,14 +126,17 @@ private function generateAlterTableSql(string $createTableSql, array $moveColumn
129126
($i === 0 ? ' FIRST' : ' AFTER ' . Util::backquote($columnNames[$i - 1]));
130127

131128
// Move column to its new position
132-
/** @var int $j */
129+
/**
130+
* @var int $j
131+
* We are sure that the value exists because we checked it with array_diff and the type of both is string
132+
*/
133133
$j = array_search($columnName, $columnNames, true);
134134
array_splice($columnNames, $j, 1);
135135
array_splice($columnNames, $i, 0, $columnName);
136136
}
137137

138138
if ($changes === []) {
139-
return null;
139+
return Message::error(__('The selected columns are already in the correct order.'));
140140
}
141141

142142
assert($statement->name !== null, 'Alter table statement has no name');

‎tests/unit/Controllers/Table/Structure/MoveColumnsControllerTest.php‎

Lines changed: 68 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66

77
use PhpMyAdmin\Controllers\Table\Structure\MoveColumnsController;
88
use PhpMyAdmin\Current;
9+
use PhpMyAdmin\Message;
910
use PhpMyAdmin\Template;
1011
use PhpMyAdmin\Tests\AbstractTestCase;
1112
use PhpMyAdmin\Tests\Stubs\ResponseRenderer as ResponseStub;
@@ -23,8 +24,11 @@ class MoveColumnsControllerTest extends AbstractTestCase
2324
* @psalm-param list<string> $columnNames
2425
*/
2526
#[DataProvider('providerForTestGenerateAlterTableSql')]
26-
public function testGenerateAlterTableSql(string $createStatement, array $columnNames, string|null $expected): void
27-
{
27+
public function testGenerateAlterTableSql(
28+
string $createStatement,
29+
array $columnNames,
30+
string|Message $expected,
31+
): void {
2832
$class = new ReflectionClass(MoveColumnsController::class);
2933
$method = $class->getMethod('generateAlterTableSql');
3034

@@ -38,19 +42,26 @@ public function testGenerateAlterTableSql(string $createStatement, array $column
3842
new Template(),
3943
$dbi,
4044
);
41-
/** @var string|null $alterStatement */
45+
/** @var string|Message $alterStatement */
4246
$alterStatement = $method->invoke($controller, $createStatement, $columnNames);
4347

44-
$expected = $expected === null ? null : preg_replace('/\r?\n/', "\n", $expected);
45-
$alterStatement = $alterStatement === null ? null : preg_replace('/\r?\n/', "\n", $alterStatement);
48+
if ($expected instanceof Message) {
49+
self::assertInstanceOf(Message::class, $alterStatement);
50+
self::assertSame($expected->getMessage(), $alterStatement->getMessage());
51+
52+
return;
53+
}
54+
55+
self::assertIsString($alterStatement);
56+
$expected = preg_replace('/\r?\n/', "\n", $expected);
57+
$alterStatement = preg_replace('/\r?\n/', "\n", $alterStatement);
4658
self::assertSame($expected, $alterStatement);
4759
}
4860

4961
/**
5062
* Data provider for testGenerateAlterTableSql
5163
*
52-
* @return array<array<string[]|string|null>>
53-
* @psalm-return list<array{string,list<string>,string}>
64+
* @return list<array{string,list<string>,string|Message}>
5465
*/
5566
public static function providerForTestGenerateAlterTableSql(): array
5667
{
@@ -117,6 +128,56 @@ public static function providerForTestGenerateAlterTableSql(): array
117128
CHANGE `enum` `enum` enum('yes','no') CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci NOT NULL DEFAULT 'no' FIRST
118129
SQL,
119130
],
131+
[
132+
<<<'SQL'
133+
CREATE TABLE `test` (
134+
`id` int(11) NOT NULL,
135+
`enum` enum('yes','no') CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci NOT NULL DEFAULT 'no',
136+
PRIMARY KEY (`id`)
137+
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb3 COLLATE=utf8mb3_unicode_ci
138+
SQL,
139+
['foo', 'id'],
140+
Message::error('The selected columns do not match the columns in the table.'),
141+
],
142+
[
143+
<<<'SQL'
144+
CREATE TABLE `test` (
145+
`id` int(11) NOT NULL,
146+
`enum` enum('yes','no') CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci NOT NULL DEFAULT 'no',
147+
PRIMARY KEY (`id`)
148+
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb3 COLLATE=utf8mb3_unicode_ci
149+
SQL,
150+
['id', 'enum'],
151+
Message::error('The selected columns are already in the correct order.'),
152+
],
153+
[
154+
<<<'SQL'
155+
CREATE TABLE `test` (
156+
`"` int(11) NOT NULL,
157+
`'` enum('yes','no') CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci NOT NULL DEFAULT 'no',
158+
PRIMARY KEY (`id`)
159+
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb3 COLLATE=utf8mb3_unicode_ci
160+
SQL,
161+
['\'', '"'],
162+
<<<'SQL'
163+
ALTER TABLE `test`
164+
CHANGE `'` `'` enum('yes','no') CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci NOT NULL DEFAULT 'no' FIRST
165+
SQL,
166+
],
167+
[
168+
<<<'SQL'
169+
CREATE TABLE `test` (
170+
`1` int(11) NOT NULL,
171+
`0` enum('yes','no') CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci NOT NULL DEFAULT 'no',
172+
PRIMARY KEY (`id`)
173+
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb3 COLLATE=utf8mb3_unicode_ci
174+
SQL,
175+
['0', '1'],
176+
<<<'SQL'
177+
ALTER TABLE `test`
178+
CHANGE `0` `0` enum('yes','no') CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci NOT NULL DEFAULT 'no' FIRST
179+
SQL,
180+
],
120181
];
121182
// phpcs:enable
122183
}

0 commit comments

Comments
 (0)