Problem/Motivation

#2318191: [meta] Database tests fail on SQLite (reason 1) and subsequently #2428695: SQLite date handling is wrongly implemented and arguments handling needs override wrongly assumed that SQLite's custom Statement class was not needed anymore, but it's quite the opposite, both the custom Statement class and its parent, StatementPrefetch, are very still very much needed otherwise every bit of code in Drupal that deals with the database would need to take care of closing the PDOStatement cursor after each SELECT query.

The reason why SQLite's statement class was not used anymore is twofold:
1) #1953800: Make the database connection serializable made the SQLite's connection constructor call to the parent too early, so the explicit NULL value of statementClass was never registered in the parent
2) #2010322: SQLite follow up to #1953800 'Make the database connection serializable' removed Drupal\Core\Database\Driver\sqlite\Connection::prepareQuery(), which was still needed

Proposed resolution

Revert #2428695: SQLite date handling is wrongly implemented and arguments handling needs override and fix the two problems mentioned above.

Remaining tasks

Review the patch.

User interface changes

Nope.

API changes

Nope.

CommentFileSizeAuthor
#1 2465633.patch10.43 KBamateescu

Comments

amateescu’s picture

amateescu commented
Status: Active » Needs review
StatusFileSize
new 2465633.patch10.43 KB

This patch combined with #2454669-7: SQLite: Fix tests in migrate_drupal test group makes most migrate_drupal tests pass, only 1 (out of 90!) is still failing.

dawehner’s picture

dawehner
Primary language German
commented 
  1. +++ b/core/lib/Drupal/Core/Database/Driver/sqlite/Statement.php
@@ -0,0 +1,146 @@
+ *
+ * The PDO SQLite driver only closes SELECT statements when the PDOStatement
+ * destructor is called and SQLite does not allow data change (INSERT,
+ * UPDATE etc) on a table which has open SELECT statements. This is a
+ * user-space mock of PDOStatement that buffers all the data and doesn't
+ * have those limitations.
+ */
+class Statement extends StatementPrefetch implements StatementInterface {

    I really like the explanation! Its adding the kind of knowledge you otherwise don't get. 

  2. +++ b/core/lib/Drupal/Core/Database/Driver/sqlite/Statement.php
@@ -0,0 +1,146 @@
+            // We will remove this placeholder from the query as PDO throws an
+            // exception if the number of placeholders in the query and the
+            // arguments does not match.
+            unset($args[$placeholder]);
+            // PDO allows placeholders to not be prefixed by a colon. See
+            // http://marc.info/?l=php-internals&m=111234321827149&w=2 for
+            // more.
+            if ($placeholder[0] != ':') {
+              $placeholder = ":$placeholder";
+            }

    Can we add a comment why we need that logic as part of the sql specific implementation? It sounds like a bug in user facing code though and we maybe should throw some exception somewhere?

  3. +++ b/core/lib/Drupal/Core/Database/Driver/sqlite/Statement.php
@@ -0,0 +1,146 @@
+    // In some weird cases, SQLite will prefix some column names by the name
+    // of the table. We post-process the data, by renaming the column names
+    // using the same convention as MySQL and PostgreSQL.
+    $rename_columns = array();

    I'm curious whether we could point to some documentation about that bug?

chx’s picture

chx commented

> In some weird cases, SQLite will prefix some column names by the name of the table.

Like, http://sqlite.1065341.n5.nabble.com/PRAGMA-short-column-names-ignored-wh... GROUP BY?

amateescu’s picture

amateescu commented

#2.2: There's no bug in user-facing code, we're just removing the placeholder from the array because it's being inserted straight into the query string in the preg_replace() call below.

#2.3 and #3: I don't really have any special knowledge about that problem, it was part of the original work/patch that added SQLite support to Drupal 7 years ago :)

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community

@amateescu described to me what this problem is about.
Under certain circumstances sqlite doesn't close its PDO cursor, when you don't executed the query. For simple queries this is not the case,
but for larger ones, probably triggered by the amount of data which is part of the migrate tests, so we have already implicit test coverage for that problem,
but as @amateescu said, it would be really tricky to reproduce in a dedicated database test.

Given that, and given that this is basically a revert of a previous issue, I think this is RTBC

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed 1731cd4 and pushed to 8.0.x. Thanks!

  • alexpott committed 1731cd4 on 8.0.x 
    Issue #2465633 by amateescu: Bring back the custom Statement class for...

Status: Fixed » Closed (fixed)

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