Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Preserve check constraint during column move #19105

Merged

Conversation

MoonE
Copy link
Contributor

@MoonE MoonE commented Apr 8, 2024

Description

Fixes #17920, fixes #18006, fixes #18962
Closes #19009

See #13592, #14371, #16224

  • Any new functionality is covered by tests.

@MoonE MoonE force-pushed the move-column-with-check-constraint branch 8 times, most recently from 85ef802 to db4f2d8 Compare April 10, 2024 23:19
Copy link
Member

@williamdes williamdes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks promissing

@MoonE MoonE force-pushed the move-column-with-check-constraint branch from db4f2d8 to d3453ed Compare April 18, 2024 23:38
@MoonE
Copy link
Contributor Author

MoonE commented Apr 18, 2024

Here is the version when merging QA_5_2 -> master

@MoonE MoonE marked this pull request as ready for review April 18, 2024 23:40
@MoonE MoonE marked this pull request as draft April 18, 2024 23:49
@MoonE MoonE force-pushed the move-column-with-check-constraint branch from d3453ed to 52a6142 Compare April 19, 2024 00:01
if (! is_array($moveColumns) || ! array_is_list($moveColumns) || ! $this->response->isAjax()) {
$this->response->setRequestStatus(false);
Copy link
Contributor Author

@MoonE MoonE Apr 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added the check for array_is_list here and also added the missing setRequestStatus.
Remove array_is_list here again as it is php >= 8.1.

Comment on lines +110 to +115
// Ensure the columns from client match the columns from the table
if (
count($columnNames) !== count($moveColumns) ||
array_diff($columnNames, $moveColumns) !== []
) {
return null;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an added sanity check that the columns of the request match the columns from the CREATE TABLE statement.

Comment on lines +139 to +140
if ($changes === []) {
return null;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If no changes detected now doesn't return incomplete statement without any column changes.

Calls to this controller were already prevented when pressing ok, but not on preview.

Comment on lines +325 to +329
const serialized = $form.serialize();
if (serialized === $form.data('serialized-unmoved')) {
modalBody.innerHTML = '';
return;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't make a request when nothing was changed. The controller now would return an error in this case. Before, an invalid alter table statement was returned without any column changes.

@MoonE MoonE marked this pull request as ready for review April 19, 2024 00:13
@MoonE MoonE force-pushed the move-column-with-check-constraint branch 2 times, most recently from 4308471 to 76ccc3b Compare April 19, 2024 19:49
Signed-off-by: Maximilian Krög <maxi_kroeg@web.de>
@MoonE MoonE force-pushed the move-column-with-check-constraint branch from 76ccc3b to 3b992e6 Compare April 19, 2024 20:01
Copy link
Member

@williamdes williamdes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you !

@williamdes williamdes added this to the 5.2.2 milestone Apr 28, 2024
@williamdes williamdes self-assigned this Apr 28, 2024
@williamdes williamdes merged commit 0bcc3e4 into phpmyadmin:QA_5_2 Apr 28, 2024
26 of 30 checks passed
@MoonE MoonE deleted the move-column-with-check-constraint branch April 28, 2024 10:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants