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

Not a bug, but a proposal for an upgrade/feature on migrations #13

Open
awesomeguy3 opened this issue Aug 9, 2017 · 17 comments
Open

Not a bug, but a proposal for an upgrade/feature on migrations #13

awesomeguy3 opened this issue Aug 9, 2017 · 17 comments
Labels
type:enhancement Enhancement

Comments

@awesomeguy3
Copy link

As I said in the title, this is not a bug, but a proposal (didn't know where to write this that would be seen.

Now, on my project I have a big database (not in terms of data but in terms of tables and columns in those tables), and I also have a lot of indexes.
I noticed today that I need to write in a migration class
$this->createIndex('name', 'table', ['columns'])
after creating a new table, what I'm thinking should be a good idea is to add a possibility for method chaining on "createTable" function where I wouldn't need to enter table name for every index.
Or perhaps another parameter in createTable function that would easily let me add indexes in one place.

@samdark
Copy link
Member

samdark commented Aug 9, 2017

How would the syntax look like?

@awesomeguy3
Copy link
Author

awesomeguy3 commented Aug 9, 2017

Maybe something like

$this->createTable('tableName', [
    'id' => $this->primaryKey(),
    'column' => $this->string()
], 'some options', [
    'index1' => ['column1', 'column2'],
    'index2' => ['column3']
]);

OR

$this->createTable('tableName', [
    'id' => $this->primaryKey(),
    'column' => $this->string()
], 'some options')->index('index1', ['column1', 'column2'])->index('index2', 'column3');

And forgive me for not knowing how to format code better in Markdown.

@samdark
Copy link
Member

samdark commented Aug 9, 2017

Formatting is easy: https://guides.github.com/features/mastering-markdown/

@samdark
Copy link
Member

samdark commented Aug 9, 2017

Second options looks interesting...

@awesomeguy3
Copy link
Author

I personally like first option better, but whatever you make is okay with me.

@kjusupov
Copy link

$this->createTable('tableName', 
        [
            'id' => $this->primaryKey(),
            'column' => $this->string()
        ]
)->createIndex(
        ['index_one', 'column1'],
        ['index_two', ['column1', 'column2']],
        ['index_three', ['column1', 'column2']],
)->createForeignKey(
        ['FK_name_one', 'column1', 'table1', 'column'],
        ['FK_name_two', 'column2', 'table2', 'column']
);

@Sarke
Copy link

Sarke commented Aug 13, 2017

How about something like this?

$this->createTable('tableName', [
    'id' => $this->primaryKey(),
    'column' => $this->string()->index(),
]);

Not sure if that would work with ColumnSchemaBuilder, and if not maybe something like this?

$this->createTable('tableName', [
    'id' => $this->primaryKey(),
    'column' => [$this->string(), 'index' => true],
]);

If it's just true then create an index for that one column with that name (I think this is the most common). otherwise a string with the name of the index.

A solution like @kjusupov would be required for more complex indices, but sounds like it would require something like a TableSchemaBuilder perhaps?

@rob006
Copy link

rob006 commented Aug 13, 2017

I'm for

$this->createTable('tableName', [
    'id' => $this->primaryKey(),
    'name' => $this->string()->index('indexName'),
    'topic_id' => $this->integer()->foreignKey('indexName', 'referencedTable', 'referencedColumn'),
]);

@awesomeguy3
Copy link
Author

Great ideas, but what about complex indexes?

@rob006
Copy link

rob006 commented Aug 13, 2017

We already have createIndex() for complex indexes.

@michaelarnauts
Copy link

I'm in favor for chaining like @kjusupov exampled. This fits with the current way of constructing the migrations.

@rob006
Copy link

rob006 commented Aug 18, 2017

I'm in favor for chaining like @kjusupov exampled. This fits with the current way of constructing the migrations.

This syntax does not give you any real benefits, it creates only another way to do the same thig (which usually decrease readability).

@awesomeguy3
Copy link
Author

I'm with @rob006 on this one, his example is quite good.

@santilin
Copy link

The syntax proposed by @rob006 allows sqlite3 to create foreign keys at the same time that the table, which is the only way to add a foreign key in sqlite3 so far, although I am working on adding that feature to yii2/sqlite3: #15007

@omzy
Copy link

omzy commented Oct 19, 2018

@kjusupov

$this->createTable('tableName', 
        [
            'id' => $this->primaryKey(),
            'column' => $this->string()
        ]
)->createIndex(
        ['index_one', 'column1'],
        ['index_two', ['column1', 'column2']],
        ['index_three', ['column1', 'column2']],
)->createForeignKey(
        ['FK_name_one', 'column1', 'table1', 'column'],
        ['FK_name_two', 'column2', 'table2', 'column']
);

The only issue I have with this approach is that you'd need to remember the order of parameters for createForeignKey() etc (unless you use an IDE that can tell you the parameters).

A better approach I think would be to make it mandatory to include the keys in the array, e.g:

$this->createForeignKey([
    'column' => 'user_id',
    'references' => 'id',
    'on' => 'user',
    'onDelete' => 'cascade',
]);

Note in this example, I didn't include the FK name - this should be optional and auto-generated (based on column and table name) if it isn't included.

@rob006
Copy link

rob006 commented Oct 19, 2018

A better approach I think would be to make it mandatory to include the keys in the array, e.g:

Now you need to remember keys names, and IDE will not help you here...

@cebe
Copy link
Member

cebe commented Oct 25, 2018

You can already add plain string lines like this to add an index:

$this->createTable('tableName', [
    'id' => $this->primaryKey(),
    'name' => $this->string(),
    'topic_id' => $this->integer(),
    'KEY topic_id(topic_id)',
    'FOREIGN KEY ... REFERENCES ...',
]);

So we can have a schemabuilder class that would build those strings in DBMS specific syntax.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:enhancement Enhancement
Projects
None yet
Development

No branches or pull requests

10 participants