-
Notifications
You must be signed in to change notification settings - Fork 439
Support dbal v3 #1169
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
Support dbal v3 #1169
Conversation
pkg/dbal/DbalTypeResolverTrait.php
Outdated
|
|
||
| trait DbalTypeResolverTrait | ||
| { | ||
| protected static function resolveDbalType(string $typeName): string |
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.
This method is compatible for dbal v2 and v3
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.
This makes it a requirement to include in all classes that need to use the compatibility layer between DBAL v2 & v3.
Considering that it would be a static method, there is really no need for it. It can be made a static method of a class on it's own - it does not depend on the class you want to attach it at all.
It also prevents any type of static code analysis from picking up potential errors (previously you had a well defined constant list, now you can't use that because you're depending on reflection to find class constants.
What I'd suggest instead is a "class with side effects". Normally you'd avoid creating classes that change their structure depending on the other libraries, but this would be a compatibility layer that is supposed to be adapting to DBAL version.
It would go like this:
if (class_exists(\Doctrine\DBAL\Types\Types::class)) {
class DbalType {
// constants defined here
}
} else {
class DbalType {
// constants from the other version
}
}Also note that you don't need to use class name as a string. You can use the ::class notation, because using class name / type on it's own does not trigger autoloading.
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.
@Steveb-p
Thanks for your reviews. I will fix it.
using class name / type on it's own does not trigger autoloading.
I did not know its feature, Thanks. I thought it will be shown an error.
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 did not know its feature, Thanks. I thought it will be shown an error.
Yup. PHP is smart enough to figure out what class you're talking about (taking into account current namespace and imports) and doesn't bother with autoloading until it's actually necessary (usually when creating a new instance, calling a static method or passing it to class_exists function and it's "family").
This has the side-effect of also not triggering autoload when doing $x instanceof FooBar (FooBar is not autoloaded), and the same applies to type-hints (even method arguments / property types!). In 99.9% of cases it's not relevant, as at some point you had to have an instance somewhere before (so the class is loaded). This becomes an issue (that 0.01% 😄 ) when you're using class_alias, but that's really a special case.
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.
Sorry for the late commit 🙏 I did fix it.
I'm thinking that do not necessary to use the if statement when creating a new class and copy&paste from the dbal source.
Therefore, I wrote only one class.
Support dbal v3 #1166