-
Notifications
You must be signed in to change notification settings - Fork 50
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
Make code compatible (and requiring) Doctrine ORM 2.10 and DBAL 3.0 #76
Conversation
I've just tested the branch in my project and it works well. |
Yes, it works well! You can merge. |
The fork is live in our project and works fine |
We are also using this for Postgres and it is working |
👍 I use it on sf 5.3, doctrine/dbal 3.1.4, doctrine/orm 2.10.2 |
friendly ping @Hikariii |
Thnx for the ping, I missed this somehow. I'll check, merge and release this next week. |
Thanks a lot @Hikariii |
@piotaixr Because of the implemented requirements in the |
I'll do it within the hour. |
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.
Just some minor comments. Thank you so much for your effort.
*/ | ||
protected function validatePlatform(SqlWalker$sqlWalker): void | ||
{ | ||
if (!$sqlWalker->getConnection()->getDatabasePlatform() instanceof PostgreSqlPlatform) { | ||
throw DBALException::notSupported(static::FUNCTION_NAME); | ||
if (!$sqlWalker->getConnection()->getDatabasePlatform() instanceof PostgreSQL94Platform) { |
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.
Json functions are available from postgres 9.3 and this limits future versions of postgres.
I would steer clear from such specific details and just leave this as it was: PostgreSqlPlatform
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 platform does not exist anymore. The parent of PostgreSQL94Platform
is now AbstractPlatform
.
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.
Doesnt look like it to me: https://github.com/doctrine/dbal/blob/3.2.x/src/Platforms/PostgreSQLPlatform.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.
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.
Hmm I'm confused by their versioning. You're right for the 3.0 tag...
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.
Can we resolve this, then?
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.
Can we resolve this, then?
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.
what's about
if (!$sqlWalker->getConnection()->getDatabasePlatform() instanceof PostgreSQL94Platform) { | |
$platform = !$sqlWalker->getConnection()->getDatabasePlatform(); | |
if (!$platform instanceof PostgreSQL94Platform && !$platform instanceof PostgreSQLPlatform) { |
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.
Curious, I tried this locally. I was expecting this to fail with doctrine 3.1. (Doctrine 3.0 cannot be used, constraints from doctrine/orm preventing it)
Now, what I don't know is if its possible for an instance of AbstractPlatform (not Postgresql) to be given to this function. If yes (and the API does not prevent it, so I think its possible), the code will break with somethong else than Doctrine\DBAL\Exception
which violates the interface contract.
I don't understand why you are pushing this much to have the check also done on PostgreSQLPlatform
, can you elaborate more?
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.
In the end we can deal with the deprecation later. I've just merged and released version 5. Thank you for all your effort.
src/Query/AST/Functions/Postgresql/PostgresqlJsonOperatorFunctionNode.php
Show resolved
Hide resolved
56fe1ae
to
98aa95b
Compare
Updated. I hope I didn't mess up anything. |
Thank you, I will do a more extended test this week. I'm still a bit confused by the different |
I just read about the 3.0 release of DBAL here: https://github.com/doctrine/dbal/releases/tag/3.0.0 |
*/ | ||
protected function validatePlatform(SqlWalker$sqlWalker): void | ||
{ | ||
if (!$sqlWalker->getConnection()->getDatabasePlatform() instanceof PostgreSqlPlatform) { | ||
throw DBALException::notSupported(static::FUNCTION_NAME); | ||
if (!$sqlWalker->getConnection()->getDatabasePlatform() instanceof PostgreSQL94Platform) { |
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.
If you support doctrine 3.*, you should support both PostgreSQLPlatform
and PostgreSQL94Platform
.
(same for other platforms)
…as dev dependency
I don't how thos happened, but the tests wouldn't pass until I added a dev dependency on |
This PR updates the code and tests in order to be compatible with Doctrine ORM 2.10 and DBAL 3.0.
This PR contains breaking changes and should result in a new major version.
The implementation of the platform mocks is a dummy one and the strict minimum in order to have green tests.
A lot of imports that were not used are also cleaned.
The package.json file makes sure to require Doctrine ORM and DBAL as this libs depends on them.
Deprecations fixed:
Doctrine\DBAL\DBALException
was renamed toDoctrine\DBAL\Exception
Doctrine\DBAL\Platforms\MySqlPlatform
was renamed toDoctrine\DBAL\Platforms\MySQLPlatform
Doctrine\DBAL\Platforms\PostgreSqlPlatform
does not exist anymore and is replaced byPostgreSql94Platform
andPostgreSql100Platform
.Doctrine\DBAL\Connection#query
now takes astring
as parameter and returns aResult
Doctrine\DBAL\Connection#executeUpdate
now returns an int. MakingConnectionMock#executeUpdate
return0
to have green testsDoctrine\DBAL\Platforms\AbstractPlatform#prefersSequences
does not exist anymoreDatabasePlatformMock#supportsIdentityColumns
return true.Doctrine\DBAL\Driver
has a new method:getExceptionConverter
. Making the method throw in order to have the mock respect the interface and have green tests.Doctrine\DBAL\Driver#getSchemaManager
now takes the platform as second argument.Closes #74