Simpletest cache tables remain after uninstall

[izmeez]

Description of the bug

Both the core simpletest module and the contrib simpletester module were enabled and simpletester used to run some tests. Later both modules were disabled and uninstalled. However, the database still includes many tables beginning with simpletest_cache_.

To prevent this, before disabling the simpletest module, the user must click the Clean Environment button on the Testing configuration page.

Expected behavior

Uninstall should remove all related tables.

Discussion in Zulip suggests that simpletest_uninstall() should remove those tables and it should be properly uninstalled even when it is a dependency of another module.

[izmeez]

Using just the core simpletest module:

1. Enable Testing (simpletest).
2. Go to configure settings (leave as default).
3. Go to testing list and run a test, such as all 4 tests for adminbar.
4. Disable and uninstall Testing (simpletest).
5. Examine database, see multiple tables simpletest_cache_ are present.
6. Enable Testing module, go to configure and click Clean Environment
7. Shows - Removed 132 profile cache tables.
8. Disable and uninstall module.
9. Examine database and no tables named simpletest_cache_ are found.

It appears the profile cache tables are not removed by uninstall.

[avpaderno]

simpletest_clean_database(), which is called by simpletest_uninstall(), skips the database tables whose name does not have a number after simpletest.

// Strip the prefix and skip tables without digits following "simpletest",
// e.g. {simpletest_prefix}.
if (preg_match('/simpletest\d+.*/', $table, $matches)) {
  db_drop_table($matches[0]);
  $count++;
}

simpletest_clean_database() is called by simpletest_clean_environment() too. It should probably have a parameter that is TRUE when it is called by simpletest_uninstall().

function simpletest_clean_database($drop_all_tables = FALSE) {
  $tables = db_find_tables(Database::getConnection()->prefixTables('{simpletest}') . '%');
  $schema = backdrop_get_schema_unprocessed('simpletest');
  $count = 0;
  foreach (array_diff_key($tables, $schema) as $table) {
    if ($drop_all_tables) {
      db_drop_table($table);
    }
    elseif (preg_match('/simpletest\d+.*/', $table, $matches)) {
      // Strip the prefix and skip tables without digits following "simpletest",
      // e.g. {simpletest_prefix}.
      db_drop_table($matches[0]);
    }

    $count++;
  }

  if ($count > 0) {
    backdrop_set_message(format_plural($count, 'Removed 1 leftover table.', 'Removed @count leftover tables.'));
  }
  else {
    backdrop_set_message(t('No leftover tables to remove.'));
  }
}

simpletest_uninstall() will call simpletest_clean_database(TRUE); other functions will call simpletest_clean_database(), and get the usual behavior.

[avpaderno]

Actually, the code I shown would not work because simpletest_schema() does not return any schema for simpletest_cache_ tables.
simpletest_clean_database() should explicitly drop tables with a name that starts with simpletest.

[avpaderno]

The fix is simpler: simpletest_uninstall() is missing a call to simpletest_clean_profile_cache_tables().

function simpletest_uninstall() {
  backdrop_load('module', 'simpletest');
  simpletest_clean_database();
  simpletest_clean_profile_cache_tables();

  // Remove generated files.
  file_unmanaged_delete_recursive('public://simpletest');
}

[avpaderno]

I followed the steps reported in the issue summary. In the live preview, when I uninstall the module, I get the following message as expected.

[izmeez]

The PR works for me and the code is a simple one line fix. I've changed labels to pr - works for me and removed pr - needs code review. I'm not sure if I can add pr - ready to be committed or if that requires a core committer to review?

[avpaderno]

@izmeez Everybody can add pr - ready to be committed (except the person who created the PR).
I would wait a few days, just in case somebody can find a reason for not calling simpletest_clean_profile_cache_tables() when the module is uninstalled.