Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[10.x] Allow specifying index name when calling ForeignIdColumnDefinition@constrained() #46746

Merged

Conversation

cosmastech
Copy link
Contributor

@cosmastech cosmastech commented Apr 11, 2023

Problem

I prefer verbose naming. I have a migration that looks like this:

use App\Models\KnowledgeBaseResource;
use Illuminate\Database\Migrations\Migration;
use Illuminate\Database\Schema\Blueprint;
use Illuminate\Support\Facades\Schema;


return new class extends Migration
{
    public function up(): void
    {
        Schema::create('company_knowledge_base_resource_matches', function (Blueprint $table) {
            $table->id();
            $table->foreignIdFor(KnowledgeBaseResource::class)->nullable()->constrained()->nullOnDelete();
            $table->timestamps();
        });
    }
}

Which blows up:

SQLSTATE[42000]: Syntax error or access violation: 1059 Identifier name 'company_knowledge_base_resource_matches_knowledge_resource_id_foreign' is too long (Connection: mysql, SQL: alter table `company_knowledge_base_resource_matches` add constraint `company_knowledge_base_resource_matches_knowledge_resource_id_foreign` foreign key (`knowledge_base_resource_id`) references `knowledge_base_resources` (`id`) on delete set null)

This can be worked around, but doesn't allow the developer to use the nice features of the schema builder, puts a lot more of a lift on developers, and leaves more room for error.

Schema::create('company_knowledge_base_resource_matches', function (Blueprint $table) {
    $table->id();
    $table->foreignIdFor(KnowledgeBaseResource::class)->nullable();
    $table->timestamps();
    
    $table->foreign('knowledge_base_resource_id', 'my_foreign_index')->references('id')->on('knowledge_base_resources')->constrained()->nullOnDelete();
});

Solution

Allow calling code to specify the index name within the call to ForeignIdColumnDefinition@constrained().

This should not break backwards compatibility.

I also changed the second parameter of constrained() so that it allows null to be passed, which will coalesce to 'id'. I think this improves developer experience, so, a developer might write this instead:

Schema::create('company_knowledge_base_resource_matches', function (Blueprint $table) {
    $table->id();
    $table->foreignIdFor(KnowledgeBaseResource::class)
        ->nullable()
        ->constrained(null, null, 'my_foreign_index')
        ->nullOnDelete();
    $table->timestamps();
});

instead of: $table->foreignIdFor(SomeModel::class)->constrained(null, 'id', 'my_foreign_index')

@cosmastech cosmastech changed the title [10.x] Allow specifying constrained index [10.x] Allow specifying index name when calling ForeignIdColumnDefinition@constrained() Apr 11, 2023
@taylorotwell taylorotwell merged commit ef77d87 into laravel:10.x Apr 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants