Improving our dump files

Yes I agree on that approach, I intend to take this on in the next couple of weeks, feel free to pick it up if you beat me to it.

i'm testing out an install profile today that we could maybe use for D6 and D7 migrate testing. we could try this as the D6 and D7 source databases so those sites can be committed and tested with. that'd also allow testers to easily check out a base install and also helps us to track the source we use for the eventual db dump. does this make sense? i'll post my progress so we can decide if it's of any value.

@benjy: i’d be happy to take on converting your D6 test database to an install profile, and/or building up a new D6 install profile, while double checking that it matches your local d6 db benchmark.

i think my sandbox install profile approach might give us a collaborative way to build up that source d6 dump, rather than a single db dump that we pass around. plus we get version control. and we could get a head start on the D7 dumps as a 7.x-1.x branch.

just to clarify after a conversation in irc w/ ultimike, i’m not suggesting we commit a new profile, especially a d6 one to d8! just that we use a profile to collaborate on the source for the dump itself.

only the full DB export (perhaps using dump-database-d6.sh) from the profile would committed to D8.

here's start to this sandbox D6 install profile that i'm suggesting: https://www.drupal.org/sandbox/bdone/2358293.

I was thinking it would be much quicker to just click a database together and then export the dump and commit that. Writing an install profile seems like a lot more effort.

But now you've mentioned it, committing the profile wouldn't be the worst idea, that would make it much easier to spool up a copy of the site if we needed to change the dumps. Eg, when you only have the DB, you need to know all the modules that need to exist in the codebase and that is more than just core for D6.

i've exported the complete simpletest D6 source tables from MigrateFullDrupalTestBase::testDrupal, so that we can discuss whether this data should be the benchmark we are striving to re-create in the mt profile i've been working with. if we were to get the entire source, regardless of using this profile or some other dump, is this the benchmark we were hoping to match whether in a profile or some other means?

How is this issue coming along? Have we done anymore work on the install profile?

Here's what we should do:

1. Write a script that runs over the dumps and builds a D6 database.
2. Write a script that dumps the D6 database into per table Drupal 8 Dump classes.

The workflow after that:

1. Run script 1.
2. Tweak from Drupal 6 UI.
3. Run script 2.

Only problem? After the first run we need to make all tests pass again... so this issue will be script 1, script 2, most dumps change, some tests change.

OK, I started on part 1, the script I used (createdb.txt) to generate the database from the dumps was simple, using the existing code from MigrateDrupal6Test. I attached it just in case we need it again, hopefully not.

I imported a working d6 database and then merged the generated database on top so we got cache, access and session tables etc. I had a few quirks to fix up such as the user table having a different structure based on some contrib modules, the file path been wrong disabling the system module etc etc. Eventually it worked and my D6 codebase was able to access the database.

I then updated the dump-database-d6.sh (2348875-db-dump-script.patch) to export the D6 database into the format required by the dumps files. Quite primitive, I rolled a basic template using str_replace. Also, i renamed it to .php, we should probably rename that back.

I've attached a patch (2348875-generated-tables.patch) with all the newly exported tables as well. That's the patch to start from when we begin converting the tests to work with the new tables.

OK, i added support for generating the table schema, fixed a few other issues with the generation and started work getting the MigrateActionConfigTest working. Still a few errors which mainly seem to be with the bugs in the dumps.

OK, wasn't a bug in the dump, just left over settings in my settings.php, MigrateActionConfigsTest should be easy to pass after the following:

* Improve the schema definition to fix unsupported types like longblob
* Schema definition needs to add primary and unique keys.
* Update the connection name used in the export script so as not to conflict with tests.

Let's see what the bot says.

OK, MigrateActionConfigTest is now passing. Updated a few more tests upto MigrateBookTest in the list, see how this goes.

Still need to add support for primary and unique keys in the definition generation.

See how this goes, still got MigrateDrupal6Test to convert and some mess in the user tests to unravel.

Fixed a bunch of tests, fixed up tests depending on the dumps and added the primary key support to the export script.

And a first attempt at fixing up MigrateDrupal6Test.

Fixed more tests, still some fails which are basically the result of corrupt dumps in that we were relying on the fact that some old dump files partially loaded tables.

I re-generated the dumps this time from a working D6 database after i'd been clicking around so a few extra tables. The cache tables have made the patch huge, i'll update the generation script to filter those out shortly.

Fixed another test that was causing me some issues, and updated the generation to exclude cache tables, regenerating the folder from scratch reduces the patch size a bit but we're still at 1mb.

Not sure what else the regeneration could have broke.

Removed watchdog, menu_router tables bloating the dumps.

Git fail, this should be right now.

This should fix most the fails, I had to manually update the Table\Users.php file. The script needs updating to not export user 0 and 1 which are both needed if you actually want to be able to browse the D6 site.

For the careful eye, the patch gets a lot smaller because i truncated the menu_links table and replaced them with menu links from our tests data.

More fixes plus updates the export script to exclude users 0 and 1.

More fixes. The current dumps are corrupt, they don't accurately represent a state of a D6 website. For example in this instance, we had fields in the field and field instance tables that did not have a column in the content_type_* tables. I've been rebuilding the fields using the D6 admin UI to ensure they're correct.

Still have a few failures but uploading for the bot, see what else I broke by re-adding the fields.

Also, normalising the dumps allowed an existing test to pick up the bug that is already fixed here: #2394571: Filter formats on cck text fields are not looked up in the idMap

OK, field and revision stuff broken as expected, couple fixes here, will carry on with the test tomorrow.

More fixes, more dump regeneration and i've also included the bug fix from #2404955: Field Formatter settings have incorrect mappings for number formats

Also the dumps caught another bug which i've fixed with this, current tests cover it.

- path_plain: file_url_plan
+ path_plain: file_url_plain

I've updated a few tests but only weights since they're fiddly to get right to match the tests and in some cases impossible.

More fixes and more exports. This should hopefully be much closer. Probably won't fix Drupal6MigrateTest though.

Found another issue #2405017: Cannot migrate boolean fields of type text however I had the dumps wrong which trigged the error so it's not blocking this issue as i'm making the new dumps resemble the old ones as best I can.

OK, more fixes and another bug: #2405023: Cannot create base_field_override entity, ID already exists

I removed this test, excluding a view mode doesn't makes sense. The only way this test worked before was because of a subset of data during the test. The "exclude" field is per field on the display, not the display itself.

-    // Make sure we don't have the excluded print entity display.
-    $display = entity_load('entity_view_display', 'node.story.print');
-    $this->assertNull($display, "Print entity display not found.");

Patch much bigger because i'm also removing the "Dump" folder. Also attaching a smaller patch of changed files to make it easier for review.

Think we're now postponed on #2404955: Field Formatter settings have incorrect mappings for number formats

Few things still referencing the deleted dumps.

Missed a few references.

Updated the export script to format the dumps to Drupal's coding standards, also fixed a few comments.

Note that the patch doesn't contain any material changes (check git diff 8.0.x --name-only | grep -v 'Tests\|Dumps\|Table' | xargs git diff 8.0.x) after #2404955: Field Formatter settings have incorrect mappings for number formats is in so this is good to go once that's in. Yes, it changes the tests and dumps quite some but who cares except those who need to maintain those?

Rerolled and included a review only patch not containing the test and dump changes.

Sigh. The drop is always moving. The review patch didn't change.

Had to move a few things to make the temp files work including a file_put_contents('/tmp/some-temp-file.jpg', ''); in MigrateDrupal6Test.

Dump got bigger because of a few temp tables on the D6 site such as cache_menu changing. I wonder if we should package a dump scrip that truncates those tables to try and reduce the amount of change in the DB?

I found two bugs, one introduced by #2394571: Filter formats on cck text fields are not looked up in the idMap but not caught by the tests because of a bug in the dumps where db_storage was incorrectly set. The other bug introduced by #2394157: Update the EntityFile destination to handle temporary files again missed by the dumps because we didn't have a migration where the temporary directory was the same on the source and destination sites.

Interdiff shows the fix for the first bug, we could have a separate issue but we already have test coverage here?

I got around the second bug with an update to make it look exactly like the old dumps, created #2405337: EntityFile destination doesn't handle temporary files when the source and destination are the same to handle the short comings in the EntityFile destination.

Took a few hours to track these down, going to tag this issue quick fix because it will conflict with many other migrate issues and it would be better if the bugs were fixed in the individual issues.

I doubt a 1MB patch can be called quickfix -- I think this is the right tag and also we can postpone any other migrate RTBC issue; I doubt there are many (thanks for all the commits).

I fixed a single letter typo in LoadEntity and so I am reuploading the whole package: patch, interdiff, review patch.

Final patch, adds the dumping of the database to the table export script as an improvement discussed in IRC. Still RTBC.

+    $this->database->insert("content_group_fields")->fields(array(
+      'type_name',
+      'group_name',
+      'field_name',
+    ))
+
+    ->execute();

The blank line here contains spaces - can the generator be fixed - or is this a feature?

Back to needs review for #69

alexpott, It does that because i'm using 4 spaces as the prefix to the export function to make the indentation of the code work within the class. There is no easy way to fix that.

As discussed in IRC, changes made:

• Removed the dependency on drush
• Renamed the script to migrate-dump-d6.sh
• Trim all white space from the generated class files.
• Improved docs

Let's try this one more time.

Sorry I should have spotted this first time around... one (very) minor thing - the template should have a new line at the end.

Fixed.

Re-roll, patch conflicted with something else.

Back to RTBC. Only added a newline.

Migrate is not frozen during beta. Committed ec9a3ec and pushed to 8.0.x. Thanks!

-    $this->assertEqual($field_storage->status(), FALSE, "Status is FALSE");
+    $this->assertEqual($field_storage->status(), TRUE);

In core/modules/migrate_drupal/src/Tests/d6/MigrateFieldTest.php is going to result in a useless test assertion telling people that true = true - a test assertion message here is a nice thing to do. Can we get a followup to fix this.

Done: #2406377: Remove unnecessary $field_storage->status assertions from MigrateFieldTest