Navigation Menu

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

Use correct column order for composite foreign keys #4978

Merged
merged 1 commit into from Nov 11, 2021
Merged

Use correct column order for composite foreign keys #4978

merged 1 commit into from Nov 11, 2021

Conversation

AndreasA
Copy link

@AndreasA AndreasA commented Nov 10, 2021

Fixes wrong foreign key order. For now just an integration test.

Q A
Type bug
BC Break no
Fixed issues #4971

@AndreasA AndreasA marked this pull request as ready for review November 10, 2021 08:37
@AndreasA AndreasA changed the title Added integration test for foreign key order MySQL Platform: Use correct column order for composite foreign keys. Nov 10, 2021
@AndreasA AndreasA changed the title MySQL Platform: Use correct column order for composite foreign keys. Bugfix #4971: MySQL Platform: Use correct column order for composite foreign keys. Nov 10, 2021
Copy link
Member

@morozov morozov left a comment

Choose a reason for hiding this comment

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

Thanks for the patch, @AndreasA. Please see a few comments inline.

src/Platforms/MySQLPlatform.php Show resolved Hide resolved
src/Platforms/MySQLPlatform.php Outdated Show resolved Hide resolved
tests/Functional/Schema/MySQLSchemaManagerTest.php Outdated Show resolved Hide resolved
tests/Functional/Schema/MySQLSchemaManagerTest.php Outdated Show resolved Hide resolved
@AndreasA
Copy link
Author

AndreasA commented Nov 11, 2021

@morozov The threads should now be fixed or if not I added a comment why not.

I have now also manually executed the two create table statements on another machine with MySQL 8.0 and the retrieve foreign key SQL for it and the result was the same regarding the foreign key columns order. without ORDINAL_POSITION it was different there as well, so I think it is not just an issue - due to some configuration - for my MySQL installation. In any case using ORDINAL_POSITION should always work.

I have also checked and ORDINAL_POSITION was in that table since 5.6 - e.g. in case of MariaDB https://dev.mysql.com/doc/refman/5.6/en/information-schema-key-column-usage-table.html

By now moving the test to be used for all platforms, I cannot be sure 100% if the issue is fixed for all of them. I have fixed it for the SQLSrv tests now because that workflow was automatically started.

I just hope that all others are already retrieving the data in the correct order 😄 From a short glance they should do so but the tests should make sure.

One other thing: I have created the PR for 3.x but should I actually create a PR for 2.x which can then be merged into 3.x? though I doubt it is an issue that occurs that often, otherwise I think it would have been addressed earlier.

@AndreasA AndreasA changed the title Bugfix #4971: MySQL Platform: Use correct column order for composite foreign keys. Bugfix #4971: Use correct column order for composite foreign keys. Nov 11, 2021
@morozov
Copy link
Member

morozov commented Nov 11, 2021

I just hope that all others are already retrieving the data in the correct order 😄 From a short glance they should do so but the tests should make sure.

I approved the test run.

One other thing: I have created the PR for 3.x but should I actually create a PR for 2.x which can then be merged into 3.x? though I doubt it is an issue that occurs that often, otherwise I think it would have been addressed earlier.

There's no need to backport the fix to 2.x unless you really need to.

@AndreasA
Copy link
Author

@morozov I have now done the necessary adjustments. Can you re-check? Not sure if the test run is running against the lastest verison.

@morozov
Copy link
Member

morozov commented Nov 11, 2021

You'll need to disable the test on SQLite (see #4978 (comment)). Its failure prevents other jobs from running.

@AndreasA
Copy link
Author

@morozov

You'll need to disable the test on SQLite (see #4978 (comment)). Its failure prevents other jobs from running.

Ah yes. Missed that part and have added it now. Thanks for letting me know.

@derrabus
Copy link
Member

Can you have a look at the test failures on Oracle and DB2?

@AndreasA
Copy link
Author

@derrabus I just did, apparantly the column names are always uppercase there but I specify them originally lowercase. I guess the easies solution would be to create them uppercase for now as I don't really care about the case just the order.

Or I ignore the case during the assertion. Not sure which way to go

@morozov
Copy link
Member

morozov commented Nov 11, 2021

Most tests create the columns in the lower case and use strtolower() on the introspection results before comparison.

@derrabus derrabus changed the title Bugfix #4971: Use correct column order for composite foreign keys. Use correct column order for composite foreign keys Nov 11, 2021
@derrabus derrabus added this to the 3.1.4 milestone Nov 11, 2021
@AndreasA
Copy link
Author

@morozov @derrabus Added a corresponding strtolower conversion for the result comparison now. Hopefully, the tests will work now.

@morozov
Copy link
Member

morozov commented Nov 11, 2021

@AndreasA, the patch looks good to me. Please squash all commits, and we can merge it.

@AndreasA
Copy link
Author

@morozov Done.

@morozov morozov merged commit 10df50f into doctrine:3.1.x Nov 11, 2021
@morozov
Copy link
Member

morozov commented Nov 11, 2021

Thanks, @AndreasA!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 12, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
3 participants