Deprecate AbstractPlatform commented type APIs #5058
Conversation
greg0ire
force-pushed
the
deprecate-mark-commented
branch
from
1af9f1e
to
caf6e29
Compare
src/Platforms/DB2Platform.php
Outdated
| @@ -95,6 +95,13 @@ protected function initializeDoctrineTypeMappings() | |||
| */ | |||
| public function isCommentedDoctrineType(Type $doctrineType) | |||
| { | |||
| Deprecation::trigger( | |||
| 'doctrine/dbal', | |||
| 'TODO', | |||
There was a problem hiding this comment.
Dang! I did a @derrabus !
| */ | ||
| public function requiresSQLCommentHint(AbstractPlatform $platform) | ||
| { | ||
| return $platform instanceof DB2Platform; |
There was a problem hiding this comment.
dbal/src/Platforms/DB2Platform.php
Lines 99 to 100 in 71b2e68
AFAIK, only PostgreSQL natively supports boolean columns but they work on all supported platforms. Is it really necessary to use a comment hint on DB2?
There was a problem hiding this comment.
I forgot to copy the comment I found in DB2Platform, it explains why this is necessary.
There was a problem hiding this comment.
Is it really necessary to use a comment hint on DB2?
I have added the comment, and now I am wondering if we shouldn't require a comment on all platforms but PG
There was a problem hiding this comment.
now I am wondering if we shouldn't require a comment on all platforms but PG
I guess it's the opposite and (I misread it as should) we don't need it anywhere (at least with the platform-aware schema comparison in place). We need to find a relevant schema comparison test and see how a scenario of deployment and comparison of a boolean column works say on MySQL.
There was a problem hiding this comment.
You read correctly, my sentence has the same meaning with or without n't (in my intention, now you make me realize what I wrote is properly incorrect english).
I don't understand why you say it would work. If MySQL does not have native support for boolean columns, why would the DBAL reverse engineer to BooleanType and not SmallIntType? Or is schema comparison working and not reverse engineering?
There was a problem hiding this comment.
I'm not certain it does work, I'm just saying I'm not aware of any related issues, so it may work somehow. We need more testing to understand the details. If it doesn't work anywhere except for PostgreSQL, then it's a separate bug.
On the other hand, the changes here are in line with the existing logic, so this unknown probably shouldn't block the deprecation.
There was a problem hiding this comment.
then it's a separate bug.
If it's a bug, then it must be separate because it should target 3.2.x, one more reason not to block this. Plus I have other rabbit holes to dig into :P
greg0ire
force-pushed
the
deprecate-mark-commented
branch
from
caf6e29
to
78ffcf2
Compare
The Type class already has an API for this.
greg0ire
force-pushed
the
deprecate-mark-commented
branch
from
78ffcf2
to
e88ce6d
Compare
The Type class already has an API for this.