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

respect to platform restrictions when creating autoincrement identifier name #4106

Merged
merged 1 commit into from
Jun 29, 2020
Merged

respect to platform restrictions when creating autoincrement identifier name #4106

merged 1 commit into from
Jun 29, 2020

Conversation

karakayasemi
Copy link
Contributor

Q A
Type bug
BC Break no
Fixed issues

Summary

OraclePlatform does not respect to 30 character max element name restriction in getAutoincrementIdentifierName method. This situation leads to ORA-00972: identifier is too long error in Oracle databases like here owncloud/twofactor_totp#140 (comment). This PR aims to fix this bug.

@morozov
Copy link
Member

morozov commented Jun 26, 2020

Thanks for the contribution, @karakayasemi. Please add a functional test or tweak an existing one that will cover your case on all supported platforms. Looks like it's not tested anywhere.

@karakayasemi karakayasemi changed the title respect to platform restrictions when creating autoincrement identfier name respect to platform restrictions when creating autoincrement identifier name Jun 28, 2020
@karakayasemi
Copy link
Contributor Author

Since generated names include table name, shortening generated name by cutting its exceeding part from the at the end of the string can result in the same name with the table name. So, I kept the suffix and cut the base string as needed to adjust the length.

As you suggest, a functional test that covers the max identifier length case has been added. @morozov please review once more. Thank you.

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.

Looks good for the most part, just needs some cleanup 👍

@karakayasemi
Copy link
Contributor Author

@morozov it is ready for review again. Thank you for your help.

@morozov
Copy link
Member

morozov commented Jun 29, 2020

Looks good. Please revert the irrelevant changes and squash.

@karakayasemi
Copy link
Contributor Author

@morozov the irrelevant change has been reverted and commits have been squashed.

@morozov morozov merged commit 13e73f1 into doctrine:2.10.x Jun 29, 2020
@morozov
Copy link
Member

morozov commented Jun 29, 2020

Thanks, @karakayasemi!

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.

2 participants