[ticket/17507] Fix column removal issues

PHPBB-17507

phpBB/phpbb/db/migrator.php

@@ -575,7 +575,7 @@ class migrator
 				$state['migration_data_state']['_total_time'] : 0.0;
 			$elapsed_time = microtime(true);
 
-			$steps = array_merge($this->helper->reverse_update_data($migration->update_data()), $migration->revert_data());
+			$steps = $migration->revert_data() ?: $this->helper->reverse_update_data($migration->update_data());
 			$result = $this->process_data_step($steps, $state['migration_data_state']);
 
 			$elapsed_time = microtime(true) - $elapsed_time;

phpBB/phpbb/db/tools/doctrine.php

@@ -300,34 +300,10 @@ class doctrine implements tools_interface
 	 */
 	public function sql_column_remove(string $table_name, string $column_name)
 	{
-		// Check if this column is part of a primary key. If yes, remove the primary key.
+		$return = $this->schema_drop_primary_key($table_name, $column_name);
-		$primary_key_indexes = $this->get_filtered_index_list($table_name, false);
+		if ($return !== true)
-
-		$primary_key_indexes = array_filter($primary_key_indexes, function($index) use ($column_name) {
-			$index_columns = array_map('strtolower', $index->getUnquotedColumns());
-			return in_array($column_name, $index_columns, true) && $index->isPrimary();
-		});
-
-		if (count($primary_key_indexes))
 		{
-			// For PostgreSQL, drop primary index first to avoid "Dependent objects still exist" error
+			return $return;
-			if (stripos($this->get_schema_manager()->getDatabasePlatform()->getname(), 'postgresql') !== false)
-			{
-				$this->get_schema_manager()->dropIndex('"primary"', $table_name);
-			}
-
-			$ret = $this->alter_schema(
-				function (Schema $schema) use ($table_name, $column_name): void
-				{
-					$table = $schema->getTable($table_name);
-					$table->dropPrimaryKey();
-				}
-			);
-
-			if ($ret !== true)
-			{
-				return $ret;
-			}
 		}
 
 		return $this->alter_schema(
@@ -835,6 +811,8 @@ class doctrine implements tools_interface
 	 */
 	protected function schema_column_remove(Schema $schema, string $table_name, string $column_name, bool $safe_check = false): void
 	{
+		$this->schema_drop_primary_key($table_name, $column_name);
+
 		$this->alter_table(
 			$schema,
 			$table_name,
@@ -1005,6 +983,47 @@ class doctrine implements tools_interface
 		$table->setPrimaryKey($columns);
 	}
 
+	/**
+	 * Removes primary key from a table
+	 *
+	 * @param string $table_name
+	 * @param string $column_name
+	 *
+	 * @return bool|string[]
+	 *
+	 * @throws SchemaException
+	 */
+	protected function schema_drop_primary_key(string $table_name, string $column_name): array|bool
+	{
+		// Check if this column is part of a primary key. If yes, remove the primary key.
+		$primary_key_indexes = $this->get_filtered_index_list($table_name, false);
+
+		$primary_key_indexes = array_filter($primary_key_indexes, function($index) use ($column_name) {
+			$index_columns = array_map('strtolower', $index->getUnquotedColumns());
+			return in_array($column_name, $index_columns, true) && $index->isPrimary();
+		});
+
+		$return = true;
+		if (count($primary_key_indexes))
+		{
+			// For PostgreSQL, drop primary index first to avoid "Dependent objects still exist" error
+			if (stripos($this->get_schema_manager()->getDatabasePlatform()->getname(), 'postgresql') !== false)
+			{
+				$this->get_schema_manager()->dropIndex('"primary"', $table_name);
+			}
+
+			$return = $this->alter_schema(
+				function (Schema $schema) use ($table_name, $column_name): void
+				{
+					$table = $schema->getTable($table_name);
+					$table->dropPrimaryKey();
+				}
+			);
+		}
+
+		return $return;
+	}
+
 	/**
 	 * Recreate an index of a table
 	 *
@@ -1016,40 +1035,43 @@ class doctrine implements tools_interface
 	 */
 	protected function recreate_index(Table $table, Index $index, array $new_columns): void
 	{
-		if ($index->isPrimary())
+		if (!empty($new_columns))
 		{
-			$table->dropPrimaryKey();
+			if ($table->hasPrimaryKey() && $index->isPrimary())
-		}
+			{
-		else
+				$table->dropPrimaryKey();
-		{
+			}
-			$table->dropIndex($index->getName());
+			else if ($table->hasIndex($index->getName()))
-		}
+			{
+				$table->dropIndex($index->getName());
+			}
 
-		if (count($new_columns) > 0)
+			if (count($new_columns) > 0)
-		{
-			if ($index->isPrimary())
 			{
-				$table->setPrimaryKey(
+				if ($index->isPrimary())
-					$new_columns,
+				{
-					$index->getName(),
+					$table->setPrimaryKey(
-				);
+						$new_columns,
-			}
+						$index->getName(),
-			else if ($index->isUnique())
+					);
-			{
+				}
-				$table->addUniqueIndex(
+				else if ($index->isUnique())
-					$new_columns,
+				{
-					$index->getName(),
+					$table->addUniqueIndex(
-					$index->getOptions(),
+						$new_columns,
-				);
+						$index->getName(),
-			}
+						$index->getOptions(),
-			else
+					);
-			{
+				}
-				$table->addIndex(
+				else
-					$new_columns,
+				{
-					$index->getName(),
+					$table->addIndex(
-					$index->getFlags(),
+						$new_columns,
-					$index->getOptions(),
+						$index->getName(),
-				);
+						$index->getFlags(),
+						$index->getOptions(),
+					);
+				}
 			}
 		}
 	}

tests/migrations/migration_test_base.php

@@ -134,7 +134,7 @@ abstract class phpbb_migration_test_base extends phpbb_database_test_case
 
 	protected function revert_migration()
 	{
-		while ($this->migrator->migration_state($this->migration_class))
+		while ($this->migrator->migration_state($this->migration_class) !== false)
 		{
 			try
 			{

tests/migrations/remove_jabber_migration_test.php

@@ -69,5 +69,8 @@ class phpbb_migrations_remove_jabber_migration_test extends phpbb_migration_test
 		$this->assertTrue($this->tools['permission']->exists('a_jabber'));
 		$this->assertTrue($this->tools['permission']->exists('u_sendim'));
 		$this->assertTrue($this->tools['module']->exists('acp', 'ACP_CLIENT_COMMUNICATION', 'ACP_JABBER_SETTINGS'));
+
+		// Apply migration back
+		$this->apply_migration();
 	}
 }