migration rollback: $table->dropForeignIdFor($model, $column) attempts (and fails) to drop wrong column name

[silas-prototype]

• Laravel Version: Laravel Framework 9.23.0
• PHP Version: 8.1.9 (cli)
• Database Driver & Version: 8.0.29 MySQL Community Server - GPL

Description:

$table->dropForeignIdFor($model, $column) in migration attempts (and obviously fails) to drop wrong column name even though the column name (second parameter) is provided.

The issue seems to be with laravel extending the column name on its own going against the behaviour that I would expect.
Namely just dropping the provided column name if a column name is provided (and if not a column that follows laravel naming conventions for the referenced model)

And laravel only produces this unexpected behaviour with the dropForeingIdFor method. The analogue foreignIdFor method with the same arguments creates a column with the correct (provided) column name without adding anything to it.

The issue should be neither with php nor the database.

Steps To Reproduce:

0. Set up php, Laravel with Sail (including mysql), the db-connection etc.
1. Add foreignId in the up() method of a migration using the foreignIdFor($model, $column) method and provide both the model as well as the column name like:

public function up()
{
Schema::table('users', function (Blueprint $table) {
$table->foreignIdFor(UserGlobalRole::class, 'global_role_id');
});
}

2. Add the corresponding dropForeignIdFor($model, $column) to the down method of the same migration with the same model and column as in the up-method like:

public function down()
{
Schema::table('users', function (Blueprint $table) {
$table->dropForeignIdFor(UserGlobalRole::class, 'global_role_id');
});
}

3. run migration in console (I simply used: sail artisan migrate)

• replacing sail with php should have the same effect
• if db-connection is correctly set up database shows the column was correctly created under the give column-name 'global_role_id'

5. roll back last migration in console (I simply used: sail artisan migrate:rollback)

• again: replacing sail with php should have the same effect if the db connection is set up
• attempts to drop column 'users_global_role_id_foreign' even tho I handed over the column name 'global_role_id' and it throws error in command line like:

Illuminate\Database\QueryException

SQLSTATE[42000]: Syntax error or access violation: 1091 Can't DROP 'users_global_role_id_foreign'; check that column/key exists (SQL: alter table users drop foreign key users_global_role_id_foreign)

at vendor/laravel/framework/src/Illuminate/Database/Connection.php:759

• changing the column name in step 2 or the table name (to another already existing table) results in the same error but attempting to drop a changed column following the pattern:
• {table}_{column}_foreign while it should just be column
• if no column is provided (or null), column will simply default to laravel naming conventions but still following the same pattern:
  -> {table}_{model_name}_id_foreign while this as well should only be {model_name}_id

[driesvints]

Heya, thanks for reporting.

We'll need more info and/or code to debug this further. Can you please create a repository with the command below, commit the code that reproduces the issue as separate commits on the main/master branch and share the repository here? Please make sure that you have the latest version of the Laravel installer in order to run this command. Please also make sure you have both Git & the GitHub CLI tool properly set up.

laravel new bug-report --github="--public"

Please do not amend and create a separate commit with your custom changes. After you've posted the repository, we'll try to reproduce the issue.

Thanks!

[ollieread]

@driesvints Out of curiosity I went and had a nose at this, and I believe I have discovered the issue. The dropForeignIdFor calls dropForeign, but passes the column as an array.

framework/src/Illuminate/Database/Schema/Blueprint.php

Line 429 in 9da642c

return $this->dropForeign([$column ?: $model->getForeignKey()]);

Then, dropForeign calls dropIndexCommand, which has a specific clause for arrays.

framework/src/Illuminate/Database/Schema/Blueprint.php

Lines 1559 to 1561 in 9da642c

	if (is_array($index)) {
	$index = $this->createIndexName($type, $columns = $index);
	}

Then, createIndexName will create what it thinks the index name should be based on the columns provided.

I would hazard that this is either a mistake or something that isn't clearly documented; either way, I'm confident this is what's happening to @silas-prototype. I even checked it in 9.23.

[driesvints]

Yes this indeed seems like an issue. But I think we won't be able to change this on a minor/patch release without breaking apps. I think the correct line there should be:

return $this->dropForeign($column ?: [$model->getForeignKey()]);

Otherwise you can't provide a literal column like you're trying to do. The type should then be string|array|null.

We'd welcome a PR to master to change this.

[driesvints]

Here's the original PR btw: #40950

[ollieread]

We'd welcome a PR to master to change this.

I'll give that a go this afternoon if no one else picks it up.

[ollieread]

Digging into this @driesvints, I suspect there's a bit more to it that we need to look into. Want me to create a new issue for a discussion about it?

[driesvints]

What's up?

[ollieread]

I think it's all just very unclear @driesvints. There isn't a bug, the method dropForeignIdFor only drops the index, but no index is created unless you call references() or constrained() on the foreignIdFor() definition.

I think @silas-prototype has made the very easy to make mistake of assuming that dropForeignIdFor is the counterpart for foreignIdFor.

[driesvints]

Right. In that case we'd appreciate a PR to the docs if someone wants to.