[Cache][HttpFoundation][Lock] Fix PDO store not creating table + add tests #52459
Conversation
| 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: |
|
Can we add a test? |
|
@derrabus Didn't notice postgres was already part of the integration suit, I'll add a test. |
HypeMC
force-pushed
the
fix-pdo-store-create-table
branch
from
e8d10cb
to
4c13bab
Compare
| if ($host = getenv('POSTGRES_HOST')) { | ||
| yield 'PostgreSQL' => ['pgsql:host='.$host.';user=postgres;password=password']; | ||
| } |
There was a problem hiding this comment.
@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 exist
LINE 1: UPDATE lock_keys SET key_expiration = CAST(EXTRACT(epoch FRO...
| @@ -127,8 +127,19 @@ public function save(Key $key) | |||
| try { | |||
| $stmt->execute(); | |||
| } 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))) { | |||
There was a problem hiding this comment.
I just noticed that the PdoAdapter has the 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?
There was a problem hiding this comment.
yes indeed, also in DoctrineDbalAdapter, right?
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
the try/catch is around the call to prepare, not to execute, that's why I'm wondering
There was a problem hiding this comment.
I've added the logic to PdoAdapter, the DoctrineDbalAdapter already works as expected. I've added some tests to the adapters/stores to make super everything works as expected.
HypeMC
force-pushed
the
fix-pdo-store-create-table
branch
3 times, most recently
from
0431dc3
to
2d0b487
Compare
HypeMC
changed the title
|
There are some deprecations, are they related? |
HypeMC
force-pushed
the
fix-pdo-store-create-table
branch
from
2d0b487
to
cc736c3
Compare
@nicolas-grekas Should be good now. |
|
Aren't the remaining deprecations related? https://github.com/symfony/symfony/actions/runs/6788097634/job/18452337658?pr=52459#step:11:1001 |
HypeMC
force-pushed
the
fix-pdo-store-create-table
branch
from
cc736c3
to
ccdf227
Compare
@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 in |
src/Symfony/Component/Cache/Tests/Adapter/DoctrineDbalAdapterTest.php
Outdated
Show resolved
Hide resolved
HypeMC
force-pushed
the
fix-pdo-store-create-table
branch
from
ccdf227
to
710c1c9
Compare
src/Symfony/Component/Cache/Tests/Adapter/DoctrineDbalAdapterTest.php
Outdated
Show resolved
Hide resolved
HypeMC
force-pushed
the
fix-pdo-store-create-table
branch
from
710c1c9
to
b6e4869
Compare
src/Symfony/Component/Cache/Tests/Adapter/DoctrineDbalAdapterTest.php
Outdated
Show resolved
Hide resolved
HypeMC
force-pushed
the
fix-pdo-store-create-table
branch
2 times, most recently
from
6a4254d
to
b75880e
Compare
carsonbot
changed the title
carsonbot
changed the title
HypeMC
force-pushed
the
fix-pdo-store-create-table
branch
from
b75880e
to
cb5d832
Compare
|
Thank you @HypeMC. |
…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 | MIT Follow 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
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.