Fixes #2726: ActiveRecord now fills default values on creating new instance of the model if defaults are available from DB schema #2815

Merged
merged 3 commits into from Mar 19, 2014

Conversation

Projects
None yet
3 participants
@samdark
Member

samdark commented Mar 19, 2014

Pros of this approach:

  • No need to manually fill values.
  • Defaults could be changed w/o regenerating model code.
  • Consistent with how DB works.
Fixes #2726: ActiveRecord now fills default values on creating new i…
…nstance of the model if defaults are available from DB schema
@samdark

This comment has been minimized.

Show comment
Hide comment
Member

samdark commented Mar 19, 2014

@qiangxue

This comment has been minimized.

Show comment
Hide comment
@qiangxue

qiangxue Mar 19, 2014

Member

This is not correct because it may overwrite the values that are set via constructor.

I suggest we introduce a method loadDefaultValues(). Users should explicitly call this method if they want to respect default values. The usage is mainly in inserting a new record.

Member

qiangxue commented Mar 19, 2014

This is not correct because it may overwrite the values that are set via constructor.

I suggest we introduce a method loadDefaultValues(). Users should explicitly call this method if they want to respect default values. The usage is mainly in inserting a new record.

@qiangxue

This comment has been minimized.

Show comment
Hide comment
@qiangxue

qiangxue Mar 19, 2014

Member

Two more issues:

  • If a user explicitly specifies some columns to return in a query (e.g. in REST API), he may find in a surprise that he gets default values in unwanted columns (and even worse is that the values could be incorrect.)
  • Performance issue: affect the performance of querying with many records
Member

qiangxue commented Mar 19, 2014

Two more issues:

  • If a user explicitly specifies some columns to return in a query (e.g. in REST API), he may find in a surprise that he gets default values in unwanted columns (and even worse is that the values could be incorrect.)
  • Performance issue: affect the performance of querying with many records
@samdark

This comment has been minimized.

Show comment
Hide comment
@samdark

samdark Mar 19, 2014

Member

Are we actually setting any values via constructor?

Member

samdark commented Mar 19, 2014

Are we actually setting any values via constructor?

@qiangxue

This comment has been minimized.

Show comment
Hide comment
@qiangxue

qiangxue Mar 19, 2014

Member

The framework doesn't, but it's totally valid to do $user = new User(['username' => 'test']);

Member

qiangxue commented Mar 19, 2014

The framework doesn't, but it's totally valid to do $user = new User(['username' => 'test']);

@samdark

This comment has been minimized.

Show comment
Hide comment
@samdark

samdark Mar 19, 2014

Member

In a rest API it will get real values from data source, not default ones.

Member

samdark commented Mar 19, 2014

In a rest API it will get real values from data source, not default ones.

@qiangxue

This comment has been minimized.

Show comment
Hide comment
@qiangxue

qiangxue Mar 19, 2014

Member

In a rest API it will get real values from data source, not default ones.

Not the case with this PR. Imagine I want to return a user list with only id and username in the result. With this PR, other irrelevant fields will be filled with default values, which is both unexpected (unexpected fields) and incorrect (the values may be incorrect).

In summary, setting default values is only useful when inserting new records. And even in this case, some users may not like prefilling with default values.

Member

qiangxue commented Mar 19, 2014

In a rest API it will get real values from data source, not default ones.

Not the case with this PR. Imagine I want to return a user list with only id and username in the result. With this PR, other irrelevant fields will be filled with default values, which is both unexpected (unexpected fields) and incorrect (the values may be incorrect).

In summary, setting default values is only useful when inserting new records. And even in this case, some users may not like prefilling with default values.

@samdark

This comment has been minimized.

Show comment
Hide comment
@samdark

samdark Mar 19, 2014

Member

Makes sense. Changed it to be separate method so the syntax is:

$model = new MyModel();
$model->loadDefaultValues()->save();
Member

samdark commented Mar 19, 2014

Makes sense. Changed it to be separate method so the syntax is:

$model = new MyModel();
$model->loadDefaultValues()->save();
@qiangxue

This comment has been minimized.

Show comment
Hide comment
@qiangxue

qiangxue Mar 19, 2014

Member

Looks good to me now. Potential future enhancement is to allow loading default values for specific attributes. Probably not needed for now.

Member

qiangxue commented Mar 19, 2014

Looks good to me now. Potential future enhancement is to allow loading default values for specific attributes. Probably not needed for now.

samdark added a commit that referenced this pull request Mar 19, 2014

Merge pull request #2815 from yiisoft/ar-default-values
Fixes #2726: ActiveRecord now fills default values on creating new instance of the model if defaults are available from DB schema

@samdark samdark merged commit 922c5da into master Mar 19, 2014

@samdark samdark deleted the ar-default-values branch Mar 19, 2014

@Sammaye

This comment has been minimized.

Show comment
Hide comment
@Sammaye

Sammaye Mar 19, 2014

If this is run by a function then say I was to do as @qiangxue says with: $user = new User(['username' => 'test']); how would I then apply defaults without potentially removing that custom value?

Sammaye commented Mar 19, 2014

If this is run by a function then say I was to do as @qiangxue says with: $user = new User(['username' => 'test']); how would I then apply defaults without potentially removing that custom value?

@samdark

This comment has been minimized.

Show comment
Hide comment
@samdark

samdark Mar 19, 2014

Member

You can't. Use individual assignments after calling loadDefaultValues() instead.

Member

samdark commented Mar 19, 2014

You can't. Use individual assignments after calling loadDefaultValues() instead.

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