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
Most tests in the migrate_drupal test group are failing on SQLite.
Proposed resolution
1. Fix the 'content_field_multivalue' table to not use a serial type (autoincrement) for the 'vid' field. This table is simulating a field data table, and those do not have an autoincrement on the vid column either.
2. The database name needs some special handling because it currently includes the relative path to the sqlite file.
Remaining tasks
Figure out if the database name needs special escape handling.
User interface changes
Nope.
API changes
Nope.
Comment | File | Size | Author |
---|---|---|---|
#11 | interdiff.txt | 853 bytes | amateescu |
#11 | 2454669-11.patch | 142.55 KB | amateescu |
#8 | interdiff.txt | 2.03 KB | amateescu |
#8 | 2454669-8.patch | 103.92 KB | amateescu |
#7 | migrate_drupal-sqlite-tests-status.txt | 43.66 KB | amateescu |
Comments
Comment #1
amateescu CreditAttribution: amateescu commentedHere's a full fail log for the migrate_drupal test group and the first part of the fix. For now I'm just curious if this causes any problems on MySQL.. it shouldn't :)
Comment #2
amateescu CreditAttribution: amateescu commentedOk, that small change doesn't break anything on MySQL, setting to NW because we still need to figure out part 2 from the issue summary.
Comment #3
benjy CreditAttribution: benjy at CodeDrop commentedYou can't make changes to these files manually, they're auto-generated dumps.
Comment #4
amateescu CreditAttribution: amateescu commented@benjy, then what's the correct procedure to update the dumps, the one explained here https://www.drupal.org/sandbox/benjy/2405029? If so, "Login to the D6 site, make changes as required." this step means that I actually need to have a D6 installation?
Comment #5
amateescu CreditAttribution: amateescu commentedIn the meantime, I made some progress here. With the patch attached, half of the migrate_drupal tests are passing but the other half fails with something like this:
I tried a rerolled version of #1120020-40: SQLite database locking errors cause fatal errors but no luck so far.. still digging :/
Comment #6
benjy CreditAttribution: benjy at CodeDrop commentedYes exactly. The code for that database is in the sandbox.
The most up-to-date instructions I believe are in /core/scripts/migrate-dump-d6.sh
Comment #7
amateescu CreditAttribution: amateescu commentedI spent the last couple of days trying to fix the database locking issue mentioned in #5 but that is not really a migrate_drupal problem and we already have an issue for it: #1120020: SQLite database locking errors cause fatal errors
Updated ContentFieldMultivalue properly via the d6.gz database dump and I'm attaching the current status for the migrate_drupal tests. Without this patch, all of them were failing.
Comment #8
amateescu CreditAttribution: amateescu commentedTogether with #2465633: Bring back the custom Statement class for the SQLite driver and another small fix here, we have all the individual migrate_drupal tests passing! The one that runs all of them in one go, Drupal\migrate_drupal\Tests\d6\MigrateDrupal6Test, is still failing :/ Probably because it skips the setUp()/tearDown() methods of individual test classes and it runs a couple of tests on the wrong database file.
I think we should open a separate issue for that one test class and just go ahead here with a patch that fixes everything else :)
Comment #9
dawehnerJust in general, I was thinking whether we should add unit tests for those methods, even if we would have to skip the constructor.
That totally makes sense, and was probably just a c&p error.
That change looks bogus ... just kidding
Comment #10
benjy CreditAttribution: benjy at CodeDrop commentedThe creating of the binary patch looks wrong, also, very small. It's usually 4-500Kbs? Some instructions on creating the patch for the gz file here: https://www.drupal.org/node/2452709#comment-9743751
getFullQualifiedTableName
I wish i'd spotted this when the method was added, getFullyQualifiedTableName() would have been much better. Maybe we can still rename this in a follow-up.
Comment #11
amateescu CreditAttribution: amateescu commentedThen we would have to write three unit tests for each db driver. And
Connection::$prefixes
is protected and only set in the constructor, so I don't even know where I would start for a unit test. Let's just add an integration test that checks if this method works in all db drivers.I re-did that d6.gz dance and this time I think it should be good, at least it's a proper binary patch :)
Running the new test from the interdiff without the patch:
With the patch applied:
Comment #12
dawehnerLooks fine now!
Comment #14
amateescu CreditAttribution: amateescu commentedThe test failure above is for an older patch, the last one from #11 is fine.
Comment #15
alexpottJust wondering how we got this wrong?
Comment #16
catchI'd guess that's a copy and paste error from a vid column on the node revision table.
Comment #17
benjy CreditAttribution: benjy at CodeDrop commentedThe dumps used to be managed by hand so a copy and paste error seems likely to me.
Comment #18
catchCommitted/pushed to 8.0.x, thanks!