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

yii2 migration code style #3335

Closed
Asetss opened this issue May 3, 2014 · 33 comments
Closed

yii2 migration code style #3335

Asetss opened this issue May 3, 2014 · 33 comments
Assignees
Milestone

Comments

@Asetss
Copy link
Contributor

@Asetss Asetss commented May 3, 2014

In yii2 a great migration,on there lame coding style.

$tableOptions = 'CHARACTER SET utf8 COLLATE utf8_general_ci ENGINE=InnoDB';
        // Users table
        $this->createTable('{{%users}}', [
            'id' => Schema::TYPE_PK,
            'username' => Schema::TYPE_STRING . '(30) NOT NULL',  
            'email' => Schema::TYPE_STRING . '(100) NOT NULL',
            'password_hash' => Schema::TYPE_STRING . ' NOT NULL',
            'auth_key' => Schema::TYPE_STRING . '(32) NOT NULL',
            'name' => Schema::TYPE_STRING . '(50) NOT NULL',
            'surname' => Schema::TYPE_STRING . '(50) NOT NULL',
            'avatar_url' => Schema::TYPE_STRING . '(64) NOT NULL',
            'role_id' => 'tinyint NOT NULL DEFAULT 0',
            'status_id' => 'tinyint(4) NOT NULL DEFAULT 0',
            'create_time' => Schema::TYPE_INTEGER . ' NOT NULL',
            'update_time' => Schema::TYPE_INTEGER . ' NOT NULL'
        ], $tableOptions);

And an example of a new style of coding

            $this->createTable('{{%users}}', [
            'id' => Schema::primaryKey(),
            'username' => Schema::string(30)->notNull(),  
            'email' => Schema::string(100)->notNull(),
            'password_hash' => Schema::string(30)->notNull(),
            'auth_key' => Schema::string(32)->notNull(),
            'name' => Schema::string(50)->notNull(),
            'surname' => Schema::string(50)->notNull(),
            'avatar_url' => Schema::string(64)->notNull(),
            'role_id' => 'tinyint NOT NULL DEFAULT 0',      // thought so tinyint()->notNull()->default(0)
            'status_id' => 'tinyint(4) NOT NULL DEFAULT 0', // thought so tinyint(4)->notNull()->default(0)
            'create_time' => Schema::integer()->notNull(),
            'update_time' => Schema::integer()->notNull()
        ], $tableOptions);

Is an example of what I mean. What do you think about this?

@Ragazzo
Copy link
Contributor

@Ragazzo Ragazzo commented May 3, 2014

I think this would be usefull, some time ago i was discussing same idea with @samdark ) However i think that something like L4 or Rails approach here can be used .

@Asetss
Copy link
Contributor Author

@Asetss Asetss commented May 3, 2014

@Ragazzo
Yes there is such a feature L4. I think it is time to implement. Waiting for the opinion of others)

@DrMabuse23
Copy link

@DrMabuse23 DrMabuse23 commented May 3, 2014

@Asetss plz do that would be wonderfull !

@samdark
Copy link
Member

@samdark samdark commented May 3, 2014

Schema::TYPE_PK is basically the same as Schema::primaryKey(). Don't see any benefit. The rest looks like OK idea and technically can be implemented without breaking backwards compatibility.

@bryglen
Copy link

@bryglen bryglen commented May 4, 2014

I like the new approach. 👍 for me

@Asetss
Copy link
Contributor Author

@Asetss Asetss commented May 4, 2014

@samdark this is just an example. integer(intlen), string(strlen) etc,I think you understand.

There such a thing if the other will look like a function and he as a static variable.

@samdark samdark added this to the 2.1 milestone May 4, 2014
@kartik-v
Copy link
Contributor

@kartik-v kartik-v commented May 4, 2014

@Asetss 👍 - would be nice if it is standardized for all column types...

@Asetss
Copy link
Contributor Author

@Asetss Asetss commented May 4, 2014

@kartik-v I think it will be so. Solution for samdark. So we are waiting :]

@samdark
Copy link
Member

@samdark samdark commented May 4, 2014

I don't mind implementing it but since we're now focused very much on docs and bugfixes, it will probably wait for 2.1... or someone will make a pull request and I'll review/merge it.

@Ragazzo
Copy link
Contributor

@Ragazzo Ragazzo commented May 4, 2014

@Asetss if you will be implenting this please note that it should be something like in Rails or L4, so it should not be just a several methods in Schema class like in your example )

@kartik-v
Copy link
Contributor

@kartik-v kartik-v commented May 5, 2014

@Asetss let know ... I can work on a PR jointly if needed... taking some cue from L4..

@Asetss
Copy link
Contributor Author

@Asetss Asetss commented May 5, 2014

@kartik-v I can help, I have the time not so much. We have to see how they are arranged on the L4.

@gonimar
Copy link
Contributor

@gonimar gonimar commented May 6, 2014

Maybe like this:

$this->createTable('{{%users}}', [
    Schema::field('id', Schema::TYPE_PK),
    Schema::field('username', Schema::TYPE_STRING, 100, true),
    Schema::field('status_id', 'tinyint', true, 0),
], $tableOptions);

public static function field($name, $type, $size=null, $notNull=false, $default=false);
@Ragazzo
Copy link
Contributor

@Ragazzo Ragazzo commented May 6, 2014

@gonimar nope, 4 or 5 params to method is wrong, Rails and L4 approach is better and more easy to read. However i dont see any difficulties on how to do this )

@cebe
Copy link
Member

@cebe cebe commented May 6, 2014

Do we really need methods for this? If you do not like writing of constants you can simply write strings which is much more readable imo:

$this->createTable('{{%users}}', [
            'id' => 'pk',
            'username' => 'string(30) NOT NULL',  
            'email' => 'string(100) NOT NULL',
            'password_hash' => 'string NOT NULL',
            'role_id' => 'tinyint NOT NULL DEFAULT 0',
            'status_id' => 'tinyint(4) NOT NULL DEFAULT 0',
        ], $tableOptions);

What are the benefits of a new abstraction syntax?

@gonimar
Copy link
Contributor

@gonimar gonimar commented May 6, 2014

Support different databases.

@cebe
Copy link
Member

@cebe cebe commented May 6, 2014

The type names are already being translated dependend on the database. Can you give an example of where 'NOT NULL' or 'DEFAULT ...' is different in different dbms?

@gonimar
Copy link
Contributor

@gonimar gonimar commented May 6, 2014

Unlikely

@cebe
Copy link
Member

@cebe cebe commented May 6, 2014

so again, why do we need this feature then? :)

@samdark
Copy link
Member

@samdark samdark commented May 6, 2014

@cebe the benefit is less concatenation and better autocomplete. Also it's possible to improve it further like the following:

$this->createTable('{{%auth_item}}', [
    'name' => Schema::string(64)->notNull(),
    'type' => Schema::integer()->notNull()->default(10),
    'description' => Schema::text(),
    'rule_name' => Schema::string(64),
    'data' => Schema::text(),
    'created_at' => Schema::integer(),
    'updated_at' => Schema::integer(),
])
  ->primaryKey('pk-auth_item', 'name')
  ->foreignKey('fk-auth_item-rule_name', 'rule_name', '{{%auth_rule}}', 'name', 'SET NULL', 'CASCADE')
  ->index('idx-auth_item-type', 'type');
@kartik-v
Copy link
Contributor

@kartik-v kartik-v commented May 6, 2014

This is more probably an aid to developer (as @samdark's code example points out) to reduce the typos when creating the migration code which can result when typing raw strings or concatenating constants with text. This will not be known until the migration is tested/run. Some of the common errors could be:

  • Missing spaces when concatenating
  • Missing a bracket in the string concatenation
  • Generic spelling typos in writing fixed strings (like string, not null etc.)

Using a function approach like above with an IDE... may help draft the column structure faster using autocomplete and with less coding typos IMO - since the errors may be known immediately through the IDE when coding itself.

Of course as @cebe pointed out there are similarities to some extent... both of the above achieve the same thing in different ways - I feel this latter option just helps the developer minimize coding errors.

@Asetss
Copy link
Contributor Author

@Asetss Asetss commented May 6, 2014

@samdark I like. Only scares line

->primaryKey('pk-auth_item', 'name')
 ->foreignKey('fk-auth_item-rule_name', 'rule_name', '{{%auth_rule}}', 'name', 'SET NULL', 'CASCADE')
  ->index('idx-auth_item-type', 'type');

foreignKey type so,cascade etc

->foreign('user_id')->cascade();

index

->index('type');

I do not understand your last line of code

@samdark
Copy link
Member

@samdark samdark commented May 6, 2014

@Asetss can't be done since we're not fixing the order in which the chain parts are specified.

@Ragazzo
Copy link
Contributor

@Ragazzo Ragazzo commented May 6, 2014

Also one thing should be noted is that we should avoid usage of direct echo in migration classes, since they can be used in other parts of code . This situation should be resolved by decorator or some other way .

@Asetss
Copy link
Contributor Author

@Asetss Asetss commented Jun 23, 2014

@samdark how are things? I think it is time to do this. Migration code looks awful ...

@samdark
Copy link
Member

@samdark samdark commented Jun 23, 2014

It's not showstopper so I'll come back to it after I'll deal with all the things for RC. Feel free to work on it if you want it faster.

@deerawan
Copy link
Contributor

@deerawan deerawan commented Jun 27, 2014

Would love to see it become real. It is really much easier to code and read.

@Ragazzo
Copy link
Contributor

@Ragazzo Ragazzo commented Jul 1, 2014

Some highlight in migration output will also be useful , just a hint

@rubenheymans
Copy link

@rubenheymans rubenheymans commented Dec 4, 2014

is it also possible to generate migration code automatically?
that would save a lot of time

@samdark samdark modified the milestones: 2.0.2, 2.1.x Dec 27, 2014
@samdark samdark modified the milestones: 2.0.3, 2.0.2 Jan 5, 2015
@ifocus22
Copy link

@ifocus22 ifocus22 commented Jan 6, 2015

I found this to be useful with Rails.

$ bin/rails generate scaffold HighScore game:string score:integer
invoke active_record
create db/migrate/20130717151933_create_high_scores.rb

$ bin/rails generate model
Usage: rails generate model NAME [field[:type][:index] field[:type][:index]] [options]

$ bin/rake db:migrate
== CreateHighScores: migrating
-- create_table(:high_scores)
-> 0.0017s
== CreateHighScores: migrated (0.0019s)

Source : http://guides.rubyonrails.org/command_line.html#rails-generate

@samdark
Copy link
Member

@samdark samdark commented Jan 7, 2015

Yes, that's cool but the issue is not about it. If you want such syntax to be supported by migrate command, create additional issue.

@samdark samdark modified the milestones: 2.0.x, 2.0.3 Jan 20, 2015
@pana1990
Copy link
Contributor

@pana1990 pana1990 commented Mar 6, 2015

Hi,

I'm trying to implement this issue, that helpers should be implemented,

  • Schema::primaryKey();
  • Schema::bigPrimaryKey();
  • Schema::string();
  • Schema::text();
  • Schema::smalIInt();
  • Schema::integer();
  • Schema::bigInt();
  • Schema::float();
  • Schema::decimal();
  • Schema::dateTime();
  • Schema::timeStamp();
  • Schema::time();
  • Schema::date();
  • Schema::binary();
  • Schema::boolean();
  • Schema::money();

Another helpers? it should be support helpers especific dbms (e.g. Schema::enum(['Monday', 'Thursday']) for mysql)?¿

any idea or tip?

@samdark
Copy link
Member

@samdark samdark commented Mar 6, 2015

For now only common types should be implemented.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
You can’t perform that action at this time.