-
Notifications
You must be signed in to change notification settings - Fork 61
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 for doctrine/dbal 4 version #436
Add support for doctrine/dbal 4 version #436
Conversation
654aeb5
to
3e2fec6
Compare
3e2fec6
to
4802f08
Compare
d190e38
to
f697ecd
Compare
f83858f
to
331ee89
Compare
28bf37b
to
dceea0e
Compare
6faa84c
to
8c8e3f5
Compare
8c8e3f5
to
54692bb
Compare
c16c7a0
to
9deb0a5
Compare
$forUpdateSql = ' FOR UPDATE'; // https://github.com/doctrine/dbal/blob/79ea9d6eda8e8e8705f2db58439e9934d8c769da/src/Platforms/AbstractPlatform.php#L1776 | ||
if ($this->getConnection()->getDatabasePlatform() instanceof SqlitePlatform) { | ||
$forUpdateSql = ''; // https://github.com/doctrine/dbal/blob/79ea9d6eda8e8e8705f2db58439e9934d8c769da/src/Platforms/SqlitePlatform.php#L753 | ||
} elseif ($this->getConnection()->getDatabasePlatform() instanceof SQLServerPlatform) { | ||
$forUpdateSql = ''; // https://github.com/doctrine/dbal/blob/79ea9d6eda8e8e8705f2db58439e9934d8c769da/src/Platforms/SQLServerPlatform.php#L1625 | ||
} | ||
|
||
$query = 'SELECT path, id FROM phpcr_nodes WHERE path LIKE ? OR path = ? AND workspace_name = ? '.$forUpdateSql; |
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.
Have you tried leveraging the query builder for adding lock hints?
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.
Not familiar with that. @dbu how would you change this part?
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 am also not familiar with the dbal query builder. did doctrine drop the getForUpdateSQL method? does the changelog not explain what to do instead?
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.
does the changelog not explain what to do instead?
1fd59eb
to
607ff5d
Compare
thanks for the work! good progress already! oh no mysql and its encoding mess :( can we be more explicit about encoding to not rely on defaults? but we would need to avoid messing up with existing legacy setups, which can be tricky. as this is 2.x we could do a BC break to require an explicit encoding configuration with mysql. i am worried about the deprecation warning spammed over the test output, we seem to use the ObjectManager in a weird way. |
Yeah mysql encoding part is very stricky as we don't know what is correct encoding for our case sensitive field now. I also did not yet found out why dbal skipping set a default charset in the new version. Maybe @derrabus know more here. I'm also not familiar enough with the code. The current |
I did rethink about the We could maybe go the way throw a error if no
Just with some friendly error message jackalope-doctrine-dbal requires a defined charset definition, please configure it in your dbal connection via |
that is indeed an idea, yeah. we should validate in the setup already - afaik the dbal connection is injected to the client with the params already in it, but we could check the params there. that would provide a better stack trace than doing it in the middle of everything. and we also need to update the documentation and examples, also for the bundle configuration. |
wrapped up in #439 😅 |
@dbu Thx |
This adds a CI run against
@dev
which we should I think have to get earlier when a dependency would break us.Also it adds
doctrine/dbal: ^4
to check what we require todo there so we can keep eventually BC breaks in mind for this jackaleop-doctrine-dbal 2.0 release.TODO
TypeError: Doctrine\DBAL\Platforms\AbstractPlatform::modifyLimitQuery(): Argument #3 ($offset) must be of type int, null given, called in /Users/alexanderschranz/Documents/Projects/jackalope-doctrine-dbal/src/Jackalope/Transport/DoctrineDBAL/Query/QOMWalker.php on line 1
PHPUnit\Framework\MockObject\IncompatibleReturnValueException: Method createSchemaConfig may not return value of type NULL, its declared return type is "Doctrine\DBAL\Schema\SchemaConfig"
PHPUnit\Framework\MockObject\MethodCannotBeConfiguredException: Trying to configure method "getSchemaManager" which cannot be configured because it does not exist, has not been specified, is final, or is static
Doctrine\DBAL\Schema\Exception\IndexAlreadyExists: An index with name "" was already defined on table "phpcr_nodes".
TypeError: Doctrine\DBAL\Schema\Table::addForeignKeyConstraint(): Argument #1 ($foreignTableName) must be of type string, Doctrine\DBAL\Schema\Table given, called in /Users/alexanderschranz/Documents/Projects/jackalope-doctrine-dbal/src/Jackalope/Transport/DoctrineDBAL/RepositorySchema.php on line
Doctrine\DBAL\Exception\InvalidColumnType\ColumnLengthRequired: Doctrine\DBAL\Platforms\MySQLPlatform requires the length of a VARCHAR column to be specified
Symfony\Component\Cache\Exception\InvalidArgumentException: Cache key "workspace:_default" contains reserved characters "{}()/\@:".
happens on PHP 8.1, Symfony 6.4, see composer.lock belowFailed asserting that exception of type "PHPCR\RepositoryException" matches expected exception "PHPCR\ReferentialIntegrityException". Message was: "Error inside the transport layer: An exception occurred while executing a query: SQLSTATE[42000]: Syntax error or access violation: 1253 COLLATION 'utf8_bin' is not valid for CHARACTER SET 'latin1'" at
I'm not sure but I think we should maybe for utf8mb4 for mysql DBAL 4 MySQL COLLATION 'utf8_bin' is not valid for CHARACTER SET 'utf8mb4'" at doctrine/dbal#6281utf8
is not longer mysql default charset in doctrine/dbal 4. there is not longer a charset forced instead the default one used. Which ends in mysql 5.7 inlatin1
and8.0
inutf8mb4
but we not know about it so we currently accidently defineutf8_bin
instead ofutf8mb4_bin
as charset.composer.lock