Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[FrameworkBundle][Cache] Allow configuring PDO-based cache pools, with table auto-creation on first use#27694
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
8350ef5 to8536573Comparedmaicher commentedJun 24, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Hmm I can imagine some scenarios where this automatic table creation could have some side-effects. For example:
Not sure how likely a scenario like this is 😄 But it could happen I guess |
nicolas-grekas commentedJun 24, 2018
@dmaicher sure that's possible. Is this any different than the same scenario with the table already created before, and items not saved for the same reason (a rollback?) |
dmaicher commentedJun 24, 2018
I added one point above: |
nicolas-grekas commentedJun 24, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Oh sorry I missed that part. Here are some docs on the topic: We may prefer disabling auto-creation when in the middle of a transaction... Status: needs work |
8536573 tocbe9183Comparenicolas-grekas commentedJun 25, 2018
@dmaicher fixed. |
cbe9183 to583eca6Compare
dmaicher left a comment
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.
one comment for the auto-creation. Otherwise looks good to me 👍
| try { | ||
| $stmt =$conn->prepare($sql); | ||
| }catch (TableNotFoundException$e) { | ||
| if (!$conn->isTransactionActive() ||\in_array($this->driver,array('pgsql','sqlite','sqlsrv'),true)) { |
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.
Ok this looks safer now.
I'm just wondering: What exactly is the use-case now for this auto-creation?
I mean we cannot really rely on it anyway on production as this will simply fail if we are in the middle of a transaction. Sure we can assume eventually there will be a request where this condition is true 😄 But as said we cannot rely on it anyway.
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.
Ease of use. And pgsql users will love it. Note also that even now, no exceptions are thrown when the table is not found. PSR-6 only allows a log line. This doesn't change it.
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.
Ah true about the muted exceptions. Forgot about that.
Yeah for pgsql indeed nice 👍
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.
Hi guys ! seems like issue here...
im using pgsql and TableNotFoundException throwed only when $stmt->execute();
$stmt = $conn->prepare($sql); is not throwing TableNotFoundException when cache_items table does not exists
@nicolas-grekas what do you think?
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 think I will forget about this in one hour :)
Please create an issue if you think there is something to track.
javiereguiluz commentedJun 28, 2018
I'm all in for easing the usage of features ... but"database table auto-creation when the PDO cache needs it" makes me nervous. Not sure if it's a real scenario, but imagine this: I enable PDO cache in production without preparing the database for it:
|
nicolas-grekas commentedJun 28, 2018
no you don't, as explainedin a comment above, PSR-6 allows no error to be reported, so this is silent - or at best logged. That's why this is an improvement over the current situation. |
…h table auto-creation on first use
583eca6 to1484117Comparefabpot commentedJul 9, 2018
Thank you@nicolas-grekas. |
…ache pools, with table auto-creation on first use (nicolas-grekas)This PR was merged into the 4.2-dev branch.Discussion----------[FrameworkBundle][Cache] Allow configuring PDO-based cache pools, with table auto-creation on first use| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | -| License | MIT| Doc PR | -* Allowed configuring PDO-based cache pools via a new `cache.adapter.pdo` abstract service* added automatic table creation when using Doctrine DBAL with PDO-based backendsCommits-------1484117 [FrameworkBundle][Cache] Allow configuring PDO-based cache pools, with table auto-creation on first use
weaverryan commentedJul 15, 2018
Do we need a docs issue? |
…ion (nicolas-grekas)This PR was merged into the master branch.Discussion----------[Cache] tell about PDO configuration and table autocreationDoc forsymfony/symfony#27694Commits-------187300a [Cache] tell about PDO configuration and table autocreation
cache.adapter.pdoabstract service