-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
DBAL-2925: Added functional test checking ability to change PK for po… #2929
Changes from 7 commits
de8a64f
881188a
f976fb3
ba9a0c1
c7a088b
2e29acd
bb17141
49992d5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2078,6 +2078,12 @@ protected function getPreAlterTableIndexForeignKeySQL(TableDiff $diff) | |
$sql[] = $this->getDropIndexSQL($index, $tableName); | ||
} | ||
foreach ($diff->changedIndexes as $index) { | ||
if ($index->isPrimary()) { | ||
$sql[] = $this->getDropPrimaryKeySQL($tableName); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Majkl578 is there a way to enforce a new line before flow control operators like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No sniff I'm aware of, but shouldn't be hard to implement. Just needs to be compatible i.e. with single statements: if (...) {
continue;
} or comments: foo();
// comment
return 123; There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah. More generically, the first case is if the previous line has flow control statement which is a block opener, there shouldn't be an empty line. Like: function x()
{
foreach (...) {
foreach (...) {
}
}
foreach (...) {
}
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've added new line before There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @carusogabriel do you want to file this as a requirement for coding standards (I see we have disabled issues for the repository). It'd be nice to have a reference point somewhere. Otherwise, I constantly forget where the discussion started. /cc @deeky666 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @morozov Require new lines before control structions you mean? :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. Before and after. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @morozov , Totally agree with this proposal, but code marked as still required your review :) Mark it properly please. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @SCIF it's not because of the formatting issue. I want to see if the behavior of dropping a non-existing PK on SQL Server is consistent with other platforms. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This thread is regarding cs only, the PK ones here |
||
|
||
continue; | ||
} | ||
|
||
$sql[] = $this->getDropIndexSQL($index, $tableName); | ||
} | ||
|
||
|
@@ -3605,4 +3611,9 @@ protected function getLikeWildcardCharacters() : string | |
{ | ||
return '%_'; | ||
} | ||
|
||
protected function getDropPrimaryKeySQL(string $table) : string | ||
{ | ||
return 'ALTER TABLE ' . $table . ' DROP PRIMARY KEY'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see, I believe, the original issue (#2925) is filed for MySQL where There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The issue originally discovered during work with Postgres. Initially I tried to modify There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. PK is not just an index, but it's a constraint in some RDBMS. Handling could be done by changing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
It's fine to have it for Postgres and MySQL which seem unable to drop the PK by name but it shouldn't be part of the abstract platform API. Those platforms which can drop PK by its name shouldn't have any changes. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I really don't understand what you propose. Do you mean define protected function getDropPrimaryKeySQL(string $table) : string
{
$this->getDropIndexSQL('primary', $table);
} ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Then, in the case when There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You also can fix it only for your use case when the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
How I can do that? I don't know a way to do it. If you mean to find a workaround for particular There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Neither do I. That's why I'm saying it's fine if you solve only the case where the passed index is an object.
The root can could be only solved by reworking the entire index-related API (I recall there was a discussion or at least mention of that /cc @deeky666). This API would account for all differences between indices, constraints, primary keys, foreign keys, etc. in different platforms and would provide some unified higher level API. It's impossible to solve it correctly in terms of the current API without introducing more controversial methods (I'm talking about There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right now, |
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1619,4 +1619,18 @@ private function generateIdentifierName($identifier) | |
|
||
return strtoupper(dechex(crc32($identifier->getName()))); | ||
} | ||
|
||
protected function getDropPrimaryKeySQL(string $table) : string | ||
{ | ||
return " | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you use heredoc syntax instead? |
||
DECLARE @table NVARCHAR(512), @sql NVARCHAR(MAX); | ||
SELECT @table = N'{$table}'; | ||
SELECT @sql = 'ALTER TABLE ' + @table + ' DROP CONSTRAINT ' + name + ';' | ||
FROM sys.key_constraints | ||
WHERE [type] = 'PK' | ||
AND [parent_object_id] = OBJECT_ID(@table); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would it make sense to move the object ID outside of the How will this query behave if the table doesn't have a PK? I'd expect There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Emm. If you will try to drop PK for any other platforms on table not containing it, exception will be thrown as well, so I don't see any inconsistency here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it the same type of exception? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, different one, you are right. Just There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You don't need a Windows PC. The SQL Server extension can be compiled for PHP on Linux, and the server itself can be running in a Docker container (microsoft/mssql-server-linux). I don't have a proposal currently. My only concern is this code may execute knowingly syntactically invalid SQL. Instead of doing the in the dynamic SQL way, can we just rely on some conventional PK name in SQL Server (like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hold on. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can't rely on any name conventions/best practices. As far as I investigated, the name builds according to schema |
||
|
||
EXEC sp_executeSQL @sql;"; | ||
} | ||
|
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Scrutinizer failure reveals this as a decent candidate for refactoring in the future. Instead of the platform knowing how to drop a given index based on its properties (primary, non-primary and so on), the index should tell the platform how to drop itself (sort of a Visitor pattern):
This is outside of scope, just noting down.
/cc @Ocramius @deeky666
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking about it but that will lead to creation of platform specific indices. Quite heavy refactoring will be required, I worry. I'm afraid I'm not able to do it in reasonable time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really fan of the suggested visitor approach. Index would then need to know how to represent itself on all existing platforms and even those not provided by DBAL out-of-box.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SCIF no need to worry about it right now. It's just for the record.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not exactly. They will need to be able to identify themselves as a type while the platform will know how to handle certain types. Same as we do with column types.