communityii / yii2-user

Configurable Yii 2 user management module with social authentication and various controls
https://groups.google.com/d/forum/communityii
Other
50 stars 9 forks source link

Do not rely on the user module in migrations #6

Closed schmunk42 closed 10 years ago

schmunk42 commented 10 years ago

If we decide to change the table names later the migrations will break. It is also nicer not to use the user module in console config.

robregonm commented 10 years ago

We can use table prefixes AND custom names (with default names), like I suggested in #4. So, the developer can choose another name than default in different scenarios. In my personal case, two scenarios:

  1. One existing app which I want migrate to use this extension. If the table name is changed, that will break some code and queries.
  2. Some DBs needs to coexist in the same DB (and sometimes to use each other), so, (in my case) I prefer to use a spanish name (or some custom name) for this particular case in order to have different user tables for different applications.

In the case of the old yii-user extension, I stopped to use it when it started to experience those odd bugs. Another guy and me developed another user extension (which also uses custom table names) and worked perfectly because the user can decide whether to use custom names.

schmunk42 commented 10 years ago

What would we do, if we decide to rename a table? Option A: Update the constant, which will make migrations fail on several update scenarios. Option B: Introduce a new constant and update all the extension code and other parts of the app, which may use it.

Btw: If we introduce constants for tables, I'd name them more verbose, nobody knows what T4 or T3 actually is. Which would lead to USER2 or something similar for Option B.

Totally unviable solutions IMHO. Maybe it's about personal coding style, but I think you can't translate or make changes on that level, unless you're willing to do a lot of work. You also don't translate the PHP Classes and functions, do you?

But let me also propose alternatives:

  1. One existing app which I want migrate to use this extension. If the table name is changed, that will break some code and queries.

Use a database-view.

  1. Some DBs needs to coexist in the same DB (and sometimes to use each other), so, (in my case) I prefer to use a spanish name (or some custom name) for this particular case in order to have different user tables for different applications.

Use a different prefix (db connection) or also a view.

kartik-v commented 10 years ago

I am ok with any approach on this. I had set this to use constant table-names earlier - and then used @robregonm suggestion to parametrize table names in the Module. I am also ok with this as it allows developer to set the names. The only thing in the second case, is module needs to be setup in yii config, before migration is run.

Personally speaking (for self) I would very very rarely change DATABASE table names for a production ready module extension - once I have set it up initially and configured as per my design. So any of the above approach personally would not have a problem for someone like me, once I have set it up initially. But its upto developers. IMO, if one is changing table names quite often - he/she would end up re-creating his/her own module - rather than actually extending from what is available.

I can understand columns/indices getting added or dropped or changed - but changing table names during course of development for me is very rare.

Let's agree on one approach (I am ok for any) and its more from your experience on what you feel is needed by most developers.

kartik-v commented 10 years ago

@schmunk42: Btw: If we introduce constants for tables, I'd name them more verbose, nobody knows what T4 or T3 actually is. Which would lead to USER2 or something similar for Option B.

I had setup this for table constants. Do we still need this to be different?

nineinchnick commented 10 years ago

I keep it simple. I just advice users to copy the migrations to their project's migration dir and adjust timestamps in names to match their timeline.

Actually, I will forever advocate not forcing users to specific structures, just providing full ready to use examples that can and should be adjusted.

schmunk42 commented 10 years ago

I am sorry to say that, but parametrizing table names like this is broken by design. I also don't want to change table names, but that's what migrations are also for - and it happens.

It's really complicated to actually find the table name for a model, because you first have to look up tableName() in the model which is stored in a constant in the module.

Let me give you an example:

Let's assume all three releases have migrations. If you're on 0.1 and did NOT update to 0.2, but now running an update to 0.3 the migration from 0.2 will fail because it will use the table name you have defined in your source code in version 0.3, which doesn't exist in 0.2.

If we'd used static prefixes in the first place, we'd have exactly this problem now.

See also https://github.com/yiisoft/yii2/blob/master/docs/guide/extensions.md#working-with-database - it's essentially the same as "Do not use Active Record models in your migrations.".

Let's keep it simple.

kartik-v commented 10 years ago

@schmunk42 I agree with all the points from all of you which are valid (only that I have rarely experienced changing of table names during course of design) .

So to summarize how should we simplify it ... i..e what is the code to be included in the migrations

Based on this, would make the changes.

nineinchnick commented 10 years ago

Why would you need a constant with the table name? Can't this be read from the User model?

kartik-v commented 10 years ago

Ok to ask again.. this kind of violates point # 3 of using ActiveRecord models mentioned before.

But even if we use User::tableName() ... what should this function's return value be? Do we set it to '{{%user}}' or Module::TBL_USER.

I am asking this question again only because the topic is around changing of table names during the development process. Should developers change table names across all models or extend the module and change the constants.

PS: If you ask me... I am ok with any option - because I personally may not be needing to change table names - once its set initially.

Some great inputs here. This can be closed based on your experiences guys.

schmunk42 commented 10 years ago

We set is as {{%user}} so you can still choose a prefix if you've conflicts with existing tables. But we have no variables or constants involved in our migrations.

@nineinchnick No, it can't. See the link to the Yii 2 extension guidelines.

nineinchnick commented 10 years ago

@schmunk42 I agree, but I'd only use the constant in the migrations and User::tableName(). In every other place the table name should be read from the model.

kartik-v commented 10 years ago

Ok if I understand from you guys... here is the summary then. For a table named '{{%user}}' - these are the places where this will be hard-coded... so any change for every table name needs to be done in the first 2 places below:

  1. In migration script - the table will be named '{{%user}}'
  2. In User model - the tableName() function will return '{{%user}}'
  3. Any other place - we get the tableName as User::tableName()

Are we ok with this?

So for folks who will change table names frequently during development... as an example... if there are 10 tables ... then developers need to change the name in at least 20 places (migration and models). This is for every instance they need to change the table names.

schmunk42 commented 10 years ago

@kartik-v Yes, I am OK with 1., 2. and 3.

@nineinchnick Do not use a constant in the migrations, it just makes no sense, complicates things and has more disadvantages. As mentioned beforehand, there are some things you can not change or define dynamically, such as ClassNames or functions().

kartik-v commented 10 years ago

Closed via commit 70df716.

schmunk42 commented 10 years ago

Nice!

robregonm commented 10 years ago

Looks great!