MDL-58584 core_ddl: getDropTableSQL should not do more than what it says

lib/ddl/database_manager.php

@@ -330,6 +330,8 @@ class database_manager {
             throw new ddl_exception('ddlunknownerror', null, 'table drop sql not generated');
         }
         $this->execute_sql_arr($sqlarr, array($xmldb_table->getName()));
+
+        $this->generator->cleanup_after_drop($xmldb_table);
     }
 
     /**

lib/ddl/mssql_sql_generator.php

@@ -183,21 +183,6 @@ class mssql_sql_generator extends sql_generator {
         return $sqlarr;
     }
 
-    /**
-     * Given one correct xmldb_table, returns the SQL statements
-     * to drop it (inside one array).
-     *
-     * @param xmldb_table $xmldb_table The table to drop.
-     * @return array SQL statement(s) for dropping the specified table.
-     */
-    public function getDropTableSQL($xmldb_table) {
-        $sqlarr = parent::getDropTableSQL($xmldb_table);
-        if ($this->temptables->is_temptable($xmldb_table->getName())) {
-            $this->temptables->delete_temptable($xmldb_table->getName());
-        }
-        return $sqlarr;
-    }
-
     /**
      * Given one XMLDB Type, length and decimals, returns the DB proper SQL type.
      *

lib/ddl/mysql_sql_generator.php

@@ -384,7 +384,6 @@ class mysql_sql_generator extends sql_generator {
         $sqlarr = parent::getDropTableSQL($xmldb_table);
         if ($this->temptables->is_temptable($xmldb_table->getName())) {
             $sqlarr = preg_replace('/^DROP TABLE/', "DROP TEMPORARY TABLE", $sqlarr);
-            $this->temptables->delete_temptable($xmldb_table->getName());
         }
         return $sqlarr;
     }

lib/ddl/oracle_sql_generator.php

@@ -211,7 +211,6 @@ class oracle_sql_generator extends sql_generator {
         $sqlarr = parent::getDropTableSQL($xmldb_table);
         if ($this->temptables->is_temptable($xmldb_table->getName())) {
             array_unshift($sqlarr, "TRUNCATE TABLE ". $this->getTableName($xmldb_table)); // oracle requires truncate before being able to drop a temp table
-            $this->temptables->delete_temptable($xmldb_table->getName());
         }
         return $sqlarr;
     }

lib/ddl/postgres_sql_generator.php

@@ -103,21 +103,6 @@ class postgres_sql_generator extends sql_generator {
         return $sqlarr;
     }
 
-    /**
-     * Given one correct xmldb_table, returns the SQL statements
-     * to drop it (inside one array).
-     *
-     * @param xmldb_table $xmldb_table The table to drop.
-     * @return array SQL statement(s) for dropping the specified table.
-     */
-    public function getDropTableSQL($xmldb_table) {
-        $sqlarr = parent::getDropTableSQL($xmldb_table);
-        if ($this->temptables->is_temptable($xmldb_table->getName())) {
-            $this->temptables->delete_temptable($xmldb_table->getName());
-        }
-        return $sqlarr;
-    }
-
     /**
      * Given one correct xmldb_index, returns the SQL statements
      * needed to create it (in array).

lib/ddl/sql_generator.php

@@ -653,7 +653,7 @@ abstract class sql_generator {
     }
 
     /**
-     * Given one correct xmldb_table and the new name, returns the SQL statements
+     * Given one correct xmldb_table, returns the SQL statements
      * to drop it (inside one array). Works also for temporary tables.
      *
      * @param xmldb_table $xmldb_table The table to drop.
@@ -674,6 +674,17 @@ abstract class sql_generator {
         return $results;
     }
 
+    /**
+     * Performs any clean up that needs to be done after a table is dropped.
+     *
+     * @param xmldb_table $table
+     */
+    public function cleanup_after_drop(xmldb_table $table): void {
+        if ($this->temptables->is_temptable($table->getName())) {
+            $this->temptables->delete_temptable($table->getName());
+        }
+    }
+
     /**
      * Given one xmldb_table and one xmldb_field, return the SQL statements needed to add the field to the table.
      *

lib/ddl/tests/ddl_test.php

@@ -1828,6 +1828,74 @@ class core_ddl_testcase extends database_driver_testcase {
         $this->assertFalse($dbman->table_exists('test_table1'));
     }
 
+    /**
+     * get_columns should return an empty array for ex-temptables.
+     */
+    public function test_leftover_temp_tables_columns() {
+        $DB = $this->tdb; // Do not use global $DB!
+        $dbman = $this->tdb->get_manager();
+
+        // Create temp table0.
+        $table0 = $this->tables['test_table0'];
+        $dbman->create_temp_table($table0);
+
+        $dbman->drop_table($table0);
+
+        // Get columns and perform some basic tests.
+        $columns = $DB->get_columns('test_table0');
+        $this->assertEquals([], $columns);
+    }
+
+    /**
+     * Deleting a temp table should not purge the whole cache
+     */
+    public function test_leftover_temp_tables_cache() {
+        $DB = $this->tdb; // Do not use global $DB!
+        $dbman = $this->tdb->get_manager();
+
+        // Create 2 temp tables.
+        $table0 = $this->tables['test_table0'];
+        $dbman->create_temp_table($table0);
+        $table1 = $this->tables['test_table1'];
+        $dbman->create_temp_table($table1);
+
+        // Create a normal table.
+        $table2 = new xmldb_table ('test_table2');
+        $table2->add_field('id', XMLDB_TYPE_INTEGER, '10', null, XMLDB_NOTNULL, XMLDB_SEQUENCE, null);
+        $table2->add_field('course', XMLDB_TYPE_INTEGER, '10', null, XMLDB_NOTNULL, null, '0');
+        $table2->add_key('primary', XMLDB_KEY_PRIMARY, array('id'));
+        $table2->setComment("This is a test'n drop table. You can drop it safely");
+        $this->tables[$table2->getName()] = $table2;
+        $dbman->create_table($table2);
+
+        // Get columns for the tables, so that relevant caches are populated with their data.
+        $DB->get_columns('test_table0');
+        $DB->get_columns('test_table1');
+        $DB->get_columns('test_table2');
+
+        $dbman->drop_table($table0);
+
+        $rc = new ReflectionClass('moodle_database');
+        $rcm = $rc->getMethod('get_temp_tables_cache');
+        $rcm->setAccessible(true);
+        $metacachetemp = $rcm->invokeArgs($DB, []);
+
+        // Data of test_table0 should be removed from the cache.
+        $this->assertEquals(false, $metacachetemp->has('test_table0'));
+
+        // Data of test_table1 should be intact.
+        $this->assertEquals(true, $metacachetemp->has('test_table1'));
+
+        $rc = new ReflectionClass('moodle_database');
+        $rcm = $rc->getMethod('get_metacache');
+        $rcm->setAccessible(true);
+        $metacache = $rcm->invokeArgs($DB, []);
+
+        // Data of test_table2 should be intact.
+        $this->assertEquals(true, $metacache->has('test_table2'));
+
+    }
+
     public function test_reset_sequence() {
         $DB = $this->tdb;
         $dbman = $DB->get_manager();

lib/upgrade.txt

@@ -30,6 +30,7 @@ information provided here is intended especially for developers.
     - mod_quiz
 * The database drivers (moodle_database and subclasses) don't need to implement get_columns() anymore.
   They have to implement fetch_columns instead.
+* Added function cleanup_after_drop to the database_manager class to take care of all the cleanups that need to be done after a table is dropped.
 
 === 3.8 ===
 * Add CLI option to notify all cron tasks to stop: admin/cli/cron.php --stop