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][HttpFoundation][Lock] Fix PDO store not creating table + add tests#52459

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

@HypeMC
Copy link
Member

QA
Branch?5.4
Bug fix?yes
New feature?no
Deprecations?no
Issues-
LicenseMIT

Unlike e.g. SQLite, PostgreSQL doesn't throw an exception on$conn->prepare() if the table is missing, it instead throws it on$stmt->execute(), so the table never gets created.

The table used to get created, but the behavior was broken with#43332.

Comment on lines +336 to +340
switch (true) {
case'pgsql' ===$driver &&'42P01' ===$code:
case'sqlite' ===$driver &&str_contains($exception->getMessage(),'no such table:'):
case'oci' ===$driver &&942 ===$code:
case'sqlsrv' ===$driver &&208 ===$code:
case'mysql' ===$driver &&1146 ===$code:
Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Taken from Doctrine.

@derrabus
Copy link
Member

Can we add a test?

@HypeMC
Copy link
MemberAuthor

HypeMC commentedNov 5, 2023•
edited
Loading

@derrabus Didn't notice postgres was already part of the integration suit, I'll add a test.

@HypeMCHypeMCforce-pushed thefix-pdo-store-create-table branch frome8d10cb to4c13babCompareNovember 5, 2023 19:02
Comment on lines 105 to 127
if ($host =getenv('POSTGRES_HOST')) {
yield'PostgreSQL' => ['pgsql:host='.$host.';user=postgres;password=password'];
}
Copy link
MemberAuthor

Choose a reason for hiding this comment

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

@derrabus This should be ok, without my changes this test fails:

1) Symfony\Component\Lock\Tests\Store\PdoStoreTest::testDsn with data set "PostgreSQL" ('pgsql:host=localhost;user=pos...d=pass')PDOException: SQLSTATE[42P01]: Undefined table: 7 ERROR:  relation "lock_keys" does not existLINE 1: UPDATE lock_keys SET key_expiration = CAST(EXTRACT(epoch FRO...

}catch (\PDOException$e) {
// the lock is already acquired. It could be us. Let's try to put off.
$this->putOffExpiration($key,$this->initialTtl);
if ($this->isTableMissing($e) && (!$conn->inTransaction() ||\in_array($this->driver, ['pgsql','sqlite','sqlsrv'],true))) {
Copy link
MemberAuthor

Choose a reason for hiding this comment

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

I just noticed that thePdoAdapter hasthe same logic but without the$this->isTableMissing() part so I'm wondering, should I add it there as well, or remove it form here to keep it consistent?

Choose a reason for hiding this comment

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

yes indeed, also in DoctrineDbalAdapter, right?

Copy link
Member

Choose a reason for hiding this comment

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

The DBAL adapter catches TableNotFoundException so it is already fine (and the DBAL adapter uses the DBAL API that does the preparation and the execution internally so the try/catch wraps both).

Choose a reason for hiding this comment

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

the try/catch is around the call to prepare, not to execute, that's why I'm wondering

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

I've added the logic toPdoAdapter, theDoctrineDbalAdapter already works as expected. I've added some tests to the adapters/stores to make super everything works as expected.

@HypeMCHypeMCforce-pushed thefix-pdo-store-create-table branch 3 times, most recently from0431dc3 to2d0b487CompareNovember 7, 2023 15:44
@HypeMCHypeMC changed the title[Lock] Fix PDO store not creating table[Cache][Lock] Fix PDO store not creating table + add testsNov 7, 2023
@nicolas-grekas
Copy link
Member

There are some deprecations, are they related?

@HypeMCHypeMCforce-pushed thefix-pdo-store-create-table branch from2d0b487 tocc736c3CompareNovember 7, 2023 17:24
@HypeMC
Copy link
MemberAuthor

There are some deprecations, are they related?

@nicolas-grekas Should be good now.

@nicolas-grekas
Copy link
Member

nicolas-grekas commentedNov 15, 2023•
edited
Loading

@HypeMCHypeMCforce-pushed thefix-pdo-store-create-table branch fromcc736c3 toccdf227CompareNovember 15, 2023 16:55
@HypeMC
Copy link
MemberAuthor

Aren't the remaining deprecations related?

https://github.com/symfony/symfony/actions/runs/6788097634/job/18452337658?pr=52459#step:11:1001

@nicolas-grekas Yes, you're right, took me a while to figure this one out. I initially thought that it wasn't related to my changes cause I didn't make any inDoctrinePostgreSqlIntegrationTest. However, after a little debugging it turns out that the deprecations were being triggered because I never cleaned up the database (dropped the tables) after the tests were done, which then triggered some additional code paths inDoctrinePostgreSqlIntegrationTest that use deprecated Doctrine methods. Sorry for the confusion.

@HypeMCHypeMCforce-pushed thefix-pdo-store-create-table branch fromccdf227 to710c1c9CompareNovember 20, 2023 14:08
@HypeMCHypeMCforce-pushed thefix-pdo-store-create-table branch from710c1c9 tob6e4869CompareNovember 20, 2023 15:10
@HypeMCHypeMCforce-pushed thefix-pdo-store-create-table branch 2 times, most recently from6a4254d tob75880eCompareNovember 20, 2023 15:36
@carsonbotcarsonbot changed the title[Cache][Lock] Fix PDO store not creating table + add tests[Lock] Fix PDO store not creating table + add testsNov 20, 2023
@carsonbotcarsonbot changed the title[Lock] Fix PDO store not creating table + add tests[Cache][HttpFoundation][Lock] Fix PDO store not creating table + add testsNov 20, 2023
@HypeMCHypeMCforce-pushed thefix-pdo-store-create-table branch fromb75880e tocb5d832CompareNovember 20, 2023 15:40
@nicolas-grekas
Copy link
Member

Thank you@HypeMC.

@nicolas-grekasnicolas-grekas merged commit48be4b3 intosymfony:5.4Nov 20, 2023
@HypeMCHypeMC deleted the fix-pdo-store-create-table branchNovember 20, 2023 15:43
nicolas-grekas added a commit that referenced this pull requestNov 21, 2023
…eMC)This PR was merged into the 6.4 branch.Discussion----------[Cache][Lock] `PdoAdapter`/`PdoStore` minor cleanup| Q             | A| ------------- | ---| Branch?       | 6.4| Bug fix?      | no| New feature?  | no| Deprecations? | no| Issues        | -| License       | MITFollow up to#52459.Basically some code cleanup such as using `match` instead of `switch` + a minor sync between the `PdoAdapter` and `PdoStore`.I've targeted 6.4 since this is just a minor code cleanup, but I can switch to 7.1 if needed.Commits-------45e17d5 [Cache][Lock] `PdoAdapter`/`PdoStore` minor cleanup
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@stofstofstof left review comments

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

@GromNaNGromNaNGromNaN approved these changes

@jderussejderusseAwaiting requested review from jderussejderusse is a code owner

Assignees

No one assigned

Projects

None yet

Milestone

5.4

Development

Successfully merging this pull request may close these issues.

6 participants

@HypeMC@derrabus@nicolas-grekas@GromNaN@stof@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp