Problem/Motivation

Drupal creates a new SQLite database file for each simpletest test case but it fails to delete them at the end of the test.

Proposed resolution

Make \Drupal\Core\Database\Schema::findTables() use unprefixed tables as an argument and for the return value.

Remaining tasks

None.

User interface changes

Nope.

API changes

\Drupal\Core\Database\Schema::findTables() works with prefixed tables by default.

Data model changes

Nope.

CommentFileSizeAuthor
#100 1713332-100.patch15.16 KBmcdruid
#100 interdiff-1713332-97-100.txt3.03 KBmcdruid
#97 1713332-97.patch15.47 KBmcdruid
#97 interdiff-1713332-96-97.txt340 bytesmcdruid
#96 1713332-96.patch15.33 KBmcdruid
#96 interdiff-1713332-92-96.txt5.58 KBmcdruid
#94 drupalci-yml-with-patch-17173332-92.patch809 bytesTR
#92 1713332-92.patch13.86 KBmcdruid
#92 interdiff-1713332-83-92.txt1.03 KBmcdruid
#83 1713332-83.patch13.02 KBmarcelovani
#76 1713332-76.patch13.01 KBpietmarcus
#74 1713332-74.patch12.38 KBpietmarcus
#61 interdiff.txt1.58 KBamateescu
#61 1713332-61.patch17.1 KBamateescu
#59 interdiff.txt1.36 KBamateescu
#59 1713332-59.patch16.76 KBamateescu
#57 interdiff.txt2.88 KBamateescu
#57 1713332-57.patch16.43 KBamateescu
#54 interdiff.txt3.14 KBamateescu
#54 1713332-54.patch16.4 KBamateescu
#53 interdiff.txt6.8 KBamateescu
#53 1713332-53.patch15.5 KBamateescu
#44 1713332-44.patch11.27 KBbzrudi71
#40 1713332-40.patch11.27 KBamateescu
#33 drupal-sqlite_simpletest-1713332-33.patch4.46 KBXen
#21 drupal-simpletest-fails-to-drop-tables-sqlite-1713332-21.patch2.05 KBXen
#13 1713332-13-fix-sqlite-find-table-D7.patch2.07 KBczigor
#11 1713332-fix-sqlite-find-table.patch2.1 KBznerol
#6 sqlite_drop_db-6.patch1.76 KBpp
sqlite_drop_db.patch1.63 KBdroplet
#1 improve-sqlite-teardown-1713332-1.patch1.81 KBwulff
do-testbot-accept-phpstorm-patch.patch1.71 KBdroplet

Issue fork drupal-1713332

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

wulff’s picture

I'm having the same problem when trying to run D7 tests on sqlite.

I have attached a patch for D7 which is based on the above patch for D8.

When running with the patch, the temporary database is deleted as expected, and I no longer get the "Failed to drop all prefixed tables." error.

droplet’s picture

@wulff,

can you review D8 ? so we can move forward and backported to D7 :)

Thanks.

droplet’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, improve-sqlite-teardown-1713332-1.patch, failed testing.

droplet’s picture

Status: Needs work » Needs review

Failed patch is D7. Please review #0 sqlite_drop_db.patch

pp’s picture

FileSize
1.76 KB
+++ b/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.php
@@ -774,18 +774,23 @@ abstract class WebTestBase extends TestBase {
+      file_unmanaged_delete($connection_info['default']['database']);

This delete the live database. Add prefix.

+      file_unmanaged_delete($connection_info['default']['database'] .'-'. $connection_info['default']['prefix']['default']);

I made a following:
Install a Drupal whit a minimal profile
Switch on the testing module
Run a test (Field types -> Text translation because I'm on the D8MI CodeSprint)

before patch:
I got 2 error: "Failed to find test tables to drop."
I found two file in the sites/default/files whit .ht.sqlite- prefix (the default sqlite file name)

after patch
I didn't get an error
The files of test database is deleted.

A attached a corrected patch.

zserno’s picture

Status: Needs review » Reviewed & tested by the community

I ran into the same issue today with latest D8 HEAD. Patch in #6 looks good and works great on my local.

gaas’s picture

I ran into the same issue. The patch fixes the problem for me as well. I agree that the patch looks sensible. One small improvement could be not to delete that empty line before parent::tearDown() (at the end of the patch).

catch’s picture

Status: Reviewed & tested by the community » Needs work

Is there absolutely no way to fix this without special-casing sqlite in simpletest?

Damien Tournoud’s picture

We are supposed to have all the code in place for this to work.

See:

  • sqlite\Schema::findTables() that properly takes into account multiple databases
  • sqlite\Database::__destruct() that is supposed to remove the empty database files

If that's not working, someone needs to investigate why.

znerol’s picture

The problem is the sqlite implementation of findTable:

[...]
   public function findTables($table_expression) {
    // Don't add the prefix, $table_expression already includes the prefix.
    $info = $this->getPrefixInfo($table_expression, FALSE);
[...]

Here we call getPrefixInfo with a table-expression. However this method is declared as:

getPrefixInfo($table = 'default', $add_prefix = TRUE)

Note that the parameter $table is supposed to be a single, non-prefixed table, while we pass a prefixed table expression. Actually most of the time we will get the right information, because the default prefix will be returned if the table cannot be found.

From reading the code it seems that getPrefixInfo is supposed to cope with the condition when table names including the prefix are passed to the $table parameter. When a '.' is fonud in $table it will split it into the schema and table fields of the result array. However I observed that this condition is not met when tables are to be removed after a test run. In this cases $table_expression looks like simpletest388380%. Note: no dot.

For drupal 8 we have plenty of options to remove the problem. I'd favor a solution where we don't mix prefix and table-pattern. E.g.:

public function findTables($table_pattern, $prefix = array())

Or even

public function findTables($table_pattern, $prefix = NULL)

If empty($prefix) fall back to the default.

For drupal 7 and for fixing the current situation, I suggest working around this problem directly in the sqlite version of findTables.

Patch attached. However I need advice on handling errors (substitute error_log with something sensible in this situation).

znerol’s picture

Status: Needs work » Needs review
czigor’s picture

I met the same bug on D7. The patch in #11 worked for me, so enjoy the same for D7.

Status: Needs review » Needs work

The last submitted patch, 1713332-13-fix-sqlite-find-table-D7.patch, failed testing.

czigor’s picture

Status: Needs work » Needs review

Dear bot, don't test my patch, it's for D7. It's only for later reference.

apassi’s picture

Cant say about D8, but i think this is little messy for D7. Currently i have spend whole day to investigate this, and if i am understand correctly, the prefix with SQLITE is implemented by "attached tables", which seams to mean, that drupal and sqlite will create totally different database file. Now, what that mean in simple test:

"drush runserver"

now there is this database.sqlite

"drush test-run Foobar"

D & S will create database file 'database.sqlite-simpletest34324', the -simpletest34324 is the prefix.
if there is simple tests in Foobar like $this->assertTrue(TRUE); it will pass, but if we are doing HTTP requests:

$this->drupalGet('user');

.. then it goes messy, because drush runserver will start new PHP-CGI process, which reads the original file name 'database.sqlite' from settings.php, and it does not have any idea we are running tests, and does not know that it should read 'database.sqlite-simpletest34324' file.

It is possible that i am wrong, but this is what i have found today, and currently i think there is no way to simpletest with sqlite currently. In D7.

czigor’s picture

Hi apassi,

I have not tried simpltetest+sqlite with drush, but from a browser it works for me (and I do $this->drupalGet()) a lot).

Is browser not an option for you? (until someone fixes sqlite for drush)

znerol’s picture

@apassi: The problem is not the db-prefix itself, that one is passed to subsequent requests running from within a test-function. The problem is due to mixed up semantics simpletest will simply fail to cleanup the attached tables after a test-run (see #11).

If you run into problems executing tests using drush runserver, then this is probably a different issue.

apassi’s picture

Ok, 15 hours with this issue, and it seems that i was wrong.. simpletest drupalGet etc. track current table prefix with HTTP headers.

For me this really was an issue with drush rs, and i can confirm that patch #13 works, and is needed.

Damien Tournoud’s picture

Component: simpletest.module » sqlite database
Status: Needs review » Needs work

Ok, looking at the code, I understand why this fails: the findTables() implementation of SQLite doesn't return fully-qualified tables, but only the table name.

The only part of #11 that is required is the bottom half:

+
+    // The table prefix needs to be prepended to the table list. Also sqlite
+    // system tables need to be excluded.
+    $tables = array();
+    $hide_tables = array('sqlite_sequence');
+    foreach ($result->fetchAllKeyed(0, 0) as $table) {
+      if (in_array($table, $hide_tables)) {
+        continue;
+      }
+
+      $prefixed_table = $info['schema'] . $table;
+      $tables[$prefixed_table] = $prefixed_table;
+    }
+
+    return $tables;
   }

The code of the top half is actually correct and should not be changed.

Xen’s picture

@Damien Tournoud

Not quite. At least for D7.

Firstly, the prefix is $info['prefix'] not $info['schema']. But that doesn't fix things as SQLite adds a period to the prefix (as it implements prefixes by using ATTACH in SQLite), which DrupalWebTestCase::tearDown() (and others) doesn't expect. tearDown() removes the prefix configuerd in the connection info, but that still leaves the period in the table name, which causes db_drop_table() to fail.

We can't remove the period from DatabaseConnection_sqlite::__contruct, as that would require that all SQLite queries should guess whether prefixtablename is really prefix.tablename or not.

The attched patch hacks around the problem, it's not pretty, but I think it's the best we can do without a major rewrite of the prefix handling in SQLite, which I somewhat doubt will ever get into D7.

As for D8, we'd preferably figure out a better solution, but how do we get around getting something like this into D7? As I said, I doubt a major rewrite of the prefixing logic will get backported, but this hack will probably not get into D8 either, so what are we supposed to do?

Damien Tournoud’s picture

@Xen: you are confusing things. DatabaseSchema::findTables() is supposed to return fully-qualified table names. It doesn't on SQLite. That's the bug.

As you point out, the code I quoted is not correct, as it is missing the dot, but otherwise the idea is sound. We need something like:

+    // We need to return fully-qualified table names. Also sqlite
+    // system tables need to be excluded.
+    $tables = array();
+    $hide_tables = array('sqlite_sequence');
+    foreach ($result->fetchColumn(0) as $table_name) {
+      if (in_array($table_name, $hide_tables)) {
+        continue;
+      }
+
+      $full_table_name = $info['schema'] . "." . $table_name;
+      $tables[$full_table_name] = $full_table_name;
+    }
Xen’s picture

@Damien Tournoud
In that case, could you explain what you mean by "fully-qualified table names"?

I tried adding the dot, early on when I was still trying to figure it out. It didn't work.

It is pretty clear that DrupalWebTestCase::tearDown() expects that "fully-qualified table names" means table names prefixed by the configured prefix (see http://drupalcode.org/project/drupal.git/blob/refs/heads/7.x:/modules/si... ).

It is also clear to me that $info['schema'] is not the table prefix. The value I've seen most of the day while debugging this, is "main", which doesn't have much to do with the prefixes or table names in action here.

Thirdly, the added period to the full table name only ensures that tearDown will *not* be able to delete the tables as it strips the prefix *without* a period from the table name before trying to delete the table, which means that it'll end up trying to delete ".variable", etc. I've confirmed this by adding debugging statements. Again, the period is added internally to SQLite (here: http://drupalcode.org/project/drupal.git/blob/refs/heads/7.x:/includes/d... ), and is never seen by DrupalWebTestCase::tearDown().

I have SQLite tests that works from drush test-runs, which didn't when I tried the code you're suggesting, including the latste amendment, so I'm not just pulling this out from a hat.

Damien Tournoud’s picture

Here is how it is supposed to work:

  • tearDown() calls db_find_tables($connection_info['default']['prefix']['default'] . '%');, which translates to db_find_tables("sXXXXXXXX.%")
  • findTables() calls getPrefixInfo(), which breaks "sXXXXXXXX.%" into schema: "sXXXXXXXX" and table: "%"
  • findTables() queries the master catalog, which returns table names like "node", "user", etc.
  • findTables() prefixes the table name with the schema name (aka. "sXXXXXXXX"), so we end up with "sXXXXXXXX.node" and "sXXXXXXXX.user"
  • tearDown() strips out the prefix and calls db_drop_table(), which re-prefix the table and deletes it

This looks ugly and convoluted, but that's because supporting virtual table names (ie. table names before prefixing takes place) in db_find_tables() would be really hard, so this function is only using real table names (ie. table names after prefixing takes place).

Xen’s picture

tearDown() calls db_find_tables($connection_info['default']['prefix']['default'] . '%');, which translates to db_find_tables("sXXXXXXXX.%")

It doesn't. Simpletest sets the prefix to "simpletest123", so it translates to db_find_tables("simpletest123%").

findTables() calls getPrefixInfo(), which breaks "sXXXXXXXX.%" into schema: "sXXXXXXXX" and table: "%"

It doesn't as there's no period.

tearDown() strips out the prefix and calls db_drop_table(), which re-prefix the table and deletes it

tearDown() doesn't, due to the missing period. db_drop_table() does, when it gets the right tablename.

Xen’s picture

@Damien Tournoud
Regardless of what you'd like db_find_tables() to return, the fact of the matter is that mysql returns bare table names, with the prefix, and that's what tearDown() expects. If you want to have DatabaseSchema_sqlite::findTables() return FQ table names, you'll have to change tearDown() to deal with that, and the default DatabaseSchema::findTables(), because the latter don't give a damn about FQ names but simply returns all tables matching the patternt (and that would include the prefix).

It might be more correct from an architecture view to return FQ names, but I think that fixing up findTables() for all database variants in order to fix one database type would be considered a bit out of order for a D7 patch.

Damien Tournoud’s picture

Indeed, I was a bit broad in my previous statement. The SQLite implementation of findTables() need to return fully-qualified table names, but just in this particular case of database prefixes used during testing. Essentially right now we are returning unprefixed table names in this particular case, which is one of the reasons this doesn't work.

The other reason might be that Simpletest doesn't read the correct prefix, but I haven't checked if that's true.

Xen’s picture

Simpletest gets the prefix that was configured for the db connection, the problem is that SQLite fudges that and adds the period due to internals. I don't think that SQLite can reach back and "fix" the connection prefix, nevermind that being a creepy thought..

The SQLite implementation of findTables() need to return fully-qualified table names, but just in this particular case of database prefixes used during testing.

You lost me there. Should it return FQ names ("main.variables") or prefixed ("simpletest123variables")? If it's FQ names, we'll have to mokey-patch tearDown() to deal with both cases, or gorilla-patch the non-SQLite findTables() to return non-prefixed and/or FQ table names.

As I see it, it's a question of where you want the ugliness. Either tearDown() (and others) expect prefixed table names and SQLites findTables() bends over backwards to create the illusion of regularly prefixed tables, or we rewrite tearDown() and friends to expect FQ and/or flat non-prefixed tables, and then the non-SQLite findTables() needs to strip prefixes/fully qualify the table names.

Damien Tournoud’s picture

This was broken by #1353516: Tests for: Tables are not deleted when a module is disabled during test execution, which got back-ported to Drupal 7 as part of #1541958: Split setUp() into specific sub-methods. The previous implementation was querying the schema directly (not using $connection->findTables()) and as a consequence did not have to deal with all of that.

I see three, not necessarily incompatible, ways of improving the situation:

  1. We implement (on all databases) a findTables() that work on the virtual, un-prefixed, name of the table; that means being able to invert the prefixing, and might not be possible for every possible like expressions; (it would be acceptable to restrict that to prefix search)
  2. We fix SimpleTest so that it uses $connection->getTablePrefix() instead of reading from the configuration directly;
  3. We remove the magic use of attached databases from SQLite. This was done to improve concurrency during testing (and works really really well), but as nobody is actually testing under SQLite right now, nobody is actually going to miss it.
Xen’s picture

1. You lost me again. You mean have findTables take unprefixed tables as an argument , or that it returns un-prefixed table names ?

2. is not an option, there is no getTablePrefix(), as there might be multiple prefixes (per table prefixes). The latter would probably have a more rubust implementation.

3. Would be a shame.

Damien Tournoud’s picture

1. You lost me again. You mean have findTables take unprefixed tables as an argument , or that it returns un-prefixed table names ?

Obviously, both.

2. is not an option, there is no getTablePrefix(), as there might be multiple prefixes (per table prefixes). The latter would probably have a more rubust implementation.

I meant $connection->tablePrefix(), which does the right thing when not passed an argument: return the default prefix.

3. Would be a shame.

Not sure who would miss it. It has always been kind-of-a-hack, as you pointed out yourself multiple times already :)

Xen’s picture

In that case, it seems to me that we only need one. If we assume for a moment that we've done what's needed:

Simpletests teardown() does a db_find_tables('%') which digs out the default connection, which is still the connection that simpletest created, thus prefixed in the connection itself.

DatabaseSchema_sqlite::findTables() would then work in its current incarnation, as it'll call DatabaseSchema::getPrfixInfo() which will get the right schema name from the connection, and it'll all just work...

DatabaseSchema::findTables() is a tad more tricky. We could take the easy way out, and just prefix the table expression with the connection default prefix and leave it at that. Means that table specific gets fuxxord, but I recon they already are somewhat. We could try to reverse the prefix logic, but in theory there's always the chance of a prefix configuration that makes whether a given db table is one with a prefix or not, ambiguous. A contrived example would be a default empty prefix and a 'sys' prefix for a table named 'tem'.

Anything I've missed?

Not sure who would miss it. It has always been kind-of-a-hack, as you pointed out yourself multiple times already :)

Well, yeah, but the problems arise from assumptions made in other places. Which seems to be primarily located in simpletests tearDown. If we can kill off that assumption, it's a somewhat cute hack, which works nicely.

Xen’s picture

Status: Needs work » Needs review
FileSize
4.46 KB

OK, lets try that then. This patch tries to make db_find_tables() prefix-less. Seems to work all-right for MySQL, but let's see what the testbot makes of it.

I get weird errors when trying to install on SQLite using the current HEAD (63057b78be16de202518d2e0ace4266a56df2d05). Can anyone see if it's just me?

dcrocks’s picture

D8 has failed to instal on SQlite for more than a month now. See #1998366: [meta] SQLite is broken.

Xen’s picture

@dcrocks
Ah, thanks for the pointer.

dcrocks’s picture

Even though the patch #93 in #1998366: [meta] SQLite is broken fails test, it will get you through an install and simple content additions.

Xen’s picture

Version: 8.0.x-dev » 7.x-dev
Component: sqlite database » ajax system
Issue summary: View changes

Seems that it's fixed in D8 now. There's not much chance of backporting here, but it would be nice to be able to use SQLite for testing in D7.

David_Rothstein’s picture

Component: ajax system » sqlite db driver
David_Rothstein’s picture

Not clear where this was fixed in D8 exactly, but it looks like the sqlite findTables() method in Drupal 8 does include some code to not set the prefix... so maybe that's worth verifying and then using as a guide for how to backport to D7?

amateescu’s picture

Title: [sqlite:simpletest] Failed to drop all prefixed tables. » The SQLite database driver fails to drop simpletest tables
Version: 7.x-dev » 8.0.x-dev
Priority: Normal » Major
Issue summary: View changes
Issue tags: -sqlite +D8 Accelerate London
Parent issue: » #2508567: DrupalCI SQLite random failures
FileSize
11.27 KB

@David_Rothstein is right, this is not fixed in D8 and it's actually one of the source problems from #2508567: DrupalCI SQLite random failures.

Before @znerol pointed me to this issue I had a slightly different approach than #33 (fixed SQLite's findTables() method as well as TestBase) but I think @Xen's patch is a bit cleaner, so here's a patch which also contains everything needed so the sqlite db files are actually deleted.

bzrudi71’s picture

Status: Needs review » Reviewed & tested by the community

This does fix all the random SQLIte testbot exceptions as verified by many runs. I did a patch review and all looks good to me. RTBC :) Great work @amateescu!

bzrudi71 queued 40: 1713332-40.patch for re-testing.

bzrudi71’s picture

Status: Reviewed & tested by the community » Needs review

well, let's test on the new bots to confirm :-)

EDIT: Where is the SQLite test button, seems we need to upload the patch again?

bzrudi71’s picture

bzrudi71’s picture

Status: Needs review » Reviewed & tested by the community

Nice, back to RTBC :)

David_Rothstein’s picture

Issue tags: +Needs backport to D7

Sounds like we do at least want to try backporting something here, if it can be done.

Damien Tournoud’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

Unfortunately, this is fairly incomplete. If we want findTable() to take a logical (unprefixed) table name (which I would support), it needs to take all the prefixes into account, not just the default one.

Needs tests, and needs some thinking about what the API should look like. I think the easiest course forward would probably be to just load all the tables and do the matching in PHP.

xjm’s picture

Priority: Major » Critical
Related issues: +#2485119: [meta] The Drupal 8.0.0-rc1 Release Checklist

Promoting to critical as a blocker for #2485119: [meta] The Drupal 8.0.0-rc1 Release Checklist that is independently actionable.

xjm’s picture

Priority: Critical » Major

We discussed this today with @alexpott, @catch, @effulgentsia, @webchick, and myself, and agreed to keep this issue at major because:

  • This issue does not actually prevent SQLite from functioning, just the test suite from running reliably
  • It may be possible to work around this core bug in DrupalCI by using only single concurrency or otherwise forcing the leftover tables to be dropped in the testrunner script
  • We will still confirm the test suite passes for #2485119: [meta] The Drupal 8.0.0-rc1 Release Checklist, and we agreed to file any additional criticals as needed at that time if this bug obscures other regressions in the meanwhile.

That said, we agreed that it's still important to resolve this as well as any other parts of #2508567: DrupalCI SQLite random failures to ensure that both DrupalCI and our SQLite driver are robust.

webchick’s picture

Issue tags: +blocked by drupalci

Tagging.

Mixologic’s picture

Is this actually blocked by drupalCI? The patch in #44 seems to be passing the sqlite Tests on the drupalCI infrastructure (https://www.drupal.org/pift-ci-job/2172).

Or is it that sqlite in general is still blocked because of this issue, and drupalci could work around it..? Hopefully we don't create a situation where the tests *only* pass on drupalci.

amateescu’s picture

Assigned: Unassigned » amateescu
Issue tags: -blocked by drupalci

Nope, this is not blocked by DrupalCI, I just need to implement Damien's feedback :)

amateescu’s picture

Assigned: amateescu » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
15.5 KB
6.8 KB

Implemented a fix for #47 by keeping a list of prefixed table names in the db connection object, which the new implementation of Schema::findTables() can use in order to take care of all prefixes.

amateescu’s picture

Of course, the SQLite implementation has to be updated as well. test coverage++

webchick’s picture

Look at that sea of green! Brings a tear to my eye. :D

Crell’s picture

A few smaller points, nothing major. I'd prefer to have an SQLite maintainer RTBC this once the following have been addressed.

  1. +++ b/core/lib/Drupal/Core/Database/Connection.php
    @@ -139,6 +139,13 @@
    +   * List of un-prefixed table names, keyed by fully qualified table names.
    +   *
    +   * @var array
    +   */
    +  protected $prefixedTablesMap = [];
    

    The comment and the variable name seem out of sync and confuse me. The variable name suggests it is prefixed table names, but the comment says no.

    The comment says it's keyed by the table names, but not what the values are. What are they? The comment should explain.

  2. +++ b/core/lib/Drupal/Core/Database/Schema.php
    @@ -173,25 +178,55 @@ public function tableExists($table) {
    +    foreach ($this->connection->query("SELECT table_name FROM information_schema.tables WHERE " . (string) $condition, $condition->arguments()) as $table) {
    

    Yikes. Can we break this query out to its own line (or two) instead of inlining it here?

amateescu’s picture

Fixed those two points.

The only problem is.. there's quite a shortage of SQLite maintainers, and the main part of this patch is actually about fixing the API of the findTables() schema method.

Speaking of which, I also drafted a change record for this change: https://www.drupal.org/node/2552435

Damien Tournoud’s picture

Status: Needs review » Needs work

Taking a quick look at #57:

1) The SQLite implementation is basically not done, it doesn't use the same strategy (fetch all the tables, do the prefixing in PHP) as MySQL, and as a consequence will just not work

2) The base implementation needs to take into account for tables that do not follow the prefixing scheme:

+      // Take into account tables that have an individual prefix.
+      if (isset($individually_prefixed_tables[$table->table_name])) {
+        $prefix_length = strlen($this->connection->tablePrefix($individually_prefixed_tables[$table->table_name]));
+      }
+      else {
+        $prefix_length = $default_prefix_length;
+      }

That second branch needs to check that the table name starts with the prefix.

Overall, this means that the test coverage has quite some gaps.

amateescu’s picture

Status: Needs work » Needs review
FileSize
16.76 KB
1.36 KB

Thanks for reviewing!

#58.1) The SQLite implementation doesn't need to use the same strategy as the base one because individually prefixed tables live in their own schema (database) (i.e. neither the main db or any attached one will contain a prefixed table name), and the patch takes that into account by looping over all the attached databases.

#58.2) Good point, we need exclude table names that don't start with the default prefix because they are not managed by Drupal. Fixed.

Damien Tournoud’s picture

#58.1) The SQLite implementation doesn't need to use the same strategy as the base one because individually prefixed tables live in their own schema (database) (i.e. neither the main db or any attached one will contain a prefixed table name), and the patch takes that into account by looping over all the attached databases.

That's probably OK for SQLite, since it's unlikely that there will be any foreign tables in there, but please add a comment explaining the reasoning.

amateescu’s picture

Damien Tournoud’s picture

Status: Needs review » Reviewed & tested by the community

Thanks, looks good to me.

The semantics of the prefixes in SQLite are *radically* different than on other database engines, which is not nice. That's my fault, this was purely a hack to make parallel testing on SQLite very fast.

webchick’s picture

Pre-emptively saving Damien Tournoud on the credit thingy. :)

The last submitted patch, 57: 1713332-57.patch, failed testing.

The last submitted patch, 59: 1713332-59.patch, failed testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 61: 1713332-61.patch, failed testing.

amateescu queued 61: 1713332-61.patch for re-testing.

amateescu’s picture

Status: Needs work » Reviewed & tested by the community

Finally, testbot came back for the latest patch and we're good to go!

alexpott’s picture

Version: 8.0.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

This issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 9ee2b53 and pushed to 8.0.x. Thanks!

  • alexpott committed 9ee2b53 on 8.0.x
    Issue #1713332 by amateescu, Xen, droplet, bzrudi71, znerol, czigor, pp...
amateescu’s picture

Thanks, @alexpott! Published the change record.

  • alexpott committed 9ee2b53 on 8.1.x
    Issue #1713332 by amateescu, Xen, droplet, bzrudi71, znerol, czigor, pp...
pietmarcus’s picture

Assigned: Unassigned » pietmarcus

I ran into this bug on my local Drupal 7 machine yesterday. I'll try to backport the fix to D7.

pietmarcus’s picture

Status: Patch (to be ported) » Needs review
FileSize
12.38 KB

Here's my attempt at backporting the fix to drupal 7. There are a few changes between the Drupal 8 code and this patch:

In DatabaseConnection_sqlite::_destruct I had to keep the following line in the code, to succesfully remove the database from disk:

  // Detach the database.
  $this->query('DETACH DATABASE :schema', array(':schema' => $prefix));

I couldn't find a good location for the The KernelTestBaseTest::run method. Also, the drupal_web_test_case::tear_down checks if the tables are dropped, so it sounded like a unnecessary method (please correct me if I'm wrong).

Please review my changes.

Status: Needs review » Needs work

The last submitted patch, 74: 1713332-74.patch, failed testing.

pietmarcus’s picture

David_Rothstein’s picture

Giving this issue a tag for greater visibility - seems like this is the main issue that blocks being able to run Drupal 7 contrib tests on SQLite (since it causes literally almost every single test case to have a failure).

  • alexpott committed 9ee2b53 on 8.3.x
    Issue #1713332 by amateescu, Xen, droplet, bzrudi71, znerol, czigor, pp...

  • alexpott committed 9ee2b53 on 8.3.x
    Issue #1713332 by amateescu, Xen, droplet, bzrudi71, znerol, czigor, pp...
Mixologic’s picture

DrupalCI was blocking d7 sqlite testing, now that is unblocked. This can be backported to D7 now.

  • alexpott committed 9ee2b53 on 8.4.x
    Issue #1713332 by amateescu, Xen, droplet, bzrudi71, znerol, czigor, pp...

  • alexpott committed 9ee2b53 on 8.4.x
    Issue #1713332 by amateescu, Xen, droplet, bzrudi71, znerol, czigor, pp...
marcelovani’s picture

marcelovani’s picture

I am using this patch to run tests on contrib modules and it works fine.

I can see that there are many errors when running tests on Drupal Core, most of the errors are to do with db lock.

01:03:46 exception 'PDOException' with message 'SQLSTATE[HY000]: General error: 5 database is locked' in /var/www/html/includes/database/prefetch.inc:168

Not sure if this is related to this ticket. As far as I understand, this patch is good.

andypost’s picture

D8 sqlite broken to return tables - follow-up #2949229: SQLite findTables Returns Empty Array on External DB.

Pancho’s picture

Status: Needs review » Needs work

D7 patch needs work.

opdavies’s picture

I've just found this issue whilst testing a D7 site with GitHub Actions and a SQLite database.

What work needs doing for the D7 patch? The one in #83 works for me, and my example tests passes and fails as expected.

TR’s picture

@opdavies: The D7 patch in #83 fails core Drupal testing, as you can see from the test results in #83. We need a new patch that fixes the errors and passes the core tests. You can see @andypost tried to do that in #85 (look at the hidden files in #83 to see what he tried) but he ran into problems caused by #2949229: SQLite findTables Returns Empty Array on External DB..

opdavies’s picture

Thanks TR. I'll take a look at this when I can, and see if I can move it forward.

mcdruid’s picture

I've just stumbled across this while working on #2978575: Mysql 8 Support on Drupal 7

It would be nice to reassure ourselves that we're not breaking other databases when making changes to e.g. add support for newer versions of MySQL.

So I'd also like to get D7 tests working again on SQLite; I wonder if we'd be better starting a new issue for that... but on the other hand there's lots of useful stuff in the previous comments here.

mcdruid’s picture

I ran the full D7 suite locally (PHP 7.2 and SQLite 3.11) with the patch from #83 and I got:

41342 passes, 441 fails, 78 exceptions

It actually looks to me like the original problem from this issue is solved by this patch; I didn't end up accumulating a ton of different simpletest12345-suffixed db files (although there were a handful leftover, perhaps from tests which didn't complete successfully?)

I think it may be worth carefully reviewing this patch - as it touches shared database code as well as the SQLite driver - and then opening a follow-up to look at the remaining failures under SQLite.

mcdruid’s picture

I think there was a small part of the backport missing, so here's a new patch with that.

Has maybe made a small difference in my local test results:

41347 passes, 439 fails, 78 exceptions

In general, I think the backport looks good - very close to what was committed to D8, and I didn't see anything that concerned me (perhaps unsurprisingly).

mcdruid’s picture

Status: Needs work » Needs review

Filed #3172878: Fix SQLite tests in Drupal 7 which very much depends on this backport.

TR’s picture

My contributed module SQLite tests have been failing for many many years because of this problem - so many years that DrupalCI no longer has the test logs that far back. The tests have always worked with MySql and PostgreSQL.

With the patch in #92, my SQLite tests now run green on DrupalCI.

Here is the drupalci.yml file that I used to run my contributed module tests on DrupalCI with patch #92. You can use this same drupalci.yml to test your own contributed module - just upload the patch to your issue queue and run your tests with SQLite on the patch.

Before #92: https://www.drupal.org/pift-ci-job/1624123
After #92: https://www.drupal.org/pift-ci-job/1833336

I don't know if the results from 1 contributed module is enough to mark this RTBC (if so, this is RTBC from me), but it does fix my SQLite tests for the first time ever and it does remove the "Failed to find test tables to drop" error.

Fabianx’s picture

Status: Needs review » Needs work

I don't want to bomb the party, but this was committed to 8.x while it was not yet released.

This is a pretty heavy behavior change - unless I miss something.

I think for Drupal 7 we have two choices:

1. Add a new API, e.g. findTablesD8 + db_find_tables_d8 to ensure that users of the old API are not affected
2. Opt in to the new behavior via a very low level DB configuration setting (and do so for new sites automatically when writing settings.php or test runners, etc.)

Personally I would opt for 1).

Thanks for all the work on this.

mcdruid’s picture

Good catch @Fabianx

Here's a patch which implements your suggested findTablesD8 + db_find_tables_d8 and leaves the existing functions for BC.

mcdruid’s picture

I think we also need a \DatabaseSchema_sqlite::findTablesD8() but it can just call the (revised) SQLite findTables().

I don't think we want to provide a BC copy of \DatabaseSchema_sqlite::findTables() because it doesn't actually work properly. Am I understanding that correctly?

mcdruid’s picture

All of the other patches from #3172878: Fix SQLite tests in Drupal 7 have just been committed, so this is the last change we need to have SQLite tests passing.

I think the test results from #96 and #97 confirm that we needed \DatabaseSchema_sqlite::findTablesD8() i.e. tests pass in SQLite with #97.

The only remaining question is whether we provide a BC copy of \DatabaseSchema_sqlite::findTables(), or alternatively fix it and have findTablesD8() call the fixed findTables() - which is how #97 works (or a variation thereupon - e.g. it could work the other way around).

My vote is for #97 - I don't see a need to provide a copy of the code that doesn't work properly for BC.

marcelovani’s picture

patch 97 is good

mcdruid’s picture

@Fabianx and I discussed this and decided to be abundantly cautious we should include the old sqlite findTables function for BC.

So here's a patch which does that.

The interdiff seems to suggest there's also been a change to includes/database/sqlite/database.inc but I think interdiff is actually getting confused as other patches have been committed to these files since #97 so line numbers have changed. The only actual difference in this patch is the BC findTables in includes/database/sqlite/schema.inc

So long as tests pass, I'll commit this shortly.

  • mcdruid committed 6d33276 on 7.x
    Issue #1713332 by amateescu, mcdruid, Xen, droplet, pietmarcus, bzrudi71...
mcdruid’s picture

Status: Needs review » Fixed

Thanks everyone that contributed to this!

mcdruid’s picture

(hopefully) updating credit that seemed to get lost between commit and marking this fixed.

Status: Fixed » Closed (fixed)

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

MustangGB’s picture