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

Deprecated platform "commented type" API #5194

Closed
mvorisek opened this issue Jan 21, 2022 · 10 comments
Closed

Deprecated platform "commented type" API #5194

mvorisek opened this issue Jan 21, 2022 · 10 comments
Labels

Comments

@mvorisek
Copy link
Contributor

Q A
Version 3.3.x

https://github.com/doctrine/dbal/blob/3.3.x/UPGRADE.md#deprecated-platform-commented-type-api

image

Support Question

AbstractPlatform::initializeCommentedDoctrineTypes() seems removed

when a platform does not support some storage type natively (like from the coverage report above, we store binary as hex encoded string), we used this method to enforce DBAL to add a comment based on the platform

how this can be achived now?

@greg0ire
Copy link
Member

The solution would be contribute an implementation of requiresSQLCommentTypeHint() in https://github.com/doctrine/dbal/blob/3.3.x/src/Types/BinaryType.php that returns true for the platform in question.

@mvorisek
Copy link
Contributor Author

Any customization of a specific type is not an option, I have given an example, we customize more types based on a platform. The customization makes sense to me, as not every DBAL type has to be natively supported as a Type impl. may expect implicitly.

@greg0ire
Copy link
Member

I read that 5 times and I don't understand… can you maybe rephrase?

@mvorisek
Copy link
Contributor Author

mvorisek commented Jan 21, 2022

yes - previously it was possible from a platform to change the type definition like in:

https://github.com/atk4/data/blob/develop/src/Persistence/Sql/Oracle/PlatformTrait.php#L22

and also the flag if the type is native or not (eg. if comment is added or not)

my question is, as the AbstractPlatform::initializeCommentedDoctrineTypes method override no longer works since 3.3.0, how to adjust the platform code, like the example code above for Oracle platform, to maintain the same behaviour as in DBAL 2.x - 3.2.x

in simple words, how to control if type should be commented from Platform side, not from the Type side

@greg0ire
Copy link
Member

in simple words, how to control if type should be commented from Platform side, not from the Type side

You no longer can, sorry. If Oracle needs the binary or blob type to be commented, then that should be contributed to doctrine/dbal so that the issue gets fixed for everyone.

@mvorisek
Copy link
Contributor Author

mvorisek commented Jan 21, 2022

the support is deprecated in https://github.com/doctrine/dbal/pull/5058/files#diff-80f8e09bba3385ad6ffd160d565b52166f469a09b0c45b3cb0f93751490a3542L493 , why is the "commented type" API/method removed from Platform?

Platform was always a great middleware to further customize a Type and when an user implements own Platform, he should be not required to reimplement all Types, which is not even (easily) possible in some projects that further extends the standard Types

I am closing this issues as it seemsAbstractPlatform::getColumnComment method can be still overridden and the desired functionality can be impl. in it.

@greg0ire
Copy link
Member

Platform was always a great middleware to further customize a Type and when an user implements own Platform, he should be not required to reimplement all Types, which is not even (easily) possible in some projects that further extends the standard Types

Good point, I don't think we considered that. Anyway, we are probably going to remove comments entirely (see #5107), because they no longer seem useful since platform-aware schema comparison was implemented.

@mvorisek
Copy link
Contributor Author

mvorisek commented Jan 21, 2022

No, please do not!

When for ex. StringType is extended to IpAddressType, with DC2Type comment, the type can be correctly resolved back. Without DC2Type comment, I think this is impossible.

Different Type, even if stored in the same column type, can store the data differently, thus the DC2Type comment holds very important info.

We rely on DC2Type heavily:

image

and even use this info in DB triggers to enforce more integrity of the stored data.

@greg0ire
Copy link
Member

Consider adding a comment to #5107 explaining why you feel it is important to be able to resolve types back from the database.

@morozov morozov closed this as not planned Won't fix, can't repro, duplicate, stale Jul 15, 2022
@github-actions
Copy link

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

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

No branches or pull requests

4 participants