[PropertyInfo] Update DoctrineExtractor for new DBAL 4 BIGINT type #54694
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
| @@ -165,7 +165,7 @@ public static function typesProvider(): array | |||
| return [ | |||
| ['id', [new Type(Type::BUILTIN_TYPE_INT)]], | |||
| ['guid', [new Type(Type::BUILTIN_TYPE_STRING)]], | |||
| ['bigint', [new Type(Type::BUILTIN_TYPE_STRING)]], | |||
| ['bigint', [new Type(Type::BUILTIN_TYPE_STRING)]], // change? separate for DBAL 3 vs 4? | |||
There was a problem hiding this comment.
Yes, Sf 5.4 can be paired with ORM3/DBAL4 but IDK if the team wants a separate test for this? I noticed another test had @requires PHP 8.1, I assumed that it would be better to have one with DBAL3 and one with DBAL4.
There was a problem hiding this comment.
either separate tests, or just an expectation that changes depending on which version is installed - that'd be fine also
There was a problem hiding this comment.
Hey Nicolas,
as Alexander hinted, you can use composer to get installed versions. Do you think this is worth it? Or should I just update the test with a new return expected value?
There was a problem hiding this comment.
Let's rather go with a feature test. There must be some class that's unique to one or the other version.
There was a problem hiding this comment.
For instance, check if the \Doctrine\DBAL\Tools\Console\ConsoleRunner class exists. If it is, we're on DBAL <4.
There was a problem hiding this comment.
You can check if the BigIntType has a getName() method. That should not be the case on DBAL 4.
There was a problem hiding this comment.
Ok, I actually even thought about having 2 extractors, one per DBAL major, and then in the compiler pass to set the respective class with the class exists check.
I will do classmethod exists as it is simple and straightforward, composer checks are fine, but, it implies composer is there to begin with 🤷
There was a problem hiding this comment.
it implies composer is there to begin with 🤷
That does not sound like a strong requirement. 😄
That being said, on this repository, we rather do feature detection than relying on Composer's runtime API, so method_exists() is the way to go here.
Additional Considerations
This issue looks pretty straight forward, but it has had me running in circles not being sure how to exactly interpret it. The new return type to make it work with DBAL 4 is fine, but it is neither an intersection nor a union type, which will cause trouble for other libs if they do not explicitly check each.
There is not easy way to get which DBAL version is the extractor is running against, so trying to optimize the flow is tricky. I am opening this PR to have a starting point in the hopes that maintainers of this package have more historical context than me.
I have tried to document as much as possible about this in the issue linked above. 🍻