-
-
Notifications
You must be signed in to change notification settings - Fork 16
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
Add support of older versions of Doctrine/Dbal library #175 #176
Add support of older versions of Doctrine/Dbal library #175 #176
Conversation
0f6db16
to
a397fac
Compare
I got one concern for testing, locally I am able to shuffle the doctrine/dbal version between 2 and 3 by modifying the composer.json, but in CI/CD not sure how we could achieve that. Actually while typing, found this lib: |
@tworzenieweb we are covered with this. |
Can you fix the compatibility in main |
* | ||
* @see https://github.com/doctrine/dbal/blob/3.6.x/UPGRADE.md#upgrade-to-31 | ||
*/ | ||
final class QueryBuilderProxy extends QueryBuilder |
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.
Do we need to extend QueryBuilder
? If they will put final
on the class, this will break.
I am fine with Ecotone's abstraction which will hide the QueryBuilder inside :)
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.
sadly there is no interface to implement to duck type, because some tests are creating QB instance and got a return type defined to be QueryBuilder from Doctrine/Dbal. We could of course change that if you like.
private function getDocumentsFor(string $collectionName): \Doctrine\DBAL\Query\QueryBuilder
{
return (new QueryBuilderProxy($this->getConnection()->createQueryBuilder()))
->select('document', 'document_type')
->from($this->getTableName())
->andWhere('collection = :collection')
->setParameter('collection', $collectionName, Types::TEXT);
}
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.
Ok, I see, let's keep it this way then.
I count on Dbal, that they will provide interface in case of making it final :)
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.
When I think about it now, it might be also a nice starting point if doctrine/dbal 4.0 will ever be released.
a397fac
to
5bbffb9
Compare
nice, wasn't aware of that 👍 |
Thanks for contribution :) |
CI runs against 3.x only (lowest as 3.6.0 to be precise), becauseof dependency on doctrine-bundle in version 2.9. DoctrineBundle 2.9.0 requires dbal 3.6.0. 😎 |
Thanks @marmichalski for taking a look on that :) I've fixed that with #179 |
In order to provide wider support of this library for legacy projects, we should add support for legacy versions of doctrine/dbal library. In particular versions ^2.12.0|^3.0 have a bit different QueryBuilder methods available.
The other small changes are related to accessing the schema manager.