-
Notifications
You must be signed in to change notification settings - Fork 2
add support for doctrine/dbal ^3.2 #5
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
Conversation
e3b89df
to
a66709f
Compare
.gitignore
Outdated
@@ -1,3 +1,5 @@ | |||
.idea |
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.
please store this in local global gitignore
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.
done. Thanks for hint
@@ -63,7 +63,7 @@ public function isEnumNameTypeMapping(): bool | |||
/** | |||
* {@inheritdoc} | |||
*/ | |||
public function convertToPHPValue($value, AbstractPlatform $platform) | |||
public function convertToPHPValue($value, AbstractPlatform $platform): ?Enum |
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.
I'm not sure whether this work if doctrine/dbal
project introduce signatures on their classes at some point. I think we should avoid this at the moment for interfaces\overrides\etc
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.
They already did: it is already marked as "mixed" in 3.3
https://github.com/doctrine/dbal/blob/3.3.x/src/Types/Type.php#L83 in PHPDoc typehint
and it will be real mixed in 4.x:
https://github.com/doctrine/dbal/blob/4.0.x/src/Types/Type.php#L87
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.
Good point. Did a verification test, looks OK https://3v4l.org/DLWKM
@@ -13,7 +13,7 @@ final class EnumTypeInitializer | |||
* @param NamingStrategyInterface|null $strategy | |||
* @param bool $enumNameTypeMapping | |||
* | |||
* @throws \Doctrine\DBAL\DBALException | |||
* @throws \Doctrine\DBAL\Exception |
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.
there is no such exception in doctrine/dbal: ^2
, but we still allow it
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.
Yes there is. Starting from 2.11, that is why ^2.11
https://github.com/doctrine/dbal/blob/2.11.0/lib/Doctrine/DBAL/Exception.php
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.
👍 Tag sorting confused me. v2.10.2 is top in tag list for me, checked that one
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 most of these changes are not mandatory to support dbal ^3, and some of these changes must be avoided until dbal introduces own signatures on their methods for Type
class.
And some other minor changes requested
a66709f
to
58e4afa
Compare
Exception replacement is "mandatory" as PHPDoc can be mandatory.
|
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.
👍 LGTM
Thanks @XilefNori! |
No description provided.