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.
Comment | File | Size | Author |
---|---|---|---|
#100 | 1713332-100.patch | 15.16 KB | mcdruid |
#100 | interdiff-1713332-97-100.txt | 3.03 KB | mcdruid |
#97 | 1713332-97.patch | 15.47 KB | mcdruid |
#97 | interdiff-1713332-96-97.txt | 340 bytes | mcdruid |
Issue fork drupal-1713332
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:
Comments
Comment #1
wulff CreditAttribution: wulff commentedI'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.
Comment #2
droplet CreditAttribution: droplet commented@wulff,
can you review D8 ? so we can move forward and backported to D7 :)
Thanks.
Comment #3
droplet CreditAttribution: droplet commentedComment #5
droplet CreditAttribution: droplet commentedFailed patch is D7. Please review #0 sqlite_drop_db.patch
Comment #6
pp CreditAttribution: pp commentedThis delete the live database. Add prefix.
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.
Comment #7
zserno CreditAttribution: zserno commentedI ran into the same issue today with latest D8 HEAD. Patch in #6 looks good and works great on my local.
Comment #8
gaas CreditAttribution: gaas commentedI 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).
Comment #9
catchIs there absolutely no way to fix this without special-casing sqlite in simpletest?
Comment #10
Damien Tournoud CreditAttribution: Damien Tournoud commentedWe are supposed to have all the code in place for this to work.
See:
If that's not working, someone needs to investigate why.
Comment #11
znerol CreditAttribution: znerol commentedThe problem is the sqlite implementation of
findTable
:Here we call
getPrefixInfo
with a table-expression. However this method is declared as: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 theschema
andtable
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 likesimpletest388380%
. 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).
Comment #12
znerol CreditAttribution: znerol commentedComment #13
czigor CreditAttribution: czigor commentedI met the same bug on D7. The patch in #11 worked for me, so enjoy the same for D7.
Comment #15
czigor CreditAttribution: czigor commentedDear bot, don't test my patch, it's for D7. It's only for later reference.
Comment #16
apassi CreditAttribution: apassi commentedCant 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.
Comment #17
czigor CreditAttribution: czigor commentedHi 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)
Comment #18
znerol CreditAttribution: znerol commented@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.Comment #19
apassi CreditAttribution: apassi commentedOk, 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.
Comment #20
Damien Tournoud CreditAttribution: Damien Tournoud commentedOk, 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 code of the top half is actually correct and should not be changed.
Comment #21
Xen CreditAttribution: Xen at Reload commented@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?
Comment #22
Damien Tournoud CreditAttribution: Damien Tournoud commented@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:
Comment #23
Xen CreditAttribution: Xen commented@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.
Comment #24
Damien Tournoud CreditAttribution: Damien Tournoud commentedHere is how it is supposed to work:
db_find_tables($connection_info['default']['prefix']['default'] . '%');
, which translates todb_find_tables("sXXXXXXXX.%")
"sXXXXXXXX.%"
into schema: "sXXXXXXXX" and table: "%"db_drop_table()
, which re-prefix the table and deletes itThis 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).
Comment #25
Xen CreditAttribution: Xen commentedIt doesn't. Simpletest sets the prefix to "simpletest123", so it translates to db_find_tables("simpletest123%").
It doesn't as there's no period.
tearDown() doesn't, due to the missing period. db_drop_table() does, when it gets the right tablename.
Comment #26
Xen CreditAttribution: Xen commented@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.
Comment #27
Damien Tournoud CreditAttribution: Damien Tournoud commentedIndeed, 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.
Comment #28
Xen CreditAttribution: Xen commentedSimpletest 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..
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.
Comment #29
Damien Tournoud CreditAttribution: Damien Tournoud commentedThis 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:
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)$connection->getTablePrefix()
instead of reading from the configuration directly;Comment #30
Xen CreditAttribution: Xen commented1. 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.
Comment #31
Damien Tournoud CreditAttribution: Damien Tournoud commentedObviously, both.
I meant
$connection->tablePrefix()
, which does the right thing when not passed an argument: return the default prefix.Not sure who would miss it. It has always been kind-of-a-hack, as you pointed out yourself multiple times already :)
Comment #32
Xen CreditAttribution: Xen commentedIn 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?
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.
Comment #33
Xen CreditAttribution: Xen commentedOK, 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?
Comment #34
dcrocks CreditAttribution: dcrocks commentedD8 has failed to instal on SQlite for more than a month now. See #1998366: [meta] SQLite is broken.
Comment #35
Xen CreditAttribution: Xen commented@dcrocks
Ah, thanks for the pointer.
Comment #36
dcrocks CreditAttribution: dcrocks commentedEven though the patch #93 in #1998366: [meta] SQLite is broken fails test, it will get you through an install and simple content additions.
Comment #37
Xen CreditAttribution: Xen commentedSeems 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.
Comment #38
David_Rothstein CreditAttribution: David_Rothstein commentedComment #39
David_Rothstein CreditAttribution: David_Rothstein commentedNot 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?
Comment #40
amateescu CreditAttribution: amateescu for Pfizer, Inc. commented@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.
Comment #41
bzrudi71 CreditAttribution: bzrudi71 commentedThis 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!
Comment #43
bzrudi71 CreditAttribution: bzrudi71 commentedwell, let's test on the new bots to confirm :-)
EDIT: Where is the SQLite test button, seems we need to upload the patch again?
Comment #44
bzrudi71 CreditAttribution: bzrudi71 commentedSame patch to start SQLite testing...
Comment #45
bzrudi71 CreditAttribution: bzrudi71 commentedNice, back to RTBC :)
Comment #46
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedSounds like we do at least want to try backporting something here, if it can be done.
Comment #47
Damien Tournoud CreditAttribution: Damien Tournoud commentedUnfortunately, 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.
Comment #48
xjmPromoting to critical as a blocker for #2485119: [meta] The Drupal 8.0.0-rc1 Release Checklist that is independently actionable.
Comment #49
xjmWe discussed this today with @alexpott, @catch, @effulgentsia, @webchick, and myself, and agreed to keep this issue at major because:
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.
Comment #50
webchickTagging.
Comment #51
MixologicIs 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.
Comment #52
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedNope, this is not blocked by DrupalCI, I just need to implement Damien's feedback :)
Comment #53
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedImplemented 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.
Comment #54
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedOf course, the SQLite implementation has to be updated as well. test coverage++
Comment #55
webchickLook at that sea of green! Brings a tear to my eye. :D
Comment #56
Crell CreditAttribution: Crell at Palantir.net commentedA few smaller points, nothing major. I'd prefer to have an SQLite maintainer RTBC this once the following have been addressed.
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.
Yikes. Can we break this query out to its own line (or two) instead of inlining it here?
Comment #57
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedFixed 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
Comment #58
Damien Tournoud CreditAttribution: Damien Tournoud commentedTaking 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:
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.
Comment #59
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedThanks 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.
Comment #60
Damien Tournoud CreditAttribution: Damien Tournoud commentedThat'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.
Comment #61
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedSure thing :)
Comment #62
Damien Tournoud CreditAttribution: Damien Tournoud commentedThanks, 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.
Comment #63
webchickPre-emptively saving Damien Tournoud on the credit thingy. :)
Comment #68
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedFinally, testbot came back for the latest patch and we're good to go!
Comment #69
alexpottThis 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!
Comment #71
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedThanks, @alexpott! Published the change record.
Comment #73
pietmarcus CreditAttribution: pietmarcus as a volunteer commentedI ran into this bug on my local Drupal 7 machine yesterday. I'll try to backport the fix to D7.
Comment #74
pietmarcus CreditAttribution: pietmarcus as a volunteer commentedHere'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:
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.
Comment #76
pietmarcus CreditAttribution: pietmarcus as a volunteer commentedFixed the system.test
Comment #77
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedGiving 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).
Comment #80
MixologicDrupalCI was blocking d7 sqlite testing, now that is unblocked. This can be backported to D7 now.
Comment #83
marcelovaniEncountered same issue. Re-rolled the patch.
Comment #84
marcelovaniI 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.
Not sure if this is related to this ticket. As far as I understand, this patch is good.
Comment #85
andypostD8 sqlite broken to return tables - follow-up #2949229: SQLite findTables Returns Empty Array on External DB.
Comment #86
PanchoD7 patch needs work.
Comment #87
opdaviesI'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.
Comment #88
TR CreditAttribution: TR commented@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..
Comment #89
opdaviesThanks TR. I'll take a look at this when I can, and see if I can move it forward.
Comment #90
mcdruidI'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.
Comment #91
mcdruidI 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.
Comment #92
mcdruidI 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).
Comment #93
mcdruidFiled #3172878: Fix SQLite tests in Drupal 7 which very much depends on this backport.
Comment #94
TR CreditAttribution: TR commentedMy 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.
Comment #95
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedI 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.
Comment #96
mcdruidGood catch @Fabianx
Here's a patch which implements your suggested findTablesD8 + db_find_tables_d8 and leaves the existing functions for BC.
Comment #97
mcdruidI think we also need a
\DatabaseSchema_sqlite::findTablesD8()
but it can just call the (revised) SQLitefindTables()
.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?Comment #98
mcdruidAll 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 havefindTablesD8()
call the fixedfindTables()
- 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.
Comment #99
marcelovanipatch 97 is good
Comment #100
mcdruid@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.
Comment #102
mcdruidThanks everyone that contributed to this!
Comment #103
mcdruid(hopefully) updating credit that seemed to get lost between commit and marking this fixed.
Comment #105
MustangGB CreditAttribution: MustangGB commented