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.

CommentFileSizeAuthor
#7 interdiff.txt1.87 KBamateescu
#7 2454625-7.patch8.98 KBamateescu
#5 interdiff.txt836 bytesamateescu
#5 2454625-5.patch7.94 KBamateescu
#2 2454625-2.patch7.93 KBamateescu
#2 fix-sqlite-query.patch3.72 KBamateescu
#1 fix_sqlite_schema_errors.patch1.74 KBamateescu

Comments

amateescu’s picture

amateescu commented
Status: Active » Needs review
StatusFileSize
new fix_sqlite_schema_errors.patch1.74 KB

Test run before:

Drupal test run
---------------

Tests to be run:
  - Drupal\content_translation\Tests\ContentTranslationSettingsTest
  - Drupal\field_ui\Tests\ManageDisplayTest
  - Drupal\file\Tests\FileFieldWidgetTest
  - Drupal\file\Tests\PrivateFileOnTranslatedEntityTest
  - Drupal\image\Tests\ImageFieldDisplayTest
  - Drupal\options\Tests\OptionsFieldUITest
  - Drupal\views\Tests\SearchMultilingualTest

Test run started:
  Wednesday, March 18, 2015 - 14:15

Test summary
------------

Drupal\file\Tests\PrivateFileOnTranslatedEntityTest           32 passes             1 exceptions             
Drupal\content_translation\Tests\ContentTranslationSettingsT 100 passes             2 exceptions             
Drupal\image\Tests\ImageFieldDisplayTest                     188 passes             1 exceptions             
Drupal\file\Tests\FileFieldWidgetTest                        279 passes             1 exceptions             
Drupal\views\Tests\SearchMultilingualTest                     18 passes             1 exceptions             
Drupal\field_ui\Tests\ManageDisplayTest                      226 passes             1 exceptions             
Drupal\options\Tests\OptionsFieldUITest                      277 passes             1 exceptions             

Test run duration: 4 min 39 sec

Test run after:

Drupal test run
---------------

Tests to be run:
  - Drupal\content_translation\Tests\ContentTranslationSettingsTest
  - Drupal\field_ui\Tests\ManageDisplayTest
  - Drupal\file\Tests\FileFieldWidgetTest
  - Drupal\file\Tests\PrivateFileOnTranslatedEntityTest
  - Drupal\image\Tests\ImageFieldDisplayTest
  - Drupal\options\Tests\OptionsFieldUITest
  - Drupal\views\Tests\SearchMultilingualTest

Test run started:
  Wednesday, March 18, 2015 - 14:23

Test summary
------------

Drupal\file\Tests\PrivateFileOnTranslatedEntityTest           52 passes                                      
Drupal\image\Tests\ImageFieldDisplayTest                     217 passes                                      
Drupal\content_translation\Tests\ContentTranslationSettingsT 165 passes                                      
Drupal\file\Tests\FileFieldWidgetTest                        331 passes                                      
Drupal\views\Tests\SearchMultilingualTest                     23 passes                                      
Drupal\field_ui\Tests\ManageDisplayTest                      253 passes                                      
Drupal\options\Tests\OptionsFieldUITest                      295 passes                                      

Test run duration: 5 min 1 sec
amateescu’s picture

amateescu commented
StatusFileSize
new fix-sqlite-query.patch3.72 KB
new 2454625-2.patch7.93 KB

Spent 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 internal handleQueryException() 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 :/

amateescu’s picture

amateescu commented

Here's the batch of additional tests that are fixed by the patch in #2:

Drupal test run
---------------

Tests to be run:
  - Drupal\comment\Tests\CommentFieldsTest
  - Drupal\contact\Tests\ContactStorageTest
  - Drupal\toolbar\Tests\ToolbarAdminMenuTest
  - Drupal\system\Tests\Module\DependencyTest
  - Drupal\user\Tests\Views\HandlerFieldRoleTest

Test run started:
  Thursday, March 26, 2015 - 22:15

Test summary
------------

Drupal\contact\Tests\ContactStorageTest                      366 passes                                      
Drupal\user\Tests\Views\HandlerFieldRoleTest                  10 passes                            1 messages
Drupal\comment\Tests\CommentFieldsTest                       119 passes                                      
Drupal\system\Tests\Module\DependencyTest                    156 passes                                      
Drupal\toolbar\Tests\ToolbarAdminMenuTest                    429 passes                                      

Test run duration: 4 min 4 sec
catch’s picture

catch
he/him
Primary language English
commented
Priority: Normal » Critical

This is independently critical along with the meta issue #2454513: [meta] Make Drupal 8 work with SQLite so promoting.

amateescu’s picture

amateescu commented
StatusFileSize
new 2454625-5.patch7.94 KB
new interdiff.txt836 bytes

I 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 :)

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Needs work 
  1. +++ b/core/lib/Drupal/Core/Database/Connection.php
@@ -562,32 +561,55 @@ public function query($query, array $args = array(), $options = array()) {
+   *
+   * @return null

    As 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) 

  2. +++ b/core/lib/Drupal/Core/Database/Driver/sqlite/Connection.php
@@ -334,6 +333,23 @@ protected function expandArguments(&$query, &$args) {
+    if (!empty($e->errorInfo[1]) && $e->errorInfo[1] === 17) {
+      return $this->query($query, $args, $options);
+    }

    I think the check is safe enough that a recursion here should never happen, so we shouldn't have to protected against that.

amateescu’s picture

amateescu commented
Status: Needs work » Needs review
StatusFileSize
new 2454625-7.patch8.98 KB
new interdiff.txt1.87 KB

Thanks for the review!

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Reviewed & tested by the community

Great, 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.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Excellent. Great work, amateescu!

Committed and pushed to 8.0.x. Thanks!

  • webchick committed cfdf10c on 8.0.x 
    Issue #2454625 by amateescu: SQLite: Fix SQLITE_SCHEMA errors in web...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.