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

Index length can be a `string`: ensure that it is an integer when read by the `MySqlSchemaManager` #3420

Merged
merged 1 commit into from Jan 6, 2019

Conversation

Projects
None yet
4 participants
@leofeyer
Copy link
Contributor

leofeyer commented Jan 2, 2019

Q A
Type bug
BC Break no

Fixes #3415.

Summary

When using MySQL, AbstractSchemaManager::listTableIndexes() will return the index length as string, which makes the strict comparison in Index::hasSameColumnLengths() fail. $this->options['length'] will have the index length as string and $other->options['length'] as integer.

I am not 100% sure if my fix is in the right place, as we could also cast to int in the _getPortableTableIndexesList() method. So feel free to adjust the PR as necessary.

@leofeyer leofeyer force-pushed the leofeyer:hotfix/index-length branch from 05f9903 to bb0a9c8 Jan 3, 2019

@leofeyer leofeyer force-pushed the leofeyer:hotfix/index-length branch from 3d3973e to 79aea88 Jan 3, 2019

@leofeyer

This comment has been minimized.

Copy link
Contributor

leofeyer commented Jan 3, 2019

@morozov Can we be certain that this issue is not limited to MySQL? Otherwise I would prefer to apply the changes to the MySqlSchemaManager class only.

Edit: I guess we should apply the logic to MySQL only, as the changes break the OCI tests.

@leofeyer leofeyer force-pushed the leofeyer:hotfix/index-length branch from 79aea88 to d74eeea Jan 4, 2019

@morozov

This comment has been minimized.

Copy link
Member

morozov commented Jan 4, 2019

@leofeyer if I'm not mistaken, prefix indices are only supported by MySQL, so it should be fine to fix/test it only for MySQL. Once the changes are complete, please squash the commits.

@leofeyer leofeyer force-pushed the leofeyer:hotfix/index-length branch from c4d5c7e to 2ff4c54 Jan 5, 2019

@leofeyer

This comment has been minimized.

Copy link
Contributor

leofeyer commented Jan 5, 2019

Done.

@Ocramius Ocramius changed the title Index length can be a string Index length can be a `string`: ensure that it is an integer when read by the `MySqlSchemaManager` Jan 5, 2019

@Ocramius
Copy link
Member

Ocramius left a comment

👍

@leofeyer leofeyer force-pushed the leofeyer:hotfix/index-length branch from 2ff4c54 to d82b245 Jan 5, 2019

@morozov morozov changed the base branch from 2.9 to master Jan 6, 2019

@morozov

This comment has been minimized.

Copy link
Member

morozov commented Jan 6, 2019

@leofeyer please rebase on master and fix the code style violation.

@leofeyer leofeyer force-pushed the leofeyer:hotfix/index-length branch from d82b245 to 8173462 Jan 6, 2019

@leofeyer

This comment has been minimized.

Copy link
Contributor

leofeyer commented Jan 6, 2019

Done.

@leofeyer leofeyer force-pushed the leofeyer:hotfix/index-length branch from 8173462 to 0608372 Jan 6, 2019

@morozov morozov added this to the 2.9.3 milestone Jan 6, 2019

@morozov

morozov approved these changes Jan 6, 2019

@morozov morozov merged commit 0731608 into doctrine:master Jan 6, 2019

4 checks passed

Scrutinizer 1 updated code elements
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuousphp Build passed successfully on continuousphp.
Details

morozov added a commit that referenced this pull request Jan 6, 2019

@morozov

This comment has been minimized.

Copy link
Member

morozov commented Jan 6, 2019

Thank you @leofeyer 👍

@leofeyer leofeyer deleted the leofeyer:hotfix/index-length branch Jan 11, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment