Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
We have a few web tests which fail with the following exception:
Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[HY000]: General error: 17 database schema has changed: SELECT 1 AS expression FROM {cachetags} cachetags WHERE ( (tag = :db_condition_placeholder_0) ); Array ( [:db_condition_placeholder_0] => entity_types ) in Drupal\Core\Cache\CacheTagsInvalidator->invalidateTags() (line 35 of /home/andrei/work/d8.dev/core/lib/Drupal/Core/Cache/CacheTagsInvalidator.php).
Proposed resolution
As indicated by http://www.sqlite.org/faq.html#q15, we need to catch that specific exception and just re-run the query.
Remaining tasks
Review the patch.
User interface changes
Nope.
API changes
Nope.
Comment | File | Size | Author |
---|---|---|---|
#7 | interdiff.txt | 1.87 KB | amateescu |
#7 | 2454625-7.patch | 8.98 KB | amateescu |
#5 | interdiff.txt | 836 bytes | amateescu |
#5 | 2454625-5.patch | 7.94 KB | amateescu |
#2 | 2454625-2.patch | 7.93 KB | amateescu |
Comments
Comment #1
amateescu CreditAttribution: amateescu commentedTest run before:
Test run after:
Comment #2
amateescu CreditAttribution: amateescu commentedSpent some more time today with the other failing tests on SQLite and I found that the fix from this patch is also needed for a few other test classes, and it can be reused by moving it up a level, directly in
\Drupal\Core\Database\Driver\sqlite\Connection::query()
.I'm attaching two patches for this; the first one just copies the parent query method and adds the needed exception handling, but this means we'll have three slightly different copies of
\Drupal\Core\Database\Connection::query()
, each with two-three line differences to the parent implementation.So I think it would be better to factor out the exception handling out of
query()
into another internalhandleQueryException()
method, which allows us to clean things up quite nicely and also improve maintainability a bit:3 files changed, 64 insertions(+), 72 deletions(-)
Edit: edited some sentences that were hard to read :/
Comment #3
amateescu CreditAttribution: amateescu commentedHere's the batch of additional tests that are fixed by the patch in #2:
Comment #4
catchThis is independently critical along with the meta issue #2454513: [meta] Make Drupal 8 work with SQLite so promoting.
Comment #5
amateescu CreditAttribution: amateescu commentedI just noticed that our custom SQLite PDO statement class was doing the same thing in D7 (and in D8 before it was unused and then removed): http://cgit.drupalcode.org/drupal/tree/includes/database/sqlite/database... , so I'm bringing back that check because it's faster than my preg_match().
It would be really helpful to get this patch and #2454729: SQLite: Fix search\Tests\SearchNumbersTest committed soon so we can see how many failures we have left on the temporary SQLite testbot :)
Comment #6
BerdirAs discussed, let's document both this and query() as Drupal\Core\Database\StatementInterface|null|int + a short description there and maybe also where it is called that implementations can either throw an exception or re-run the query, so it can return the same as query(). (or return NULL, apparently)
I think the check is safe enough that a recursion here should never happen, so we shouldn't have to protected against that.
Comment #7
amateescu CreditAttribution: amateescu commentedThanks for the review!
Comment #8
BerdirGreat, this looks good to me. I've verified that it fixes those tests and it is a nice cleanup in query(). Let's get this in so we know where we stand with sqlite.
Comment #9
webchickExcellent. Great work, amateescu!
Committed and pushed to 8.0.x. Thanks!