Document ways to remove migration tables (ID map etc.)

Migrate is still experimental, so moving to major and tagging 'migrate critical' instead.

The ID map table will be removed in MigrateIdMapInterface::destroy() - however I don't see that called anywhere outside tests so think you're right that at the moment there's no straightforward way to remove the tables.

I don't see why this is a bug report, having those tables actually still around is interesting for figuring out what was migrated, what status they have and messages (if any). This allows you to even write custom post migration scripts to fix or do additional things that aren't in a plugin at the moment or aren't straightfoward to handle.

So to me this is a task, not a bug.

- edit - oh wait, on uninstall .. that's different, I need to read everything :)
I'd say this is a normal bug report though :)

Agreed–I do not see that having extra tables in a database qualifies as Major.

Which means not migrate critical either. If there was a problem re-installing it'd be different, but because the tables are created when needed that shouldn't be an issue.

Personally, when I uninstall a module and read

The following modules will be completely uninstalled from your site, and all data from these modules will be lost!

I kind of expect the migration map tables to be part of "all data". Drupal\migrate\Plugin\migrate\id_map\Sql::destroy() implements exactly this functionality (and is currently the only id_map core knows).

I see two main options we could implement:

1. Implement hook_uninstall and run destroy() on all migrations' id_map plugins, as long as the migrations requirements are met. This ensures that only tables stemming from our migrations are affected.
2. Provide some other means to run destroy(), e.g. via a button on a (not yet existing) migrate configuration page or a drush command in Contrib

Just to confirm:
In the meantime, Is it safe for me to manually drop these tables?

Confirming that problem still available for 8.2.4
Decision with manual removing tables works for me

+1 for #4. Those tables should not be removed. The behavior it's not a bug, it's intentional. The same behavior was in the Migrate module.

I feel that if you need to keep the references from the maps you can just keep the migrate module installed.

Let's think about next use case:
I enabled modules for migration (incl. "Migrate_drupal_ui").
I migrated content by using UI.
Now have drupal8 and want to live with it.

What is the point of having an interface for migration and not be able to remove everything (by using UI/uninstalling modules) that I do not need after the migration has been completed?

Here's another use case for removing them:

I have migrations running on cron on Drupal 8.0.x. I upgraded to 8.2.x, and all the migrations are broken. I can fix the migration and custom source plugins easily (simple migrations, not migrate_drupal), but I'm still stuck because there is no upgrade path between migrate module versions, see for example https://www.drupal.org/node/2679797.

I'd like to just rollback all content, uninstall all migrate related modules, upgrade drupal, then re-install migrate and import the new content. That fails because there are still all the old map and message tables with invalid schema.

When I uninstall a module, I want to start from scratch. I believe that was the reasoning for removing the "disabled" status for modules.

So the solution is to drop any table with migrate_map_* or migrate_message_* ?

Dropping tables sound reasonable, at least it would offer a way to start from scratch without deleting tables from database manually.

Dropping the tables is fine and safe as long as you delete both map and message tables. To delete all tables of currently present migrations, you can run something like this before uninstalling the module:

  // Specify migration ids. An empty array means "all migrations".
  $migrations = [];
  $plugin_manager = \Drupal::service('plugin.manager.migration');
  $instances = $plugin_manager->createInstances($migrations);

  foreach ($instances as $id => $instance) {
    $instance->getIdMap()->destroy();
  }

Per conversation with Mike, the process described in #3 and #17 present challenges.

It would be safer to check and delete the tables in the state, rather than scan the tables prefixed with migrate. Why? it could happen that other contrib or custom module creates tables whose name would begin with migrate -- therefore, the uninstall process suggeted here would result in bad consequences (break other modules)

I found the problem with #3. When you patch 'migrate' module with this patch and try to uninstall module, you get

in_array() expects parameter 2 to be array, null given migrate.install:57

because you haven't prefixed_migrate_tables variable in DB.
I fixed the patch to check this situation. Minor styling added also.

Thanks everyone for working on this.

I am concerned that the tables will be removed prematurely. That is, before the site is thoroughly tested or without the user fully understanding the risk. Whatever solution is taken to remove the tables it really should include a warning (in big red scary letters) that those tables are needed to fix problems with the migration. They need to be left until they are 100% sure that the site has been thoroughly tested. And that once removed, it may not be possible to fix a problem.

yes that is what I am talking about......I installed just a core drupal 8 site...used migrate to migrade D6 site, it was just a standard site, nothing fancy.....few blog posts...few pages......nothng major, no extra field, custome stuff.....it has like 70 tables in drupal 6.....but after using coremigrate module.....the number of tables jumped to over 220.....which is a disaster..because blue host limits me with 1000 tables per hostig acccount.....so I want to save on tables.....

somebody needs to fix this...this module is just a disaster.....

@sopranos, thanks for reporting your problem with the extra tables. As pointed in #11 and #22 the tables are left after the upgrade so that it is possible to fix any problem. It is up to the user to delete them, knowing that it will make fixing problems with the upgrade difficult. But once you are completely sure the new site is functioning as expected it is perfectly OK to drop the tables by whatever means available to you.

To help fix this, you could try the patch in #21 in a test environment and report the results here.

I've got the same issue as @rocketeerbkw in #14.

Uninstall module => reinstall module => migration failed because of invalid old table schema. For me this is a bug, and the tables should be removed on uninstall.

I can't follow the reasoning behind "leaving the tables so user can fix problems with migration"... when the user uninstalls the module, his intentions are obvious. IMHO the current handling is counter-intuitive.

Rerolled the patch and tidied it up. I don't like the solution but we need to get rid of the tables (we have over 1,100 on our site right now) so it'll do for the short term.

I'd be interested in trying out the destroy() method.

Oh no! Updated file paths :)

Aaaaaaand fixing a PHP problem that I forgot to fix duh

We discussed in the migrate maintainers meeting. One option very much liked was to delete tables and remove all the mapping tables when migrate API module is un-installed. If a module depends on the mapping tables, then they need to keep the migrate module enabled.

Ideally we'd like to provide an extra confirm explaining to users that when they uninstall the migrate module, the mapping tables will disappear. Are they sure they want to do this? Because if they do disable it, then they have no way to get that data back except from database backup, etc.

On one hand, it would make sense to delete the tables on uninstall. On the other hand, deleting the tables is a new feature and would be un-expected. And could break a lot of folks.

When a module is being uninstalled, there is a final confirmation that says like this:
--clips--
The following modules will be completely uninstalled from your site, and all data from these modules will be lost!

• Migrate

Would you like to continue with uninstalling the above?
--clips--

This text comes from here:
https://api.drupal.org/api/drupal/core%21modules%21system%21src%21Form%2...

We could and should alter this form and add a big fat warning that by uninstalling Migrate, the map tables will be lost forever.

Cheers,
Markus

Is it not fair to say that if a module relies on the migration map tables, they should add a dependency on the migrate module(s) (therefore the migrate module(s) can't be uninstalled without first uninstalling the module that depends on it)?

The message "all data will be lost" implies that the default behaviour of removing all tables this module has created will be removed. The fact it doesn't remove the data seems like an anti-pattern.

@Sophie.SK makes an excellent point. We could just make Migrate remove the tables on module uninstallation, and update our handbook documentation to explain what will happen when Migrate is uninstalled, so that it's at least documented and not a gotcha! for people who need the map tables to stick around.

I'm a little unsure of deleting on uninstall. However, if we do it, we should early in the life of 8.6 and see what feedback we get on it. And not do in 8.5. Too much chance of breaking folks.

Are the mapping and messaging tables considered part of Migrate's API? I don't think they are, but I would rather get a framework manager's opinion on that (or at least a consensus from my fellow maintainers). If they are part of our API, I don't think we can unilaterally delete them on uninstall until D9 (although we could easily provide a helper function to do the job), since that'd break backwards compatibility.

@phenaproxima, what makes you think the map and message tables would not be part of Migrate API? In my opinion they very much are.

To summarize how I see this is as follows:
1. We have to have a way for a site builder to get rid of the tables when she/he considers the migrations to be completed and these tables are not needed anymore.

2. Regardless of the approach we choose, we need to document it in the handbook. Adding the 'Needs Documentation' tag for that.

3. For the actual deletion, we have two options as I see this:

3A: We delete the tables upon Migrate module uninstall.

• If we choose to do this, we need to alter the uninstall form so that those users doing the uninstall with a browser will get the last warning that the map tables will be forever gone.
• The (only) downside of this approach in my eyes is that modules can be also uninstalled with drush / drupal console.
• At least 'drush pmu' will not show any final confirmation, it just does the uninstall.

3B: We provide a way how the user can nuke the tables using contributed Migrate Tools

• This could be executed either with Drush...
• ...or by using the browser UI Migrate Tools provided
• This approach is safer in that sense that wiping out those tables requires an explicit execution of that specific 'nuke em all' command so it can't be a surprise for anyone.

I'm leaning towards 3B at the moment.

Cheers,
Markus

If the tables are part of our API, I suggest adding an API method that can clean the tables up, which at least gives us a documentable way for developers to delete the map tables. I envision something like this:

\Drupal::service('plugin.manager.migration')->createInstance('my_migration')->getIdMap()->clean();

From there, we can file follow-up issues and patches to expose that function in install hooks, UIs, Drush commands, and so forth. But we should start there.

+1

I must be enough onboard with the suggestion in #41 that I want to start debating naming. So +1. We can build the API here and decide who uses it later. This really feels like one of those things that is just a PHP snippet that someone develops and those that care just run the snippet to remove all tables.

BTW, I prefer delete() over clean().

An initial attempt, without tests.

BTW, I prefer delete() over clean().

I disagree, because delete() is ambiguous. Are we deleting a specific mapping? Several mappings? The relevant tables or mapping storage? The word "delete" doesn't indicate anything specific. Since this new method is really a way to tell the ID map to clean up after itself, clean() makes that much more obvious. Self-documenting FTW!

1. +++ b/core/modules/migrate/src/Plugin/migrate/id_map/Sql.php
   @@ -158,7 +158,12 @@ class Sql extends PluginBase implements MigrateIdMapInterface, ContainerFactoryP
   +      'keep_tables' => TRUE,
   

   How would this get overriden in practice? I don't see how the config is injectable. Maybe an example in the doxygen? NW for this point.

2. +++ b/core/modules/migrate/src/Plugin/migrate/id_map/Sql.php
   @@ -999,4 +1004,12 @@ function (array $id) {
   +    if (empty($this->configuration['keep_tables'])) {
   

   If we call this clean, shouldn't the key also be called clean?

   I'm still not sure I like clean. Could we call it dropTables() and drop_tables => FALSE?

How would this get overridden in practice? I don't see how the config is injectable.

Once migration plugins implement ConfigurablePluginInterface, it'll be easier to inject the configuration. In the meantime, I think we can pass ID map configuration in the migration definition, much the same way that we can configure the source and destination.

If we call this clean, shouldn't the key also be called clean?

No. "Clean" is a generic concept that could apply to any sort of ID map plugin. In Sql's case, cleaning involves dropping tables, so that configuration key is specific to Sql because it illustrates what it actually does.

I'm still not sure I like clean. Could we call it dropTables() and drop_tables => FALSE?

I don't want to name the method dropTables(), since that's Sql-specific and this method should ultimately go on MigrateIdMapInterface. But renaming the config key should be okay.

OK, clean (or cleanup) it is then. Needs work for tests and rekeying the config option.

--- a/core/modules/migrate/src/Plugin/migrate/id_map/Sql.php
+++ b/core/modules/migrate/src/Plugin/migrate/id_map/Sql.php

@@ -999,4 +1004,12 @@ function (array $id) {
+  public function clean() {

I'm not sure why the patch in #44 is adding a new method, #7 points out there is already a method to clean-up, can't we not call it if we don't want to run it?

For those interested, I ran this in a custom update hook before uninstalling migrate to clean-up.

  /** @var \Drupal\migrate\Plugin\MigrationPluginManager $pluginManager */
  $pluginManager = \Drupal::service('plugin.manager.migration');

  /** @var \Drupal\migrate\Plugin\Migration $migration */
  foreach ($pluginManager->createInstances(NULL) as $migration) {
    $migration->getIdMap()->destroy();
  }

How exactly can we delete the tables now, either in CMS, or using drush?

I think this is a major problem because when I writing a new migration and modifying the migrate yml files, it looks like the migrate_map_* tables are created when the config is imported first time, but there's no way to update the mapping table schema ever since. So when I change the number of fields, ids from 2 to 1. The ID map schema still expecting sourceid1, sourceid2 instead of just sourceid1 and it causes SQL errors.

@yang_yi_cn, you can use drush to delete the map table and it will be created when the migration is run again.

#36 @Sophie.SK makes an excellent point:

The message "all data will be lost" implies that the default behaviour of removing all tables this module has created will be removed. The fact it doesn't remove the data seems like an anti-pattern.

My understanding is that the current patch does not delete the tables. So the user sees a message "all data will be lost", yet that does not happen, which is different from almost any other module. Potentially there are 100+ tables in the database that the user doesn't want - maybe even data they had deliberately not migrated for legal Data Protection reasons.

I propose that there should be a clear warning displayed to the user that data has been kept with a clear explanation of how to remove it.

My situation was that I had already uninstalled many migrate modules and removed the contrib module code from my server, and then I discovered the tables were still present. From a quick scan, I didn't see a comment above that explained what to do in that case.

Here's what I did - run the code from drush php and be careful, run at your own risk - check the output of the first command carefully before running the second.

$database_tables = db_query("SHOW TABLES LIKE 'migrate\_%'")->fetchCol();
foreach ($database_tables as $table) db_drop_table($table);

I'm now coming to the conclusion that keeping any data after a module is removed is a bad idea.

Remember that in D7 it was possible to disable a module without uninstalling it. However it didn't really work, as far as I can remember:

• Migration hooks didn't run for the disabled module so the data was some poorly defined older version that might no longer be valid.
• If you delete the module code before uninstalling then you are stuck - especially if it was custom code or a deleted insecure module.

Hence I believe D8 dropped the idea of disable and you can only uninstall, in which case D8 modules are expected to remove all their data. If migrate doesn't follow the rule, it opens up the above problems again, but even more confusing because it runs contrary to the D8 expected pattern. For example I found that the code from #48 fails to run if a single migration exists where the code from the uninstalled module is no longer present, and also see the problem in #49.

That all makes me think that migrate should conform to the D8 expected behavior. This suggests that when any migrate module is uninstalled, the corresponding migrations are deleted, because otherwise they become stale data that may not be valid later. However as per #35 we should

add a big fat warning that the map [& message] tables will be lost forever.

Will this be a surprise to people given it's the same as all other D8 modules? It seems natural to me to uninstall the migrate modules exactly when you are content the migration was successful and want to tidy up. If you aren't yet sure, would there be any disadvantage to keep the migrate modules enabled?

Let's add this as an API addition only for now. Then the UI button to delete things can be a follow-up issue. Let's add an interface that adds the cleanup method. Then when the UI or drush needs to find migrations that clean-up after themselves, they can just check if the id_map implements the cleanup interface, list the migrations and give a button or confirmation to delete the map tables.

Re-classifying

It seems to me that @benjy is right in #48. We already have an API that does exactly the job: MigrateIdMapInterface::destroy. I'm not just speculating as I have a working implementation of a drush command using this.

Therefore can move on to the next steps?

• Communicate to site admins doing an uninstall that migration tables are not automatically removed
• #2997576: Provide a drush wrapper to destroy/delete migration tables - I have uploaded a rough patch to that issue
• Provide a GUI to destroy state

Then when the UI or drush needs to find migrations that clean-up after themselves

I'm afraid that I don't follow. I found that calling destroy works on every migration with needing them to have special code to clean-up after themselves.

I need your advice please.

I'm done with all migrations and don't think I'm going to need to refer to the tables anymore and if did then I do have DB backups just in case. It would be lighter and tidier to move to production with over 100+ DB tables less.

I applied @AdamPS patch and not sure if best to uninstall the modules first or second or if it matters at all.

Drupal 8.6.1
Drush 9.4.0

Thanks

@cestmoi

not sure if best to uninstall the modules first or second or if it matters at all.

You should destroy the migrations first and uninstall the modules second. (If you no longer have the modules, then are stuck with the hack in #52.)

If it works for you, the please set the other issue to RTBC.

@AdamPS

If it works for you, the please set the other issue to RTBC.

Done successfully but didn't update the other issue for the reason mentioned there (12 tables still not-destroyed).

Thanks for the patch :)

Discussed in migrate maintainers meeting with @mikelutz, @phenaproxima, @quietone, @heddn today. The question came up about do we really need an interface. In 9.0 we'll probably have enough changes in the migrate map plugin system that these might not exist in their current form. And we aren't aware of any non-SQL based id maps. So, to keep things easier, it was proposed to add to the existing sql id map concrete class (w/o any interface). Then we can just do an instance_of check when we iterate over id maps and only call the sql based ones.

The method to add on the sql map is dropTables(). Add it there, and add tests that when it is called we do in fact destroy the mapping and message tables.

In a follow-up (let's open one), we will then add into migrate_drupal_ui an admin page w/ checkboxes and the ability to drop on a per migration basis the backing tables for migrations. I'll let that issue flesh out the UI and if we really want to use checkboxes and how it looks, but this should give an implementer an idea of architecturally what we are looking for.

@heddn thanks for taking time to move this one forward.

However I still don't understand why we need a new dropTables() method. We already have an API that does exactly the job: MigrateIdMapInterface::destroy.

  /**
   * {@inheritdoc}
   */
  public function destroy() {
    $this->getDatabase()->schema()->dropTable($this->mapTableName());
    $this->getDatabase()->schema()->dropTable($this->messageTableName());
  }

Excuse me for repeating myself, but I don't yet see that anyone has explained why I'm wrong.

If you just want to manually delete the tables this will generate the SQL statements to do it, after running this SQL select print text with full view at the bottom of phpMyAdmin to show the full names and then copy and run the generated statements.

SELECT CONCAT('DROP TABLE ',  `TABLE_NAME`, ';') 
FROM INFORMATION_SCHEMA.TABLES 
WHERE TABLE_NAME LIKE 'migrate_message_%' OR TABLE_NAME LIKE 'migrate_map_%';

Is this still a required step, if so what are the pros and cons of taking this action?

I used drop tables in migration tables and it worked out fine....no problems now for over 1 year

Added a brief statement to the IS explaining why the tables are not removed on uninstall.

Reviewed in weekly migration maintainers call. Here's some thoughts.

Generate a Form (and route) in Drupal console in the migrate module. This form lists all migrations with a checkbox in front of it. It's submit handler instantiates all the selected migrations, then calls the id map on them and finally, calls destroy. Then print out what migrations were successfully removed. Profit.

Also, patches accepted in migrate_tools downstream that do similarly.

Also, patches accepted in migrate_tools downstream that do similarly.

There is an initial version of a patch to add a drush command here #2997576: Provide a drush wrapper to destroy/delete migration tables.

@heddn So will the patch be pushed the current sable and remove the old migration data once uninstalled, or still a manual process?

I envision the automated uninstall to be a follow-up. Let's add a manual process, then discuss how we want to make that automated.

Working on #67

Addressing #67

Can we add a Functional test of this too?

+++ b/core/modules/migrate/src/Form/RemoveMigrationTables.php
@@ -0,0 +1,123 @@
+    $migration_plugins = array_filter($migration_plugins, function ($migration) {
+        return $migration->getIdMap()->importedCount();
+    });

I think we want to remove all migrations that have migrate mapping and message tables. Not just if they are empty or not.

Thanks for review @heddn.
I am attaching a patch to destroy migrations that have migrate map or message tables.
This still needs tests.

adding tests.

1. +++ b/core/modules/migrate/src/Form/RemoveMigrationTables.php
   --- /dev/null
   +++ b/core/modules/migrate/tests/modules/remove_migration_tables/migrations/remove_migration_tables.yml
   
   +++ b/core/modules/migrate/tests/modules/remove_migration_tables/migrations/remove_migration_tables.yml
   @@ -0,0 +1,15 @@
   diff --git a/core/modules/migrate/tests/modules/remove_migration_tables/remove_migration_tables.info.yml b/core/modules/migrate/tests/modules/remove_migration_tables/remove_migration_tables.info.yml
   

   Rather than creating a new test module, could we use migrate_external_translated_test or migrate_high_water_test or migration_directory_test?

2. +++ b/core/modules/migrate/tests/src/Functional/RemoveMigrationTablesTest.php
   @@ -0,0 +1,67 @@
   +    $type = $this->container->get('entity_type.manager')->getStorage('node_type')
   +      ->create([
   +        'type' => 'article',
   +        'name' => 'Article',
   +      ]);
   +    $type->save();
   

   It is more typical for this to be:

       // Create the node bundles required for testing.
       $type = NodeType::create([
         'type' => 'article',
         'name' => 'Article',
       ]);
       $type->save();
   

3. +++ b/core/modules/migrate/tests/src/Functional/RemoveMigrationTablesTest.php
   @@ -0,0 +1,67 @@
   +    $this->container->get('router.builder')->rebuild();
   

   This shouldn't be necessary.

4. +++ b/core/modules/migrate/tests/src/Functional/RemoveMigrationTablesTest.php
   @@ -0,0 +1,67 @@
   +    $account = $this->drupalCreateUser(['access administration pages']);
   

   Create the user in setup above. Like

       // Create some users.
       $this->adminUser = $this->drupalCreateUser(['access administration pages']);
   

5. +++ b/core/modules/migrate/src/Form/RemoveMigrationTables.php
   @@ -0,0 +1,141 @@
   +  public function buildForm(array $form, FormStateInterface $form_state) {
   

   This form could use some preliminary text stating what the form will do. That with it you can remove migration mapping and message tables. This is a process that is not reversible and if you delete the table it cannot be restored. The tables can be useful after migrations have run for troubleshooting. Remove them only with caution.

6. +++ b/core/modules/migrate/src/Form/RemoveMigrationTables.php
   @@ -0,0 +1,141 @@
   +    // $migration = \Drupal::service('plugin.manager.migration')->createInstance('d6_user');
   +
   +    // $executable = new MigrateExecutable($migration);
   +    // $executable->import();
   

   Let's remove this commented out code.

7. +++ b/core/modules/migrate/src/Form/RemoveMigrationTables.php
   @@ -0,0 +1,141 @@
   +    // Collect migrations that have migrate map or message tables on the database.
   

   Longer than 80 chars.

8. +++ b/core/modules/migrate/src/Form/RemoveMigrationTables.php
   @@ -0,0 +1,141 @@
   +      '#empty' => $this->t('No migrations available.'),
   

   This might read, "No migration tables exist."

9. +++ b/core/modules/migrate/src/Form/RemoveMigrationTables.php
   @@ -0,0 +1,141 @@
   +      if (!empty($value)) {
   

   Could we just do if ($value) {?

10. +++ b/core/modules/migrate/src/Form/RemoveMigrationTables.php
    @@ -0,0 +1,141 @@
    +        '#markup' => $this->t('The following migrations have been detroyed.'),
    
    +++ b/core/modules/migrate/tests/src/Functional/RemoveMigrationTablesTest.php
    @@ -0,0 +1,67 @@
    +    $this->assertSession()->pageTextContains('The following migrations have been detroyed.');
    

    Misspelling of 'destroyed'.

Working on #77

addressing #77

Closer. Looking very nice. Can we get a screenshot of the the page added to the IS? Tagging Novice for the screenshot.

1. +++ b/core/modules/migrate/src/Form/RemoveMigrationTables.php
   index 0000000000..b30cbb470b
   --- /dev/null
   
   --- /dev/null
   +++ b/core/modules/migrate/tests/modules/migrate_external_translated_test/migrations/remove_migration_tables.yml
   
   +++ b/core/modules/migrate/tests/modules/migrate_external_translated_test/migrations/remove_migration_tables.yml
   +++ b/core/modules/migrate/tests/modules/migrate_external_translated_test/migrations/remove_migration_tables.yml
   @@ -0,0 +1,15 @@
   
   @@ -0,0 +1,15 @@
   +id: remove_migration_tables
   +label: Remove Migration Tables
   +source:
   

   I don't think we need a new yml file. Just use one of the existing ones in these test modules.

2. +++ b/core/modules/migrate/tests/src/Functional/RemoveMigrationTablesTest.php
   @@ -0,0 +1,66 @@
   +    $this->adminUser = $this->drupalCreateUser(['access administration pages']);
   

   This should be done in setup. And we're missing the definition of protected $adminUser at the class level.

3. +++ b/core/modules/migrate/src/Form/RemoveMigrationTables.php
   @@ -0,0 +1,141 @@
   +      '#markup' => '<p>' . t('With this form you can remove migration mapping and message tables. This is a process that is not reversible and if you delete the table it cannot be restored. The tables can be useful after migrations have run for troubleshooting. Remove them only with caution.') . '</p>',
   

   "if you delete the table it cannot be restored"...

   Let's make that a plural, 'tables'.

addressing points of #80. This still needs a screenshot.

@edysmp, thanks for moving this along.

I've only just skimmed the patch and what strikes me is 'remove' when elsewhere in Drupal 'delete' is used. I think, that once we have a screenshot we need to tag this (which tag?) for the UI folks to check the wording.

Thanks @quietone for the review, maybe this tag it's could work?

I have attached a screenshot of the /admin/migrate/destroy page

@heddn does the screenshot I provided work?

re #88, it is perfect. Which is why we can now proceed to usability review.

Nice work, really pleased to see the progress.

The page title should be in sentence case

+++ b/core/modules/migrate/migrate.routing.yml
@@ -0,0 +1,8 @@
+    _title: 'Remove Migration Tables'

Page title should be sentence case. "Remove migration tables". See https://www.drupal.org/node/604342

What is displayed if there are no migrations tables? Can there be a screenshot of that case?

+++ b/core/modules/migrate/src/Form/RemoveMigrationTables.php
@@ -0,0 +1,141 @@
+      '#markup' => '<p>' . t('With this form you can remove migration mapping and message tables. This is a process that is not reversible and if you delete the tables it cannot be restored. The tables can be useful after migrations have run for troubleshooting. Remove them only with caution.') . '</p>',

If this is going to have 'caution' message then I think it should also have a confirmation form. Give folks a chance to back out. Apologies if it there I am missing it.

Leaving at NR to not block a usability review.

Regarding #85, wording in the screenshot may not be understood. "The tables can be useful after migrations have run for troubleshooting."

1. In the case of an unsuccessful migration, would the average user know how to use these tables for troubleshooting?
2. If a migration appears successful, should migration tables be deleted?

Lets face it, most individuals will simply browse the site to confirm content has been migrated. Perhaps outline when the migration tables should or should not be deleted? :)

I have attached a new screenshot. I am not a UX or Content reviewer by any means, but I feel that the section inside the help text that says 'if you delete the tables it cannot be restored.' should read 'if you delete the tables they cannot be restored.'

Thanks to all for reviewing this.
I am fixing some wording issues:
#82
#90.1
#92

Adding a Confirm form.

In testing #94 to get the screenshots, I noticed two things:

#1) The 'Delete Migration Tables' button is no longer available, and has been replaced by a link.

#2) The confirmation page displays li elements not selected on the previous form as '0'. So for example if you only selected one of the elements to delete out of 5, the remaining 4 elements on the confirm page show up as

. I have attached a couple of screenshots for clarity.

EDITED for correctness. I prematurely assumed the danger--button was broken. After looking further, I think that's the default behavior for buttons that are danger. In looking at other places in the UI, I had a hard time finding any other places this button was used, so I wonder if for consistency we should just leave it as the button--primary that appears on the confirmation page.

Will remove the files from display once this issue is resolved, and replace them with two accurate screenshots.

Thanks @jfurnas for a quick review. I will check it.

I revised my review in my previous comment. Please re-review it for more accurate information :)

Only show seleted migrations.

Turn back primary button.

Leaving at NR for UX feedback, but here's some things I'd like to see addressed from a non visual perspective.

1. +++ b/core/modules/migrate/migrate.routing.yml
   @@ -0,0 +1,16 @@
   +  requirements:
   +    _access: 'TRUE'
   ...
   +  requirements:
   +    _access: 'TRUE'
   

   The access here should be tied to some type of more elevated permission. Anonymous shouldn't be able to delete migrations.

2. +++ b/core/modules/migrate/tests/src/Functional/DeleteMigrationTablesTest.php
   @@ -0,0 +1,61 @@
   +    $this->submitForm($edit, 'Delete migration tables');
   +    $this->assertSession()->pageTextContains('Are you sure you want to delete migration tables?');
   +
   +    $this->submitForm([], 'Delete');
   

   Let's include an assertion for the names of migrations we expect to see and ones do no not expect to see on this confirm page.

Feedback from today's UX meeting:

• Wordsmith the text at the top of the page. Both on the first and confirm pages. They could probably repeat the same text.
• Make it explicit on the top of confirm page what will be deleted. Perhaps:

• Database tables related to the following migrations will be deleted. This is a process that is not reversible. If you delete the tables they cannot be restored. These tables can be useful after migrations have run for troubleshooting. Delete them only with caution.

• Need to test w/ and w/o labels. If no label, then we shouldn't put the migration ID in parenthasis.
• Wording for the submit button on the first page should change to: 'Delete tables'

Oooh, wordsmithing. This is interesting, and I just noticed it, that this is using 'migration' in the UI, where as previous work on the UI specifically avoided 'migration' in favor of 'upgrade'. Maybe the text should also change to 'upgrade tables'? I'm leaning that way right now.
And let's change 'process' to 'action'

Just trying out another text to see what it looks like.
"Database tables for the following upgrades will be deleted. This action is not reversible. These tables are useful for troubleshooting and if deleted, they cannot be restored. Delete with caution."

re #103: previous UI rendering is part of migrate drupal ui, which is all about upgrading. These are changes added to migrate base module. So I feel that the terms, 'migrate' or 'migration' are appropriate. Since we are dealing with migrations in the migrate module. Not necessarily upgrade migrations.

@heddn, yes. good point. Migration is appropriate.

fixing #101 and #102.

Are the screenshots in the issue description up to date? (It looks as though they were updated earlier today.)

The issue description does not mention the navigation to get to the "Delete migration tables" page.

We discussed this again at the Drupal usability meeting - 2019-05-07 (recording on YouTube) and came up with some additional recommendations. Please either implement these as part of this issue or create follow-ups.

Requirements

1. Add a documentation page with more information. At least summarize the reasons discussed here for why one might want to keep the tables.
2. Give instructions for making a backup (dump) of the tables to be removed.

Nice to have

1. In the table, say how big the tables are. (Probably the number of rows is the easiest measure.)
2. Is there any good way in Drupal core to group the migrations instead of just listing them alphabetically?
3. Make it easy to create a backup of the tables to be removed. Perhaps a download button on the "Delete migration tables" page. Perhaps show a SQL command for generating a backup of the selected tables either on that page or on the confirmation page.

Would it be feasible to create a report on deletion of the content of the tables rather than suggesting a backup?
(Sorry if I'm in way over my head with this comment.)

If we have uninstalled the migration modules, how do we use this patch to remove the tables?

Revisited this and changed all references to 'migration' in the UI to upgrade and some other tweaks. See the screenshots in the IS.

#018 Requirements 1 - Added a doc page, URL in the IS

1. +++ b/core/modules/migrate/src/Form/DeleteMigrationTables.php
   @@ -0,0 +1,144 @@
   +      return ($this->database->schema()
   +          ->tableExists($idMap->mapTableName()) || $this->database->schema()
   +          ->tableExists($idMap->messageTableName()));
   

   Indentation is wrong here.

2. +++ b/core/modules/migrate/tests/modules/delete_migration_tables/delete_migration_tables.info.yml
   @@ -0,0 +1,6 @@
   +core: 8.x
   

   This isn't right for D9, It's breaking tests, I think we settled on not needing it anymore for test modules, but check.

This should address all the points in #114. The test needed some tweaking and I removed the unnecessary quotes from yml files.

+++ b/core/modules/migrate/src/Form/DeleteMigrationTables.php
@@ -0,0 +1,143 @@
+      'migration' => $this->t('Upgrade'),
...
+      '#markup' => '<p>' . $this->t('Database tables related to the following upgrades will be deleted. This is a process that is not reversible. If you delete the tables they cannot be restored. These tables can be useful after upgrade for troubleshooting. Delete them only with caution.') . '</p>',
...
+      '#empty' => $this->t('No upgrade tables available.'),

+++ b/core/modules/migrate/src/Form/DeleteMigrationTablesConfirm.php
@@ -0,0 +1,151 @@
+    return $this->t('Are you sure you want to delete upgrade tables?');
...
+        '#markup' => '<p>' . $this->t('Database tables related to the following upgrades will be deleted. This is a process that is not reversible. If you delete the tables they cannot be restored. These tables can be useful after upgrade for troubleshooting. Delete them only with caution.') . '</p>',
...
+        '#markup' => $this->t('The following upgrade tables will be deleted.'),
...
+          '#markup' => $this->t('The following upgrade tables have been deleted.'),

Here (and maybe elsewhere). I'm not as keen on the name 'Upgrade" for deleting the migration map and message tables. We have the folks who use migrate for upgrades, but we also have many folks using it for migrations. And all the additions here are in the base API of migrate module. So I guess I'd vote for some name more akin to migrate.

OK, that make sense. Changes made.

Does this need a menu entry?

Exposing the tables to remove looks good.

It seemed odd to me that it's at admin/migrate/destroy without a parent menu item, yep - should I see other items at admin/migrate?

The text says "Database tables related to the following upgrades will be deleted." - I see your comment 103 above but I do feel like "migrate" is the right term here. I'm not yet familiar with the decision to rename "migrate" to "upgrade" (oh, this is UI stuff that I've missed, ok).

I have a database which has a bunch of these old tables remaining but the migrations don't get picked up, because the Migration config items are already removed.

With the patch currently, if there are no discovered migrations to clean up, we see an E_NOTICE in DeleteMigrationTablesConfirm::buildForm() where $migrations is not an iterable array.

If we have uninstalled the migration modules, how do we use this patch to remove the tables?

@tjtj, it doesn't look like this patch supports that usage. However if you're in that situation (as I am) you could do something like this.

Good point, I reckon we should include migrate tables that exist but the migration itself is no longer available.

But how to display that? The current form uses the migration label and migration machine name as table headers. Neither of those exists for tables with no migration. And we can't assume that the part of the name after migrate_map_ or migrate_message_ is the migration name because the table names may be truncated.

Ideas?

We could add a second table (set of inputs) for "not associated with migrations" and handle those similarly. It would require separate presentation I think as the temp storage passes them by migration currently, and the second set would need to be passed by table name. Form validation would prevent someone tampering and deleting tables they shouldn't have access to.

Here is an attempt at adding the migration tables that exist in the db but do not have a migration. There are two values in the tempstore, 'migrations' and 'tables'. Just takes a bit of fiddling to get the list of tables without a migration. The Confirm form displays both, if they exist, and the final confirmation does as well. And I've added a menu item in admin/reports as suggested by xurizaemon.

This will need some screenshots at some point.

We discussed this issue at #3171562: [meeting] Migrate Meeting 2020-09-24 and agreed that the new table needs a re-review for usability, so I am adding the issue tag for that. In order to facilitate the review, we will need to update the issue summary (at least new screenshots as mentioned in #121).

Added updated screenshots to the IS, fixed the tests and changed the form description from 'Destroy ..' to 'Delete ...'.

I think that the path should be '/admin/migrate/delete' instead of '/admin/migrate/destroy' but I haven't made those changes. Anyone agree with the suggestion?

+++ b/core/modules/migrate/src/Form/DeleteMigrationTables.php
@@ -0,0 +1,163 @@
+ * Class DeleteMigrationTables.

Change 'Class DeleteMigrationTables' to 'Provides a from for deleting migration table'

And add @internal. See, say BookAdminForm.php, for an example.

This change is suitable for a novice, adding tag. Remember to remove the tag when this work is done.

Made the changes suggested in #126.

Last patch had whitespace error.

@ayushmishra206, thanks for fixing this so quickly. Since you've uploaded #129 so soon after #128, it is a good idea to cancel the test for the patch in #128. Just click on 'PHP 7.3 & MySQL 5.7 Running' and there will be a cancel button.

Fixed typo ("from for" > "form for").

EDIT: I don't see the cancel button @quietone, neither in the two tests started by @ayushmishra206, nor under my own test in this comment. Perhaps it's a question of permissions?
EDIT2: I do see the "Cancel test" button in my own test now, so you can actually cancel a test if you started it, which makes sense.

+++ b/core/modules/migrate/migrate.routing.yml
@@ -0,0 +1,16 @@
+  path: '/admin/migrate/destroy'
...
+  path: '/admin/migrate/confirm-destroy'

Re #125: Delete is more in line with what we typically call these routes and action inside of Drupal.

I agree @quietone and @heddn: delete is more in line with naming conventions, this patch updates the path names.

I have updated the corresponding issue about Drush integration #2997576: Provide a drush wrapper to destroy/delete migration tables but am not sure if it will get picked up, since the Migrate Tools issue queue seems to have been shut down.

Maybe the recent improvements in GitLab Integration with drupal.org makes it worth reconsidering opening the Migrate Tools issue queue at drupal.org?

This needs work to not show an error when there are no migration tables.

There is also an confusing mix of name when the patch from #3063856: Add ability to view migrate_message table data is applied as well. In one patch they are called Upgrade Messages and in the other Migrate Tables. We need to be consistent.

Catch pdoexceptions when creating the migrations. That way, if the source db is not available the form will not fatal. And since the migrations are not created any migrate tables will be list in the 'Migration tables without migration' section.

Forgot to run commit-code-check.

In core, we avoid referring to "migration" in UI text, since the core UI only supports upgrades from earlier versions of Drupal. We should update the text to refer to "upgrade".

This is especially important if #3063856 is committed in its current form. That issue and this one add links to the Administration > Reports menu, currently "Migrate tables" (this issue) and "Upgrade messages" (the other issue). Those links should use consistent terms. See the screenshot in #3063856-57: Add ability to view migrate_message table data.

Change UI text from migration to upgrade. And also change the delete form to show the migration label instead of the id.

Renaming variables to be more like what is typically in migration files.

There is, at least, the following to do.

With the patch from #3063856: Add ability to view migrate_message table data applied the page admin/reports now has 'Upgrade log' and 'Upgrade tables'. The link for 'Upgrade log' is 'admin/reports/upgrade' and for 'Update tables' it is 'admin/upgrade/delete', which is odd. The latter is added in this issue and should change, but to what?

When there isn't a migration for existing migrate tables the following is displayed, 'Upgrade tables without migration'. That still uses migration and needs to be fixed. Haven't though of what to replace it with.

All the UI text needs to handle singular and plural. And all the text needs review.

New screenshots for the IS.

If we put this functionality into migrate.module, we shouldn't use upgrade in the UI elements. migrate.module provides an API. It doesn't provide an implementation for D2D upgrades. That's rather migrate_drupal_ui.module. So we should continue to call this "migrate" or "migration", given the implementation going into migrate.module.

And for the record, I think this should go into the API module. People use the migrate module for lots of different things. We broke the features into parts that make logical sense. A module for an API, another for an upgrade implementation. Having a UI to clean-up migrations seems reasonable to put into the API module.

We discussed this issue at #3201985: Drupal Usability Meeting 2021-03-12. Thanks for updating the screenshots in the issue summary!

It is a hard problem that the Migrate API serves different audiences. We try to avoid using the term "migrate" in the site upgrade process, so using it here could be confusing to upgraders. This is probably the audience that is most likely to want to delete the tables, and to do it from the UI. On the other hand, someone using the Migrate API for a different purpose, such as upgrading/moving/migrating (whatever they choose to call it) from another system, will be confused by "upgrade".

The consensus at the meeting is that we should use the term "migrate" here, not "upgrade". I apologize for the back and forth on this point. On the plus side, this recommendation is consistent with #143.

There are also two more specific suggestions that came out of the meeting:

1. On the confirmation page, show the table name as well as the related migration.
2. Do some more wordsmithing. For example, replace "This is a process that is not reversible" with "This action cannot be undone", which is used pretty consistently in Drupal.

I will make a more specific suggestion for (2) when I have a little more time.

This patch renames 'upgrade' to 'migration' for the forms. Also, changes the text for #144.2.

#144.1. Can there be more clarification of how to display the migration name and the two map table names? For example for the D7 color migration the data is "Color", "migrate_map_d7_color", and "migrate_message_d7_color". The easiest is to implode them but that isn't very readable and will be even worse when multiple tables are deleted.

This will need new screenshot too.

NW for 144.2, but looks like we need some additional comments from @benjifisher to clarify next steps.

144.2. Changed to 'this action cannot be undone.' Not running tests because this needs more work and it is a text only change.

Updated the relevant screenshot.

This patch will fix the following error, if anyone is trying this out: https://www.drupal.org/project/smtp/issues/3190531

Drupal\Component\Plugin\Exception\PluginException: The plugin (d7_system_mail) did not specify an instance class. in Drupal\Component\Plugin\Factory\DefaultFactory::getPluginClass() (line 79 of core/lib/Drupal/Component/Plugin/Factory/DefaultFactory.php).
Drupal\Core\Plugin\Factory\ContainerFactory->createInstance('d7_system_mail', Array) (Line: 111)
Drupal\migrate\Plugin\MigrationPluginManager->createInstances(Array) (Line: 79)
Drupal\migrate\Form\DeleteMigrationTables->buildForm(Array, Object)
call_user_func_array(Array, Array) (Line: 531)
Drupal\Core\Form\FormBuilder->retrieveForm('delete_migration_tables', Object) (Line: 278)
Drupal\Core\Form\FormBuilder->buildForm(Object, Object) (Line: 73)

I ran into this issue for a different reason. I haven't finished migrations yet, but I found that when you revert your migrations then rename a couple migrations like so:

migrate_plus.migration.b.yml => migrate_plus.migration.c.yml
migrate_plus.migration.a.yml => migrate_plus.migration.b.yml

What you are left with is that the migration map table for "b" in this example can have the wrong table structure/columns. I had to delete the tables and start over. While it's a good thing that the tables aren't removed on module uninstall (it's helpful to reinstall a module to pick up changes to custom migrations), it was certainly confusing why my migrations weren't working.

The patch still applies.

I am setting to Needs Review and noting that @benjifisher commented in #44 that he was going to add some more suggestions. Still, others can add reviews if they wish.

For those who do migrations via Drush, adding a Drush command to remove a migration table might be helpful. Otherwise, it is always possible to execute a DROP TABLE command in SQL via Drush.

For anyone who want to drop all migrate tables in one go, here is a command to do it

drush sqlq --extra='-ss' "show tables like 'migrate_%'" | sed -r '/^\s*$/d' | while read TABLE ; do echo "drush sqlq 'DROP TABLE $TABLE;'" ; done

credits to @xurizaemon on slack

This command is dangerous and use it only if you know what you are doing. IT WILL DROP ALL MIGRATE TABLES.

Moving this back to NW, as it definitely needs some work. I'm also not sold on putting this in the migrate module. I feel like for core, We should add an option in migrate drupal UI to remove tables when done. We might also add a drush command to the core migrate drush commands to remove the tables. As far as a UX like this to remove all migration tables, I think it should live in migrate_tools. I'm hesitant to add any UX to the api module directly, I'm a fan of keeping that purely api. Outside of the migrate_drupal_ui, the migrate api requires migrations to be run through drush, migrate_tools UX, or custom code, and I feel like those are the places where table removal should happen. The api module should only provide an api method of removing the tables, which it does through MigrateIdMapInterface::destroy()

Ran into this again while migrating my own site. At minimum this needs clear documentation.

The migrate_tools issue that #159 refers to (#2997576: Provide a drush wrapper to destroy/delete migration tables) has not progressed either, which is why I propose a pragmatic docs-only solution for core.

The Drupal 7 EOL is in a few days, so many more people are bound to run into this very soon 😅

Until a Drush command or UI is ready, a documentation page with a solution on how to clean up migrate tables after upgrading from Drupal 7 to Drupal 10 would be great.

I have updated After the upgrade, linking to this issue and #2997576: Provide a drush wrapper to destroy/delete migration tables, adding a Cleaning up, delete migration tables section.

This page could also include a solution on how to take care of old, lingering blocks in configuration, as seen in #2694895: Single item import fails with validation errors from other config, so I have added a section Clean up orphaned blocks from a deleted theme.

Ideally, both "Cleaning up, delete migration tables" and "Clean up orphaned blocks from a deleted theme" should contain solutions, and not just link to these issues, and anyone should feel free to add them to that doc page.

Re-Rolled patch from #149 against 11.x to bump the issue.

Left comments on MR.

Closing MR and hiding files as we decided on a documentation only solution for core. Work on a UI should be done in drush or migrate_tools.