-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Improving formatting of Identifier Generation Strategies #10574
Conversation
ThomasLandauer
commented
Mar 12, 2023
•
edited
Loading
edited
- Removing duplications
- Improving scanability (which database uses which field type)
Thank you for your changes @ThomasLandauer. Can you please add a description what exactly you accomplish with your changes? |
Sure, done :-) |
preferred by the used database platform. The preferred strategies | ||
are IDENTITY for MySQL, SQLite, MsSQL and SQL Anywhere and SEQUENCE | ||
for Oracle and PostgreSQL. This strategy provides full portability. | ||
preferred by the used database platform: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this is no longer true since #8870 : AbstractPlatform::prefers*
methods have been removed. It's now more based on what the platforms supports:
orm/lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php
Lines 727 to 745 in da9b9de
private function determineIdGeneratorStrategy(AbstractPlatform $platform): int | |
{ | |
if ( | |
$platform instanceof Platforms\OraclePlatform | |
|| $platform instanceof Platforms\PostgreSQLPlatform | |
) { | |
return ClassMetadata::GENERATOR_TYPE_SEQUENCE; | |
} | |
if ($platform->supportsIdentityColumns()) { | |
return ClassMetadata::GENERATOR_TYPE_IDENTITY; | |
} | |
if ($platform->supportsSequences()) { | |
return ClassMetadata::GENERATOR_TYPE_SEQUENCE; | |
} | |
throw CannotGenerateIds::withPlatform($platform); | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Related: #8931
OK, then I'm closing this. Just wanted to do some minor formatting improvements - your PR is more important! |
Well it's been opened for 2 years, and I'm not sure when I'm going to look into it again. I wasn't suggesting you close this, just that you reword slightly this to avoid saying the platform prefers anything. |
I know. But if there's some change ahead, I'd rather wait for the outcome. The current text is not that bad that it's urgent in any way... |