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

Rework AbstractPlatform code for table and column creation #6808

Open
wants to merge 6 commits into
base: 5.0.x
Choose a base branch
from

Conversation

morozov
Copy link
Member

@morozov morozov commented Feb 24, 2025

  1. Making the $options parameter of the AbstractPlatform#_getCreateTableSQL() non-optional and marking certain keys of the CreateTableParameters array as non-optional allows to remove some if (!empty($options[...])) checks. They are always passed.
  2. The $name parameter of the AbstractPlatform#getColumnDeclarationSQL() was redundant as the $column parameter always contains the column name.
  3. The primary key of the CreateTableParameters array was redundant as the primary_index key already contains the whole Index definition.
  4. In the AbstractPlatform#_getCreateTableSQL() method implementations of AbstractPlatform and subclasses, calls to AbstractPlatform#getColumnDeclarationListSQL() have been replaced with a foreach loop and calls to AbstractPlatform#getColumnDeclarationSQL(). The "list" method has the same implementation for all platforms and doesn't do anything than iteration. It produces a comma separated list of column declarations, which is later augmented with constraint declarations, etc. From the code organization standpoint, it's easier to operate on an array and join it once.

TODO in 4.3.x:

  1. Deprecate defining primary key indexes with duplicate columns. There's no point in having the DBAL responsible for that.
  2. Mark AbstractPlatform#getColumnDeclarationListSQL() as internal or deprecate (it is only used in a test that creates a temporary table in a way that it's not supposed to).
  3. Backport changes from AbstractMySQLPlatform where a call to Index#getColumns() is replaced with Index#getQuotedColumns(). Not quoting column names in SQL is a bug. There's another bug that makes adding a new PK with quoted columns on MySQL not work, so we cannot test this yet.

Comment on lines -615 to +610
$modifyColumnSQL[] = $newColumn->getObjectName()->toSQL($this) . $newSQL;
$modifyColumnSQL[] = $newSQL;
Copy link
Member Author

@morozov morozov Feb 24, 2025

Choose a reason for hiding this comment

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

This was a hack. From the SQL syntax perspective, a column name followed by the column definition (which also starts with the column name) doesn't make sense. This code worked only because the column definition was generated for a column with empty name (which is also invalid).

@morozov morozov force-pushed the rework-column-creation branch from 22923e2 to f146c57 Compare February 24, 2025 02:01
@morozov morozov force-pushed the rework-column-creation branch from f146c57 to 8610346 Compare February 24, 2025 02:30
@morozov morozov marked this pull request as ready for review February 24, 2025 02:40
@morozov morozov requested a review from greg0ire February 24, 2025 02:40
@morozov morozov changed the title Rework AbstractPlatform code fore table and column creation Rework AbstractPlatform code for table and column creation Feb 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant