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

Deprecate AbstractPlatform::getName() #4755

Merged
merged 3 commits into from
Aug 26, 2021

Conversation

morozov
Copy link
Member

@morozov morozov commented Aug 24, 2021

Q A
Type deprecation
BC Break no

Closes #4749.

@morozov morozov added this to the 3.2.0 milestone Aug 24, 2021
@morozov morozov linked an issue Aug 24, 2021 that may be closed by this pull request
@morozov morozov requested a review from greg0ire August 24, 2021 22:13
@morozov morozov force-pushed the deprecate-platform-get-name branch 2 times, most recently from e9bd8b3 to cd00ac3 Compare August 25, 2021 21:16
$this->connection->getDatabasePlatform()->getName() !== 'mssql'
! $platform->supportsInlineColumnComments() &&
! $platform->supportsCommentOnStatement() &&
! $platform instanceof SQLServer2012Platform
Copy link
Member

Choose a reason for hiding this comment

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

Should we (re)-introduce SQLServerPlatform, PostgreSQLPlatform before advising people to write instanceof calls? It would be great to have names stable across major DBAL versions, otherwise this is going to be a pain point I think.

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's do.

Copy link

@emodric emodric Nov 30, 2021

Choose a reason for hiding this comment

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

Hi,

I believe there's an issue with PostgreSQLPlatform class here. In 2.13, the name of the class is lowercase, PostgreSqlPlatform, so BC is not actually maintained between 2.13.x and 3.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.

Class names are case-insensitive in PHP. If this difference in casing creates an actual issue for you, please file a bug report.

Copy link

Choose a reason for hiding this comment

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

I need to test a bit. PHP did complain about class not existing for me.

Copy link
Member

Choose a reason for hiding this comment

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

Well, if you maintain a codebase that should be compatible with DBAL 2 and 3 and you attempt to create instances of that class directly and you operate on a case-sensitive file system, you might need to trigger the autoloader on the old casing to make it work, e.g.:

https://github.com/doctrine/orm/blob/6857a2e8d4eba83444b679231260d6e87fcddfa0/tests/Doctrine/Tests/ORM/Query/SelectSqlGenerationTest.php#L37-L39

Copy link

Choose a reason for hiding this comment

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

Alright, that helped, thanks!

@morozov morozov force-pushed the deprecate-platform-get-name branch 3 times, most recently from a08eb66 to fe88c7e Compare August 26, 2021 03:27
greg0ire
greg0ire previously approved these changes Aug 26, 2021
@greg0ire greg0ire requested a review from derrabus August 26, 2021 06:13
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.

RFC: deprecate AbstractPlatform::getName()
4 participants