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

Getting rid of the column name index #3583

Merged
merged 1 commit into from Jun 4, 2019

Conversation

morozov
Copy link
Member

@morozov morozov commented Jun 1, 2019

Q A
Type improvement
BC Break yes

Right now, column names are contained both in array keys and the actual definitions which creates an ambiguity which is a potential source of issues. The proposal is to not rely on the keys and always use the name from the column definition as the single source of truth.

if (isset($column['sequence'])) {
$sql[] = $this->getCreateSequenceSQL($column['sequence']);
}

if (! isset($column['autoincrement']) || ! $column['autoincrement'] &&
(! isset($column['autoinc']) || ! $column['autoinc'])) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure if it's worth mentioning the removal of 'autoinc' as a breaking change. I don't see it mentioned in the documentation and this is the only platform which has it. The rest of the usages were removed prior to DBAL 2.0.

Copy link
Member

Choose a reason for hiding this comment

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

I would remove it in another PR and add it to the upgrade doc. That isn't really related to getting rid of the column name index.

@jwage
Copy link
Member

jwage commented Jun 1, 2019

Should this be done in Doctrine\DBAL\Schema\Table too?

@morozov
Copy link
Member Author

morozov commented Jun 3, 2019

This is a very good question. Internally, Table uses string keys in order to enforce column name uniqueness. But at the same time, they don't need to be exposed outside. Let me see if the same is applicable to the rest of the codebase. For now, I'll put this on hold.

@jwage
Copy link
Member

jwage commented Jun 4, 2019

@morozov We can just merge this and create another issue for improving Doctrine\DBAL\Schema\Table. They can be done separate?

@morozov
Copy link
Member Author

morozov commented Jun 4, 2019

Yes, let's get this one merged then.

@jwage jwage self-assigned this Jun 4, 2019
@jwage jwage merged commit 0c4aa7e into doctrine:develop Jun 4, 2019
morozov pushed a commit that referenced this pull request Jun 13, 2019
Getting rid of the column name index
morozov pushed a commit that referenced this pull request Jun 27, 2019
Getting rid of the column name index
morozov pushed a commit that referenced this pull request Jun 27, 2019
Getting rid of the column name index
morozov pushed a commit that referenced this pull request Jun 27, 2019
Getting rid of the column name index
morozov pushed a commit to morozov/dbal that referenced this pull request Aug 26, 2019
morozov pushed a commit to morozov/dbal that referenced this pull request Aug 26, 2019
morozov pushed a commit to morozov/dbal that referenced this pull request Aug 26, 2019
morozov pushed a commit to morozov/dbal that referenced this pull request Aug 27, 2019
morozov pushed a commit that referenced this pull request Nov 2, 2019
Getting rid of the column name index
@morozov morozov added this to the 3.0.0 milestone Nov 2, 2019
@morozov morozov deleted the event-types-cleanup branch December 31, 2019 19:54
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 9, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants