Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[Cache] Fix DBAL deprecations#50507

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged

Conversation

@MatTheCat
Copy link
Contributor

@MatTheCatMatTheCat commentedMay 31, 2023
edited
Loading

QA
Branch?5.4
Bug fix?yes
New feature?no
Deprecations?no
TicketsPart of#50481
LicenseMIT
Doc PRN/A

Unfortunately callingTable::getColumns will still produce deprecations 🤔

@nicolas-grekasnicolas-grekasforce-pushed thedoctrine_deprecations_cache branch from2c6dd3b tobec5302CompareMay 31, 2023 14:01
Copy link
Member

@nicolas-grekasnicolas-grekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

The remaining deprecation should be fixed on doctrine/dbal I guess...

Indirect deprecation triggered by Symfony\Component\Cache\Tests\Adapter\DoctrineDbalAdapterTest::testConfigureSchemaDecoratedDbalDriver:Doctrine\DBAL\Schema\Table::getPrimaryKeyColumns is deprecated. Use getPrimaryKey() and Index::getColumns() instead. (Table.php:816 called by Table.php:708, https://github.com/doctrine/dbal/pull/5731, package doctrine/dbal)Stack trace:#0 [internal function]: Symfony\Bridge\PhpUnit\DeprecationErrorHandler->handleError(16384, '...', '...', 195)#1 vendor/doctrine/deprecations/lib/Doctrine/Deprecations/Deprecation.php(195): trigger_error('...', 16384)#2 vendor/doctrine/deprecations/lib/Doctrine/Deprecations/Deprecation.php(99): Doctrine\Deprecations\Deprecation::delegateTriggerToBackend('...', Array, '...', '...')#3 vendor/doctrine/dbal/src/Schema/Table.php(816): Doctrine\Deprecations\Deprecation::trigger('...', '...', '...', '...')#4 vendor/doctrine/dbal/src/Schema/Table.php(708): Doctrine\DBAL\Schema\Table->getPrimaryKeyColumns()#5 vendor/doctrine/dbal/src/Platforms/AbstractPlatform.php(2066): Doctrine\DBAL\Schema\Table->getColumns()#6 vendor/doctrine/dbal/src/Platforms/AbstractPlatform.php(2040): Doctrine\DBAL\Platforms\AbstractPlatform->buildCreateTableSQL(Object(Doctrine\DBAL\Schema\Table), true, true)#7 vendor/doctrine/dbal/src/Platforms/SqlitePlatform.php(979): Doctrine\DBAL\Platforms\AbstractPlatform->getCreateTableSQL(Object(Doctrine\DBAL\Schema\Table), 3)#8 vendor/doctrine/dbal/src/Platforms/SqlitePlatform.php(915): Doctrine\DBAL\Platforms\SqlitePlatform->getCreateTableSQL(Object(Doctrine\DBAL\Schema\Table))#9 vendor/doctrine/dbal/src/SQL/Builder/CreateSchemaObjectsSQLBuilder.php(65): Doctrine\DBAL\Platforms\SqlitePlatform->getCreateTablesSQL(Array)#10 vendor/doctrine/dbal/src/SQL/Builder/CreateSchemaObjectsSQLBuilder.php(32): Doctrine\DBAL\SQL\Builder\CreateSchemaObjectsSQLBuilder->buildTableStatements(Array)#11 vendor/doctrine/dbal/src/Schema/Schema.php(433): Doctrine\DBAL\SQL\Builder\CreateSchemaObjectsSQLBuilder->buildSQL(Object(Doctrine\DBAL\Schema\Schema))#12 src/Symfony/Component/Cache/Adapter/DoctrineDbalAdapter.php(125): Doctrine\DBAL\Schema\Schema->toSql(Object(Doctrine\DBAL\Platforms\SqlitePlatform))#13 src/Symfony/Component/Cache/Tests/Adapter/DoctrineDbalAdapterTest.php(70): Symfony\Component\Cache\Adapter\DoctrineDbalAdapter->createTable()

@nicolas-grekas
Copy link
Member

Thank you@MatTheCat.

@nicolas-grekasnicolas-grekas merged commitfb32b92 intosymfony:5.4May 31, 2023
@MatTheCatMatTheCat deleted the doctrine_deprecations_cache branchMay 31, 2023 14:05
This was referencedJun 26, 2023
@drupol
Copy link
Contributor

drupol commentedMar 6, 2024
edited
Loading

Hello!

Today, my colleague@samsonradu and I noticed an issue regarding theDoctrineDbalAdapter, potentially introduced by this PR.

Basically, by making some parameters lazily injected, the order of those parameters are no more preserved, and we end up with totally broken SQL queries. Here are more details:

Failed to save key "ulm_ac_rbac_action_roles" of type array: An exception occurred while executingMERGE INTO cache_items USING DUAL ON (item_id = ?) WHEN NOT MATCHED THEN INSERT (item_id, item_data, item_lifetime, item_time) VALUES (?, ?, ?, ?) WHEN MATCHED THEN UPDATE SET item_data = ?, item_lifetime = ?, item_time = ? with params [  3600,   1709126855,   3600,   1709126855,   "EeA4QGjhnZ:ulm_ac_rbac_action_roles",   "EeA4QGjhnZ:ulm_ac_rbac_action_roles",   "\x00...\x69..."]

The introduction of a lazy loading mechanism in this PR has led to issues with the ordering of bound values in SQL statements. New keys added by the lazy loading process are appended at the end of the parameters array, disrupting the intended logical order. See it here:

/app/vendor/symfony/cache/Adapter/DoctrineDbalAdapter.php:354:object(Doctrine\DBAL\Statement)[1878]  protected 'sql' => string 'MERGE INTO cache_items USING DUAL ON (item_id = ?) WHEN NOT MATCHED THEN INSERT (item_id, item_data, item_lifetime, item_time) VALUES (?, ?, ?, ?) WHEN MATCHED THEN UPDATE SET item_data = ?, item_lifetime = ?, item_time = ?' (length=223)  protected 'params' =>     array (size=8)      4 => int 3600      5 => int 1709713419      7 => int 3600      8 => int 1709713419      1 => string 'q55PWWD1R0:ulm_ac_rbac_action_roles' (length=35)      2 => string 'q55PWWD1R0:ulm_ac_rbac_action_roles' (length=35)      3 => string 'value1'      6 => string 'value1'

In here, you can see that theparams are not correctly ordered and this is breaking the SQL query.

IMHO, I think this issue should be fixed indoctrine/dbal with a simpleksort onparams, but I'm not sure, that's the reason why I comment in here.
Alternatively, we could implement explicit parameter mapping instead of positional indexes to ensure each bound value maintains its correct order, regardless of when it's added.

From thedoctrine/dbal doc, I see:

Support for positional and named prepared statements varies between the different database extensions. PDO implements its own client side parser so that both approaches are feasible for all PDO drivers. OCI8/Oracle only supports named parameters, but Doctrine implements a client side parser to allow positional parameters also.

Maybe this is something to fix somewhere else?

I created a draft PR to show you what I have in mind at:#54172, just let me know what you think.

samsonradu and jeanseba reacted with thumbs up emoji

@drupoldrupol mentioned this pull requestMar 6, 2024
@drupol
Copy link
Contributor

drupol commentedMar 7, 2024
edited
Loading

Hey all,

I wanted to provide an update regarding the issue I reported.

After further investigation and numerous attempts to reproduce the issue indoctrine/dbal#6328, including starting from a blank application state and trying to replicate the conditions under which the issue was initially observed, my colleague and I have come to a conclusion that we are now unable to reproduce the issue in any form. Perhaps, this indicates that the problem may have been influenced by factors or states within our specific project setup that we could not accurately replicate in isolated testing, but as of today, we are totally unable to reproduce it.

Edit: We've discovered a rational explanation for the behaviour we observed. The root cause of the confusion stems from thecache_items table being manually created with incorrect column types in our environment. This discrepancy was not immediately evident, leading to our initial belief that there was a problem with theDoctrineDbalAdapter. With this new understanding, I want to clarify that the issue was not with thedoctrine/dbal or the changes introduced by the mentioned PR. The anomaly was due to our project-specific database schema misconfiguration.

Given this, the issue I reported is a false alarm. TheDoctrineDbalAdapter behaves as expected. I want to extend my apologies for any confusion or concern my initial report may have caused.

I believe it is now appropriate to close the associated PRs (#54172 anddoctrine/dbal#6328), unless you think replacing positional parameters with named parameters worth it.

Thanks.

samsonradu and derrabus reacted with thumbs up emoji

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

Assignees

No one assigned

Projects

None yet

Milestone

5.4

Development

Successfully merging this pull request may close these issues.

4 participants

@MatTheCat@nicolas-grekas@drupol@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp