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

Separate “attributes” and “DB attributes” for ActiveRecord #1967

Closed
klimov-paul opened this Issue Jan 14, 2014 · 49 comments

Comments

Projects
None yet
@klimov-paul
Member

klimov-paul commented Jan 14, 2014

Remember issue yiisoft/yii#627
At the moment Yii2 does not achieve much in solving it.

I assume “Model::attributes” specify the list of properties, which can be populated via “Model::load()” and validated via “Model::validate()”.

I suppose taking database table fields as a set of “Model::attributes” is a limitation, which does not allow you to add some attributes, which may require validation, but should not be saved in database.

I suppose for the ActiveRecord there should be “dbAttributes” in addition to regular “attributes”. By default “ActiveRecord::attributes()” will return result of “ActiveRecord::dbAttributes()” but can be overridden if needed.

@ghost ghost assigned qiangxue Jan 14, 2014

@nineinchnick

This comment has been minimized.

Show comment
Hide comment
@nineinchnick

nineinchnick Jan 14, 2014

Contributor

Could this also allow custom mapping of db column names to model attribute names?

I've got a use case for this where I have to use a legacy db schema and can introduce proper model naming but use old table names. I'd like to do the same with column names without creating views.

I guess the drawback is confusion when referring to attribute names in plain SQL expressions in query criteria.

Contributor

nineinchnick commented Jan 14, 2014

Could this also allow custom mapping of db column names to model attribute names?

I've got a use case for this where I have to use a legacy db schema and can introduce proper model naming but use old table names. I'd like to do the same with column names without creating views.

I guess the drawback is confusion when referring to attribute names in plain SQL expressions in query criteria.

@lucianobaraglia

This comment has been minimized.

Show comment
Hide comment
@lucianobaraglia

lucianobaraglia Jan 14, 2014

Contributor

@nineinchnick maybe something like a column aliases property (of course, empty by default)?
So you could refer the property with the original column name and the alias...
Well...that is just an idea...

Contributor

lucianobaraglia commented Jan 14, 2014

@nineinchnick maybe something like a column aliases property (of course, empty by default)?
So you could refer the property with the original column name and the alias...
Well...that is just an idea...

@klimov-paul

This comment has been minimized.

Show comment
Hide comment
@klimov-paul

klimov-paul Jan 14, 2014

Member

Could this also allow custom mapping of db column names to model attribute names?

If the issue will be accepted and resolved, you will be able to achieve this via setter/getter.

/**
 * @param string $user_name db field
 */
class User extends ActiveRecord
{
    public function attributes()
    {
        return array_merge(
            parent::attributes()
            [‘name’]
        );
    }

    public function setName($value)
    {
        $this->user_name = $value;
    }

    public function getName()
    {
        return $this->name;
    }
}
Member

klimov-paul commented Jan 14, 2014

Could this also allow custom mapping of db column names to model attribute names?

If the issue will be accepted and resolved, you will be able to achieve this via setter/getter.

/**
 * @param string $user_name db field
 */
class User extends ActiveRecord
{
    public function attributes()
    {
        return array_merge(
            parent::attributes()
            [‘name’]
        );
    }

    public function setName($value)
    {
        $this->user_name = $value;
    }

    public function getName()
    {
        return $this->name;
    }
}
@qiangxue

This comment has been minimized.

Show comment
Hide comment
@qiangxue

qiangxue Jan 14, 2014

Member

I can't tell where the problem is. You can already use attributes() to declare attributes that are not DB columns.

Member

qiangxue commented Jan 14, 2014

I can't tell where the problem is. You can already use attributes() to declare attributes that are not DB columns.

@klimov-paul

This comment has been minimized.

Show comment
Hide comment
@klimov-paul

klimov-paul Jan 14, 2014

Member

You can already use attributes() to declare attributes that are not DB columns.

In this case ActiveRecord will attempt to save them inside the database table.

Member

klimov-paul commented Jan 14, 2014

You can already use attributes() to declare attributes that are not DB columns.

In this case ActiveRecord will attempt to save them inside the database table.

@qiangxue

This comment has been minimized.

Show comment
Hide comment
@qiangxue

qiangxue Jan 14, 2014

Member

QueryBuilder ensures only table columns are being inserted/updated.

Member

qiangxue commented Jan 14, 2014

QueryBuilder ensures only table columns are being inserted/updated.

@klimov-paul

This comment has been minimized.

Show comment
Hide comment
@klimov-paul

klimov-paul Jan 14, 2014

Member

QueryBuilder ensures only table columns are being inserted/updated.

Unfortunally this would not help for NOSQL solution like Mongo or Redis.

Member

klimov-paul commented Jan 14, 2014

QueryBuilder ensures only table columns are being inserted/updated.

Unfortunally this would not help for NOSQL solution like Mongo or Redis.

@klimov-paul

This comment has been minimized.

Show comment
Hide comment
@klimov-paul

klimov-paul Jan 14, 2014

Member

Also not about methods “ActiveRecord::getAttribute()” and “ActiveRecord::hasAttribute()”, which operates only “ActiveRecord::_attributes” field ignoring any attribute, which can be defined as public field or via setter/getter.

Member

klimov-paul commented Jan 14, 2014

Also not about methods “ActiveRecord::getAttribute()” and “ActiveRecord::hasAttribute()”, which operates only “ActiveRecord::_attributes” field ignoring any attribute, which can be defined as public field or via setter/getter.

@qiangxue

This comment has been minimized.

Show comment
Hide comment
@qiangxue

qiangxue Jan 14, 2014

Member

For NoSQL, how do you determine which attributes to save?

Member

qiangxue commented Jan 14, 2014

For NoSQL, how do you determine which attributes to save?

@qiangxue

This comment has been minimized.

Show comment
Hide comment
@qiangxue

qiangxue Jan 14, 2014

Member

hasAttribute() is correct, getAttribute() needs to be fixed.

Member

qiangxue commented Jan 14, 2014

hasAttribute() is correct, getAttribute() needs to be fixed.

@klimov-paul

This comment has been minimized.

Show comment
Hide comment
@klimov-paul

klimov-paul Jan 14, 2014

Member

For NoSQL, how do you determine which attributes to save?

In NOSQL like MongoDB there is schema. Each collection may store any fields set. More of that: same 2 different documents in the same collection may have entirely different set of fields.
For MongoDB I have no choice but saving all declared attributes.

I understand for regular DB this issue is not a problem, but it would improve things for NOSQL solution.

hasAttribute() is correct, getAttribute() needs to be fixed.

What about moving their declaration to “yii\base\Model” then?

Member

klimov-paul commented Jan 14, 2014

For NoSQL, how do you determine which attributes to save?

In NOSQL like MongoDB there is schema. Each collection may store any fields set. More of that: same 2 different documents in the same collection may have entirely different set of fields.
For MongoDB I have no choice but saving all declared attributes.

I understand for regular DB this issue is not a problem, but it would improve things for NOSQL solution.

hasAttribute() is correct, getAttribute() needs to be fixed.

What about moving their declaration to “yii\base\Model” then?

@qiangxue

This comment has been minimized.

Show comment
Hide comment
@qiangxue

qiangxue Jan 14, 2014

Member

Alright. I agree with you that we can add a method dbAttributes() as you proposed.

hasAttribute() is correct, getAttribute() needs to be fixed.
What about moving their declaration to “yii\base\Model” then?

Yes, that's fine.

Member

qiangxue commented Jan 14, 2014

Alright. I agree with you that we can add a method dbAttributes() as you proposed.

hasAttribute() is correct, getAttribute() needs to be fixed.
What about moving their declaration to “yii\base\Model” then?

Yes, that's fine.

@klimov-paul

This comment has been minimized.

Show comment
Hide comment
@klimov-paul

klimov-paul Jan 14, 2014

Member

@cebe, I would like to hear your opinion on this.

Member

klimov-paul commented Jan 14, 2014

@cebe, I would like to hear your opinion on this.

@nineinchnick

This comment has been minimized.

Show comment
Hide comment
@nineinchnick

nineinchnick Jan 14, 2014

Contributor

I assume that dbAttributes() will be used instead of attributes() when populating a model and when updating/inserting data? Could it support two array formats: default unordered array and associative for name mapping?
Since it's specialized (used in very limited way) I don't see any drawbacks of this.

Contributor

nineinchnick commented Jan 14, 2014

I assume that dbAttributes() will be used instead of attributes() when populating a model and when updating/inserting data? Could it support two array formats: default unordered array and associative for name mapping?
Since it's specialized (used in very limited way) I don't see any drawbacks of this.

@Ragazzo

This comment has been minimized.

Show comment
Hide comment
@Ragazzo

Ragazzo Jan 15, 2014

Contributor

Hm, i guess Yii2 will end-up with doctrine for Yii? rly? dont think this change is needed. It just one more step for DDD ORM like doctrine.

Contributor

Ragazzo commented Jan 15, 2014

Hm, i guess Yii2 will end-up with doctrine for Yii? rly? dont think this change is needed. It just one more step for DDD ORM like doctrine.

@nineinchnick

This comment has been minimized.

Show comment
Hide comment
@nineinchnick

nineinchnick Jan 15, 2014

Contributor

@Ragazzo if it's optional and gives more functionality, why not include it? It will prevent code bloat when needed by avoiding the need to introduce another layer of models and/or mass of getters/setters.

Contributor

nineinchnick commented Jan 15, 2014

@Ragazzo if it's optional and gives more functionality, why not include it? It will prevent code bloat when needed by avoiding the need to introduce another layer of models and/or mass of getters/setters.

@Ragazzo

This comment has been minimized.

Show comment
Hide comment
@Ragazzo

Ragazzo Jan 15, 2014

Contributor

nope, i understand the problem with noSQL and other. but the point is

It will prevent code bloat when needed by avoiding the need to introduce another layer of models and/or mass of getters/setters.

yii ORM is AR, thats mean that you should (not must) avoid some custom public fields in the model, i dont like it too, but thats the rules. Also by implementing this one there will be nothing to stop us to introduce column-specifications in php-docs like in doctrine. And one-more issue and you will end-up with doctrine for Yii. So maybe in such cases you should reconsider logic of your application rather then implementing doctrine?

And what about writing php-doc @db-attribute against this dbAttribute method? Ahh... right, its like Doctrine :D

Contributor

Ragazzo commented Jan 15, 2014

nope, i understand the problem with noSQL and other. but the point is

It will prevent code bloat when needed by avoiding the need to introduce another layer of models and/or mass of getters/setters.

yii ORM is AR, thats mean that you should (not must) avoid some custom public fields in the model, i dont like it too, but thats the rules. Also by implementing this one there will be nothing to stop us to introduce column-specifications in php-docs like in doctrine. And one-more issue and you will end-up with doctrine for Yii. So maybe in such cases you should reconsider logic of your application rather then implementing doctrine?

And what about writing php-doc @db-attribute against this dbAttribute method? Ahh... right, its like Doctrine :D

@cebe

This comment has been minimized.

Show comment
Hide comment
@cebe

cebe Jan 15, 2014

Member

I do not get the problem. You are currently able to add public properties which are not stored in db but you can validate them. this is done in advanced app with password field.
Is there a use case that does not work currently?

attributes are fields that are stored in db, public properties are not, but both can be validated.

Member

cebe commented Jan 15, 2014

I do not get the problem. You are currently able to add public properties which are not stored in db but you can validate them. this is done in advanced app with password field.
Is there a use case that does not work currently?

attributes are fields that are stored in db, public properties are not, but both can be validated.

@klimov-paul

This comment has been minimized.

Show comment
Hide comment
@klimov-paul

klimov-paul Jan 15, 2014

Member

Is there a use case that does not work currently?

  1. In NOSQL like MongoDB there is schema. Each collection may store any fields set. More of that: same 2 different documents in the same collection may have entirely different set of fields.
    For MongoDB I have no choice but saving all declared attributes.

I understand for regular DB this issue is not a problem, but it would improve things for NOSQL solution.

  1. hasAttribute() is correct, getAttribute() needs to be fixed.
    What about moving their declaration to “yii\base\Model” then?
Member

klimov-paul commented Jan 15, 2014

Is there a use case that does not work currently?

  1. In NOSQL like MongoDB there is schema. Each collection may store any fields set. More of that: same 2 different documents in the same collection may have entirely different set of fields.
    For MongoDB I have no choice but saving all declared attributes.

I understand for regular DB this issue is not a problem, but it would improve things for NOSQL solution.

  1. hasAttribute() is correct, getAttribute() needs to be fixed.
    What about moving their declaration to “yii\base\Model” then?
@creocoder

This comment has been minimized.

Show comment
Hide comment
@creocoder

creocoder Jan 15, 2014

Contributor

@Ragazzo

yii ORM is AR, thats mean that you should (not must) avoid some custom public fields in the model, i dont like it too, but thats the rules

Any proof links about that recommendation?

i guess Yii2 will end-up with doctrine for Yii?

It just one more step for DDD ORM like doctrine.

Ahh... right, its like Doctrine :D

Hmm... panic detected ;) Doctrine implements DataMapper pattern. This issue suggestion have nothing similar. All fine here.

Contributor

creocoder commented Jan 15, 2014

@Ragazzo

yii ORM is AR, thats mean that you should (not must) avoid some custom public fields in the model, i dont like it too, but thats the rules

Any proof links about that recommendation?

i guess Yii2 will end-up with doctrine for Yii?

It just one more step for DDD ORM like doctrine.

Ahh... right, its like Doctrine :D

Hmm... panic detected ;) Doctrine implements DataMapper pattern. This issue suggestion have nothing similar. All fine here.

@klimov-paul

This comment has been minimized.

Show comment
Hide comment
@klimov-paul

klimov-paul Jan 15, 2014

Member

Also what about attributes from virtual properties (defined by setter/getter)?

Check this:
https://github.com/yiisoft/yii2/blob/master/framework/db/BaseActiveRecord.php#L220

If define virtual attribute via getter BaseActiveRecord::__get() will return null without attempt to invoke parent::__get().

Member

klimov-paul commented Jan 15, 2014

Also what about attributes from virtual properties (defined by setter/getter)?

Check this:
https://github.com/yiisoft/yii2/blob/master/framework/db/BaseActiveRecord.php#L220

If define virtual attribute via getter BaseActiveRecord::__get() will return null without attempt to invoke parent::__get().

@creocoder

This comment has been minimized.

Show comment
Hide comment
@creocoder

creocoder Jan 15, 2014

Contributor

@klimov-paul There is also one thing about virtual and real attribute collisions. For example we have real db attribute name and have getName() method. I think its good idea to make $model->name invoke getName() to return result instead of real name attribute and $model->name = 'Foo Bar' should invoke setName() if exists. When creating something complex than models for blog its rare when you need raw attributes.

Contributor

creocoder commented Jan 15, 2014

@klimov-paul There is also one thing about virtual and real attribute collisions. For example we have real db attribute name and have getName() method. I think its good idea to make $model->name invoke getName() to return result instead of real name attribute and $model->name = 'Foo Bar' should invoke setName() if exists. When creating something complex than models for blog its rare when you need raw attributes.

@Ragazzo

This comment has been minimized.

Show comment
Hide comment
@Ragazzo

Ragazzo Jan 15, 2014

Contributor

@creocoder

Any proof links about that recommendation?

prooflink for what is AR? :)

Hmm... panic detected ;) Doctrine implements DataMapper pattern. This issue suggestion have nothing similar. All fine here.

could be, but as i see it now Yii2 just going sf1 pathway, and this is just simple small first step :)

Contributor

Ragazzo commented Jan 15, 2014

@creocoder

Any proof links about that recommendation?

prooflink for what is AR? :)

Hmm... panic detected ;) Doctrine implements DataMapper pattern. This issue suggestion have nothing similar. All fine here.

could be, but as i see it now Yii2 just going sf1 pathway, and this is just simple small first step :)

@creocoder

This comment has been minimized.

Show comment
Hide comment
@creocoder

creocoder Jan 15, 2014

Contributor

@Ragazzo

prooflink for what is AR? :)

Note, that Yii use not original Active Record pattern from Martin Fowler. It use it own vision of it. This is some kind of original + additional features like virtual attributes, etc. Additional features does not convert Active Record pattern to something different. Its still Active Record ;-)

I can even say how to cook Yii 2 Active Record: take abstract Active Record pattern from Martin Fowler, add some practice features, experiments, experiments, refactoring... Done ) Currently experiments come to vision that we need to separate virtual attributes from real db attributes. This vision come from practical reasons. Lets see what will happen.

Contributor

creocoder commented Jan 15, 2014

@Ragazzo

prooflink for what is AR? :)

Note, that Yii use not original Active Record pattern from Martin Fowler. It use it own vision of it. This is some kind of original + additional features like virtual attributes, etc. Additional features does not convert Active Record pattern to something different. Its still Active Record ;-)

I can even say how to cook Yii 2 Active Record: take abstract Active Record pattern from Martin Fowler, add some practice features, experiments, experiments, refactoring... Done ) Currently experiments come to vision that we need to separate virtual attributes from real db attributes. This vision come from practical reasons. Lets see what will happen.

@Ragazzo

This comment has been minimized.

Show comment
Hide comment
@Ragazzo

Ragazzo Jan 15, 2014

Contributor

Additional features does not convert Active Record pattern to something different. Its still Active Record ;-)

yeas, i know, i am not saying anything against this. I am just saying that this is the first step - determine what attributes are db and what are not, to the DDD ORM like Doctrine. I even suggested to mark db-attributes by php-docs but ofcourse it will not be done, because of its like in Doctrine. Sometimes yii-core just dont want to see what things are. Well, bad to them so :D

Contributor

Ragazzo commented Jan 15, 2014

Additional features does not convert Active Record pattern to something different. Its still Active Record ;-)

yeas, i know, i am not saying anything against this. I am just saying that this is the first step - determine what attributes are db and what are not, to the DDD ORM like Doctrine. I even suggested to mark db-attributes by php-docs but ofcourse it will not be done, because of its like in Doctrine. Sometimes yii-core just dont want to see what things are. Well, bad to them so :D

@klimov-paul

This comment has been minimized.

Show comment
Hide comment
@klimov-paul

klimov-paul Jan 15, 2014

Member

@Ragazzo, lets start from simple question: how ActiveRecord should determine, which data should be saved into the database?

Member

klimov-paul commented Jan 15, 2014

@Ragazzo, lets start from simple question: how ActiveRecord should determine, which data should be saved into the database?

@Ragazzo

This comment has been minimized.

Show comment
Hide comment
@Ragazzo

Ragazzo Jan 15, 2014

Contributor

@klimov-paul strange question, i think the answer is just in its name. :)

Contributor

Ragazzo commented Jan 15, 2014

@klimov-paul strange question, i think the answer is just in its name. :)

@creocoder

This comment has been minimized.

Show comment
Hide comment
@creocoder

creocoder Jan 15, 2014

Contributor

I even suggested to mark db-attributes by php-docs but ofcourse it will not be done, because of its like in Doctrine.

This is not only one way how mark attributes in Doctrine. This is notations way. There is another ways. Also notations used in many other Symfony places. So to be honest notations it just a tool. Wide tool. Not having any relation to the Doctrine itself.

Contributor

creocoder commented Jan 15, 2014

I even suggested to mark db-attributes by php-docs but ofcourse it will not be done, because of its like in Doctrine.

This is not only one way how mark attributes in Doctrine. This is notations way. There is another ways. Also notations used in many other Symfony places. So to be honest notations it just a tool. Wide tool. Not having any relation to the Doctrine itself.

@klimov-paul

This comment has been minimized.

Show comment
Hide comment
@klimov-paul

klimov-paul Jan 15, 2014

Member

strange question, i think the answer is just in its name

@Ragazzo, so do you think ActiveRecord should just save all its internal data in the database?

Member

klimov-paul commented Jan 15, 2014

strange question, i think the answer is just in its name

@Ragazzo, so do you think ActiveRecord should just save all its internal data in the database?

@Ragazzo

This comment has been minimized.

Show comment
Hide comment
@Ragazzo

Ragazzo Jan 15, 2014

Contributor

@klimov-paul i think that using additional fields like in yii2-advanced $password is just a shit code, that should be removed. I also think that using a dozen of scenarios is just bad practice, but unfortunately for Yii it is a standard to produce bad code asap :) hope this could be changed.

Contributor

Ragazzo commented Jan 15, 2014

@klimov-paul i think that using additional fields like in yii2-advanced $password is just a shit code, that should be removed. I also think that using a dozen of scenarios is just bad practice, but unfortunately for Yii it is a standard to produce bad code asap :) hope this could be changed.

@klimov-paul

This comment has been minimized.

Show comment
Hide comment
@klimov-paul

klimov-paul Jan 15, 2014

Member

I see.

Member

klimov-paul commented Jan 15, 2014

I see.

@Ragazzo

This comment has been minimized.

Show comment
Hide comment
@Ragazzo

Ragazzo Jan 15, 2014

Contributor

@creocoder yep, but they are used in Doctrine, ofcourse not the same way, but as i said its just a simple first step :)

Contributor

Ragazzo commented Jan 15, 2014

@creocoder yep, but they are used in Doctrine, ofcourse not the same way, but as i said its just a simple first step :)

@klimov-paul

This comment has been minimized.

Show comment
Hide comment
@klimov-paul

klimov-paul Jan 15, 2014

Member

In NOSQL like MongoDB there is schema. Each collection may store any fields set. More of that: same 2 different documents in the same collection may have entirely different set of fields.
For MongoDB I have no choice but saving all declared attributes.

@cebe, do you confirm same issue for the Redis or it is just MongoDB problem?

Member

klimov-paul commented Jan 15, 2014

In NOSQL like MongoDB there is schema. Each collection may store any fields set. More of that: same 2 different documents in the same collection may have entirely different set of fields.
For MongoDB I have no choice but saving all declared attributes.

@cebe, do you confirm same issue for the Redis or it is just MongoDB problem?

@creocoder

This comment has been minimized.

Show comment
Hide comment
@creocoder

creocoder Jan 15, 2014

Contributor

@Ragazzo

i think that using additional fields like in yii2-advanced $password is just a shit code, that should be removed. I also think that using a dozen of scenarios is just bad practice, but unfortunately for Yii it is a standard to produce bad code asap :) hope this could be changed.

ABSOLUTTELY DISAGREE. Scenarios is unique Yii/Yii 2 feature! Moreover it allow to not create separate models for trivial reasons. Using scenarios is good practice. Not using === not folowing yii way. As about yii2-advanced-app than i can say that there excess LoginForm. Should be implemented as scenario to be consistent with other signup and recoveryPassword. Also for example realize you have model Post and need tagNames virtual attribute for it. What you will do? Make model Post and make model PostForm because we have virtual attribute here??? So $passord virtual attribute not only not bad code, this use unique yii tecniques. Bad practice is to use Yii model layer as it used in Zend or in Symfony. This is what real bad practice is.

Contributor

creocoder commented Jan 15, 2014

@Ragazzo

i think that using additional fields like in yii2-advanced $password is just a shit code, that should be removed. I also think that using a dozen of scenarios is just bad practice, but unfortunately for Yii it is a standard to produce bad code asap :) hope this could be changed.

ABSOLUTTELY DISAGREE. Scenarios is unique Yii/Yii 2 feature! Moreover it allow to not create separate models for trivial reasons. Using scenarios is good practice. Not using === not folowing yii way. As about yii2-advanced-app than i can say that there excess LoginForm. Should be implemented as scenario to be consistent with other signup and recoveryPassword. Also for example realize you have model Post and need tagNames virtual attribute for it. What you will do? Make model Post and make model PostForm because we have virtual attribute here??? So $passord virtual attribute not only not bad code, this use unique yii tecniques. Bad practice is to use Yii model layer as it used in Zend or in Symfony. This is what real bad practice is.

@Ragazzo

This comment has been minimized.

Show comment
Hide comment
@Ragazzo

Ragazzo Jan 15, 2014

Contributor

@creocoder i think we will not find something in common, because i truly think that using dozen of scenarios is just a shit-code, in in Yii2 its the same shit code as in Yii1. And it is more suitable to use +1 form-model for that case then just creating one more shit-scenario and put it all in god-object. Anyway its just a matter of developer quality, someone think scenarios are superuseful and overuse them, someone think they are not so useful.

Contributor

Ragazzo commented Jan 15, 2014

@creocoder i think we will not find something in common, because i truly think that using dozen of scenarios is just a shit-code, in in Yii2 its the same shit code as in Yii1. And it is more suitable to use +1 form-model for that case then just creating one more shit-scenario and put it all in god-object. Anyway its just a matter of developer quality, someone think scenarios are superuseful and overuse them, someone think they are not so useful.

@klevron

This comment has been minimized.

Show comment
Hide comment
@klevron

klevron Jan 15, 2014

Contributor

Issue tracker is not the right place for trolling...

Contributor

klevron commented Jan 15, 2014

Issue tracker is not the right place for trolling...

@creocoder

This comment has been minimized.

Show comment
Hide comment
@creocoder

creocoder Jan 15, 2014

Contributor

@Ragazzo Rules should be clear and strict. For example: Separate models === separate them all. Not use scenarios at all. Or for example: Use scenarios? Use it always do not separate models. Or for example: Separate only in search scenario case, etc. Anyway good practice is to have strict and clear rules without any exceptions to be ensure code use similar approaches, etc. If you need virtual attribute in model this not mean that ring is bell and you need model separation... Only looking on "separation rules list" you can 100% say separation needed or not and matter of developer quality is having such secret list ;-)

Contributor

creocoder commented Jan 15, 2014

@Ragazzo Rules should be clear and strict. For example: Separate models === separate them all. Not use scenarios at all. Or for example: Use scenarios? Use it always do not separate models. Or for example: Separate only in search scenario case, etc. Anyway good practice is to have strict and clear rules without any exceptions to be ensure code use similar approaches, etc. If you need virtual attribute in model this not mean that ring is bell and you need model separation... Only looking on "separation rules list" you can 100% say separation needed or not and matter of developer quality is having such secret list ;-)

@Ragazzo

This comment has been minimized.

Show comment
Hide comment
@Ragazzo

Ragazzo Jan 15, 2014

Contributor

Rules should be clear and strict.

there are always exceptions for the rules. I am for scenarios when they not introducing external fields/rules. Anyway it can become a holywar so i think its time to end this)) no offense of course :) Also in Yii2 it was introduced search model, but if you said about it about half-year ago in Yii1 everyone would say its not Yii way :D I am out)) not trolling, just pointing out things that are not good at my opinion :)

Contributor

Ragazzo commented Jan 15, 2014

Rules should be clear and strict.

there are always exceptions for the rules. I am for scenarios when they not introducing external fields/rules. Anyway it can become a holywar so i think its time to end this)) no offense of course :) Also in Yii2 it was introduced search model, but if you said about it about half-year ago in Yii1 everyone would say its not Yii way :D I am out)) not trolling, just pointing out things that are not good at my opinion :)

@qiangxue

This comment has been minimized.

Show comment
Hide comment
@qiangxue

qiangxue Jan 15, 2014

Member

Scenarios are good for reducing redundant code. However, overusing scenarios may make your model code bloated and hard to maintain. So developers need to make wise decisions on when to use scenarios. OK, no more discussion here about scenarios (If you want, feel free to open another thread with your proposal of change).

For this issue, we are deciding whether we need to introduce a new method dbAttributes(). This is not required for traditional DB because we can determine table columns from schema information. For databases that do not have such information, it seems to me such a method is probably necessary.

Then @nineinchnick suggested that perhaps we can take this chance to support attribute aliasing (or whatever name you call it.) For example, created_at becomes createdAt.

Member

qiangxue commented Jan 15, 2014

Scenarios are good for reducing redundant code. However, overusing scenarios may make your model code bloated and hard to maintain. So developers need to make wise decisions on when to use scenarios. OK, no more discussion here about scenarios (If you want, feel free to open another thread with your proposal of change).

For this issue, we are deciding whether we need to introduce a new method dbAttributes(). This is not required for traditional DB because we can determine table columns from schema information. For databases that do not have such information, it seems to me such a method is probably necessary.

Then @nineinchnick suggested that perhaps we can take this chance to support attribute aliasing (or whatever name you call it.) For example, created_at becomes createdAt.

@Ragazzo

This comment has been minimized.

Show comment
Hide comment
@Ragazzo

Ragazzo Jan 15, 2014

Contributor

For example, created_at becomes createdAt

is not it already so? yep will be good) i also think that we can add some scopes ability for gii models, what do you think? for example is_admin field in database will produce admin() scope notAdmin scopes in model. could be very useful, it will be custom like a build-relations options. if yes, then will open new issue for discussion :)

Contributor

Ragazzo commented Jan 15, 2014

For example, created_at becomes createdAt

is not it already so? yep will be good) i also think that we can add some scopes ability for gii models, what do you think? for example is_admin field in database will produce admin() scope notAdmin scopes in model. could be very useful, it will be custom like a build-relations options. if yes, then will open new issue for discussion :)

@samdark

This comment has been minimized.

Show comment
Hide comment
@samdark

samdark Jan 15, 2014

Member

Automatic scoping is one of the features of current stable CakePHP I've used for quite a while. It sounds like a good feature but in reality you'll harldly need most of these. So I'm against generating it for every field.

Making it possible to remap fields sounds like a good idea since it was asked many times but there's a big issue with it. It will cause lots of confusion because in queries you still should real database field names and it will be inconsistent. Introducing a special query language to fixing inconsistency is a bit too much for complexity and performance reasons.

Member

samdark commented Jan 15, 2014

Automatic scoping is one of the features of current stable CakePHP I've used for quite a while. It sounds like a good feature but in reality you'll harldly need most of these. So I'm against generating it for every field.

Making it possible to remap fields sounds like a good idea since it was asked many times but there's a big issue with it. It will cause lots of confusion because in queries you still should real database field names and it will be inconsistent. Introducing a special query language to fixing inconsistency is a bit too much for complexity and performance reasons.

@Ragazzo

This comment has been minimized.

Show comment
Hide comment
@Ragazzo

Ragazzo Jan 15, 2014

Contributor

Automatic scoping is one of the features of current stable CakePHP I've used for quite a while. It sounds like a good feature but in reality you'll harldly need most of these. So I'm against generating it for every field.

yep, and its optional. So also is there some more ideas/features that we can take from it as you used it for a while? Last my offtop comment here)

Contributor

Ragazzo commented Jan 15, 2014

Automatic scoping is one of the features of current stable CakePHP I've used for quite a while. It sounds like a good feature but in reality you'll harldly need most of these. So I'm against generating it for every field.

yep, and its optional. So also is there some more ideas/features that we can take from it as you used it for a while? Last my offtop comment here)

@samdark

This comment has been minimized.

Show comment
Hide comment
@samdark

samdark Jan 15, 2014

Member

Scopes is another topic, let's discuss in a separate issue or forums.

Member

samdark commented Jan 15, 2014

Scopes is another topic, let's discuss in a separate issue or forums.

@cebe

This comment has been minimized.

Show comment
Hide comment
@cebe

cebe Jan 16, 2014

Member

I assume “Model::attributes” specify the list of properties, which can be populated via “Model::load()” and validated via “Model::validate()”.

this assumption is not true, you can have normal properties set by Model::load() and validate them when they are in scope and have rules.

I suppose taking database table fields as a set of “Model::attributes” is a limitation, which does not allow you to add some attributes, which may require validation, but should not be saved in database.

Normal properties are not saved in the DB, they are not really attributes as they do not have the dirty feature and are not stored so they are not part of the attributes definition and hasAttribute() will not return true for them. Also getAttribute() is just fine.

I suppose for the ActiveRecord there should be “dbAttributes” in addition to regular “attributes”. By default “ActiveRecord::attributes()” will return result of “ActiveRecord::dbAttributes()” but can be overridden if needed.

This feature already exists but in different way. When you declare public properties and add them to the rules/scenarios you will have "attributes" that are not stored in db.
It makes no sense to try to support dirty feature for non db attributes. All other features are there and working.

  1. hasAttribute() is correct, getAttribute() needs to be fixed.

What exactly is wrong?

@cebe, do you confirm same issue for the Redis or it is just MongoDB problem?

the above explained works fine in elasticsearch, not tried with redis yet but should be no difference.

Member

cebe commented Jan 16, 2014

I assume “Model::attributes” specify the list of properties, which can be populated via “Model::load()” and validated via “Model::validate()”.

this assumption is not true, you can have normal properties set by Model::load() and validate them when they are in scope and have rules.

I suppose taking database table fields as a set of “Model::attributes” is a limitation, which does not allow you to add some attributes, which may require validation, but should not be saved in database.

Normal properties are not saved in the DB, they are not really attributes as they do not have the dirty feature and are not stored so they are not part of the attributes definition and hasAttribute() will not return true for them. Also getAttribute() is just fine.

I suppose for the ActiveRecord there should be “dbAttributes” in addition to regular “attributes”. By default “ActiveRecord::attributes()” will return result of “ActiveRecord::dbAttributes()” but can be overridden if needed.

This feature already exists but in different way. When you declare public properties and add them to the rules/scenarios you will have "attributes" that are not stored in db.
It makes no sense to try to support dirty feature for non db attributes. All other features are there and working.

  1. hasAttribute() is correct, getAttribute() needs to be fixed.

What exactly is wrong?

@cebe, do you confirm same issue for the Redis or it is just MongoDB problem?

the above explained works fine in elasticsearch, not tried with redis yet but should be no difference.

@klimov-paul

This comment has been minimized.

Show comment
Hide comment
@klimov-paul

klimov-paul Jan 24, 2014

Member

Issue discarded.

Member

klimov-paul commented Jan 24, 2014

Issue discarded.

@senguttuvang

This comment has been minimized.

Show comment
Hide comment
@senguttuvang

senguttuvang Jun 6, 2015

Contributor

I'm trying to use virtual properties by overriding attributes() in my Model. The attributes are only for validation and they don't have corresponding table fields.

    public function attributes() {
        $attributes = parent::attributes();
        return array_merge($attributes, [
            'phone'
        ]);
    }

When I'm doing $model->save(), Yii2 attempts to save phone field as well and throws the following exception.

SQLSTATE[42S22]: Column not found: 1054 Unknown column 'phone' in 'field list'

If QueryBuilder() ensures only DB fields are saved() as stated by @qiangxue, it should skip phone because it is merely virtual field, not mean to be saved. I'm sure I'm missing something simple & obvious, but I'm newbie. Please help.

Contributor

senguttuvang commented Jun 6, 2015

I'm trying to use virtual properties by overriding attributes() in my Model. The attributes are only for validation and they don't have corresponding table fields.

    public function attributes() {
        $attributes = parent::attributes();
        return array_merge($attributes, [
            'phone'
        ]);
    }

When I'm doing $model->save(), Yii2 attempts to save phone field as well and throws the following exception.

SQLSTATE[42S22]: Column not found: 1054 Unknown column 'phone' in 'field list'

If QueryBuilder() ensures only DB fields are saved() as stated by @qiangxue, it should skip phone because it is merely virtual field, not mean to be saved. I'm sure I'm missing something simple & obvious, but I'm newbie. Please help.

@lynicidn

This comment has been minimized.

Show comment
Hide comment
@lynicidn

lynicidn Jun 6, 2015

Contributor

try use safeAttributes

Contributor

lynicidn commented Jun 6, 2015

try use safeAttributes

@cebe

This comment has been minimized.

Show comment
Hide comment
@cebe

cebe Jun 6, 2015

Member

there is no need to add them to the attributes() just define the property and declare a rule for it, validation willl work.

Member

cebe commented Jun 6, 2015

there is no need to add them to the attributes() just define the property and declare a rule for it, validation willl work.

@senguttuvang

This comment has been minimized.

Show comment
Hide comment
@senguttuvang

senguttuvang Jun 6, 2015

Contributor

Yes, I got it. There's a typo in the attribute name where I set the attributes in controllers. I defined phone as private model property, defined getters / setters that's it. Removed both attributes() and safeAttributes() method overrides. Now, it works as expected. Thank you guys!

Contributor

senguttuvang commented Jun 6, 2015

Yes, I got it. There's a typo in the attribute name where I set the attributes in controllers. I defined phone as private model property, defined getters / setters that's it. Removed both attributes() and safeAttributes() method overrides. Now, it works as expected. Thank you guys!

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