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

Idea: foreign() / references() #6

Open
robsontenorio opened this issue Aug 28, 2018 · 6 comments
Open

Idea: foreign() / references() #6

robsontenorio opened this issue Aug 28, 2018 · 6 comments
Labels
enhancement New feature or request
Projects

Comments

@robsontenorio
Copy link
Collaborator

Just like Laravel migrations it would be useful:

<?php

namespace App;

use Deiucanta\Smart\Field;
use Deiucanta\Smart\Model;

class Product extends Model
{
    public function fields()
    {
        return [
            Field::make('id')->increments(),
            Field::make('user_id')
                  ->unsignedInteger()
                  ->references('id')
                  ->on('users');
        ];
    }
}
@deiucanta
Copy link
Owner

@robsontenorio good idea :)

It might be even better to have it in the same method

Field::make('user_id')->unsignedInteger()->references('users.id');
Field::make('user_id')->unsignedInteger()->references('users', 'id');
Field::make('user_id')->unsignedInteger()->references(User::class, 'id');

What about deleting on cascade? Do you use db level deletion or you delete related entities in the app?

@robsontenorio
Copy link
Collaborator Author

I don’t use DB level deletion.

I just suggest the same syntax of Laravel migration just for consistency with well known methods from Laravel.

@deiucanta deiucanta added this to To do in Kanban Aug 28, 2018
@robsontenorio robsontenorio added the enhancement New feature or request label Aug 30, 2018
@robsontenorio
Copy link
Collaborator Author

robsontenorio commented Sep 1, 2018

@deiucanta I think this is a pretty important issue. A must have.

Without this we can't guarantee FK constraints out of box for smart migrations, and would be necessary do it manually editing files (boring).

About syntax i do prefer.

Field::make('user_id')->unsignedInteger()->references('users.id');

I'm not expert on databases. But, on this first version we can consider that FK's will always be unsignedInteger() type (i have never used other type than this). So, this statement can be suppressed by adding it on "field types" by default when using references().

Field::make('user_id')->required()->unsignedInteger()->references('users.id')->label("User");
Field::make('user_id')->required()->references('users.id')->label("User");

Can you handle it?

@deiucanta
Copy link
Owner

The issue here might be if the user id is not unsignedInteger. It can be string, integer (with negative values), unsignedBigInteger, etc. How to handle that?

I created REST APIs in Laravel that used UUIDs for the primary key. I also can imagine someone having big tables and using bigIncrements instead of increments.

@deiucanta
Copy link
Owner

The diff algorithm is just a proof of concept and I'd rather refactor it than patch it to make foreign keys work. Especially for dropping them.

We also didn't touch the requirement of having indexes on multiple fields. That would also impact the diff algorithm, the migration generation, and even the API for field definition.

I'd say we should address all these issues together to get the best outcome.

@robsontenorio
Copy link
Collaborator Author

robsontenorio commented Sep 1, 2018 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Kanban
  
To do
Development

No branches or pull requests

2 participants