yiisoft / yii2

Yii 2: The Fast, Secure and Professional PHP Framework
http://www.yiiframework.com
BSD 3-Clause "New" or "Revised" License
14.24k stars 6.91k forks source link

Active record virtual properties #2262

Closed Ragazzo closed 10 years ago

Ragazzo commented 10 years ago

Currently we cant use getters and setters in active record for fields.

Active record classes should first check if getter or setter for property exists and call it, if no then return $this->_attributes[$name] or set it directly. For properties with _ we should define camel-case getters/and setters.

Laravel4 handles this situation so Yii2 should also i think.

samdark commented 10 years ago

I think it should be fixed.

Ragazzo commented 10 years ago

Same for Yii1, getters and setters are not called for properties.

cebe commented 10 years ago

what is your use case? normal attributes can not be getters/setters or properties, as we have the dirty attributes feature which would not work for them. everything else is working fine. see https://github.com/yiisoft/yii2/issues/1967#issuecomment-32446239

samdark commented 10 years ago

This is not about saving what is defined by setter and getter, it's just about calling it like a property.

cebe commented 10 years ago

so you are saying virtual properties defined by getters and setters are not working in AR? I can confirm that they are, I am using them every day...

Ragazzo commented 10 years ago

so you are saying virtual properties defined by getters and setters are not working in AR?

yep, if i have attribute login then setLogin will not work.

cebe commented 10 years ago

yes of course it will not, as you can not mix properties and attributes as already mentioned above (dirty feature, etc...).

cebe commented 10 years ago

Please give valid use cases for having a setter for a property.

Ragazzo commented 10 years ago

yes of course it will not as you can not mix properties and attributes as already mentioned above (dirty feature, etc...).

why? i dont see any drawbacks, we only need to make $_attributes as protected and thats all. Getters and Setters are very useful.

samdark commented 10 years ago

Noone want to mix attributes with properies:

/**
 * Has @property $username
 */
class User extends ActiveRecord
{
  public function setUsername($value) {
    $this->attributes['usename'] = dosomething($value);
  }
}
samdark commented 10 years ago
$user->username = 'xxx'; // will use setter
echo $user->username; // will use attributes directly since there's no setter
cebe commented 10 years ago

What in this case can not be done with validation filters? Please give some usecases.

samdark commented 10 years ago
/**
 * Has @property $username
 */
class User extends ActiveRecord
{
  public function setUsername($value) {
     throw new Exception('Username is read only!');
  }
}
Ragazzo commented 10 years ago

@cebe password generating:

public function setPassword($value)
{
    $this->attributes['password'] = SecurityHelper::generatePassword($value);
}

more then usual and very common usecase i think :)

cebe commented 10 years ago

okay, labeled as enhancement. Will think about it a bit. In general I find it confusing as there is no rule that getter or setter really changes the attribute you think it does. Can also cause confusion if you add columns to table which then conflict with the getter/setter etc...

Ragazzo commented 10 years ago

Can also cause confusion if you add columns to table which then conflict with the getter/setter etc...

this is developer issue, he write code and he is responsible for it. Also users are testing migrations too.

cebe commented 10 years ago

Sure, but a framework should avoid introducing things that may confuse. We should carefully check if a feature causes problems before introducing it. Also find the most straight forward way of implementing it.

qiangxue commented 10 years ago

I'm against this change. You basically want to let getter/setter have higher precedence over column names. This is problematic because it means if you define a getter/setter in a base class for a different purpose, it will cause problem to data population silently. For example, in BaseActiveRecord we have oldAttributes property. If your table column also has such a column, you are in trouble.

Ragazzo commented 10 years ago

@qiangxue but in this way we can have any other issue with virtual properties, right? for any other Yii2 component.

cebe commented 10 years ago

@qiangxue but in this way we can have any other issue with virtual properties, right? for any other Yii2 component.

example?

qiangxue commented 10 years ago

Currently, the rule is very clear: column names have higher precedence over getter/setter property names. This is consistent in both data population (create()) and data access (__get()). What you are proposing is to change the precedence in __get() only. This will definitely cause problem and confusion. But if you change both places, then you lose the "dirty attribute" feature.

Ragazzo commented 10 years ago

But if you change both places, then you lose the "dirty attribute" feature.

not so familiar with fw internals, but L4 has this good feature, and i find it useful, i think other developers also will do. Very strange why everybody is against and acting like i am doing unneeded stuff, note i am not saying that i am victim or other, i just dont know how this good features can be not counted?

qiangxue commented 10 years ago

@Ragazzo We have been explaining to you why this may cause problems, but you only want to read what you want. Yes, such feature seems nice, well at the first sight, but it will bite you later. L4 doesn't care many things. For example, its base class Model contains many class members, such as visible. If your table happens to have the same named column, doing $model->visible will give you unexpected result. It also has the problem I described above.

With the current design, if you want to get/set a column whose format is different from what it reads from or displays to end users, you should define a property via getter/setter for it. Use created_at as an example, you should write getCreationTime()/setCreationTime() or something similar.

Ragazzo commented 10 years ago

@qiangxue

For example, its base class Model contains many class members, such as visible. If your table happens to have the same named column, doing $model->visible will give you unexpected result. It also has the problem I described above.

but is not this a developer problem and he need do use/fix it? php for example does not allow you use reserved words, and what? :)

With the current design, if you want to get/set a column whose format is different from what it reads from or displays to end users, you should define a property via getter/setter for it. Use created_at as an example, you should write getCreationTime()/setCreationTime() or something similar.

but in this case i cant use good feature as mass assignment.

qiangxue commented 10 years ago

but is not this a developer problem and he need do use/fix it? php for example does not allow you use reserved words, and what? :)

True, but this is not an excuse and should be avoided as much as possible. And if such a problem happens, there should be a way to warn you, rather than giving you an unexpected result silently.

but in this case i cant use good feature as mass assignment.

Why not? Instead of using the column name, you use the property defined by your getter/setter, do validation, massive assignment, etc., as usual.

P.S. and if we all will be honest then lets say that Yii2 AR was taken from L4 :)

What do you want to express here? No doubt L4 has many creative ideas and we learned a lot from it. But this doesn't mean anything it does is correct and we should blindly follow it. And saying Yii2 AR is taken from L4 is not honest at all. Most of the ideas and designs in Yii2 AR are original or inherited from Yii1 (very obviously). The only thing we learned from L4 is the relation definition which we further enhanced significantly.

Ragazzo commented 10 years ago

What do you want to express here? No doubt L4 has many creative ideas and we learned a lot from it. But this doesn't mean anything it does is correct and we should blindly follow it. And saying Yii2 AR is taken from L4 is not honest at all. Most of the ideas and designs in Yii2 AR are original or inherited from Yii1. The only thing we learned from L4 is the relation definition which we further enhanced significantly.

yeah, i deleted it to not to start holywar (also not only relation and scopes and $query object) :) you can ignore this as it was said when i was disappointed. :)

Why not? Instead of using the column name, you use the property defined by your getter/setter, do validation, massive assignment, etc., as usual.

i cant, because if i have for example password field (beforeSave example lets not count here) i cant make it as getter/setter and use it. or i missed something and can you give good example?

qiangxue commented 10 years ago

i cant, because if i have for example password field (beforeSave example lets not count here) i cant make it as getter/setter and use it. or i missed something and can you give good example?

You should define a property with a different name (e.g. plainPassword).

Ragazzo commented 10 years ago

yes, but users will not be using it and using directly property. so we have situation when user should remember that there is a getter or virtual property for original attribute.

qiangxue commented 10 years ago

Well, that's the trade off you need to take. I just reviewed L4's code regarding dirty attributes. It seems it will have problem with dirty attributes if you define getter/setter for a column.

Ragazzo commented 10 years ago

Ok, then i will close it. Lets hope someday php will have getters/setters support)

andrewmclagan commented 9 years ago

If we look around at other frameworks... they do this.

andrewmclagan commented 9 years ago

If i have a field that is json encoded... i really dont want to have Json::encode / Json::decode in my controllers.

There should be the possibility to access $model->myJsonProperty and output json data via property mutators and accessors?

cebe commented 9 years ago

@andrewmclagan http://www.yiiframework.com/doc-2.0/guide-db-active-record.html#data-transformation

andrewmclagan commented 9 years ago

okay... off to another framework we go....

cebe commented 9 years ago

you've been quite unspecific about your problem, so maybe #4899 is more like what you need?

briedis commented 9 years ago

Feel your pain @andrewmclagan! Good luck with laravel :)

But on the topic, laravel has mutators - http://laravel.com/docs/5.1/eloquent-mutators

public function getMyJsonAttribute(){
   return json_decode($this->my_json);
}

public function setMyJsonAttribute($value){
   $this->attributes['my_json'] = json_encode($value);
}

which are damn useful. Defining a property with a different name seems much worse, and codes can easily forget to call the "right" property.

armpogart commented 9 years ago

:+1: Laravel Mutators and Accessors are a little bit more clear and better for data transformation and they're also easily implementable in Yii2. @cebe Should I prepare PR for it? Is there any chance that such functionality will be accepted?

andrewmclagan commented 9 years ago

I agree, its super handy having mutators not only in Laravel but frameworks from other languages. Seem to be quite a common feature.

samdark commented 9 years ago

@armpogart yes, there's a chance.

dhoomm commented 8 years ago

Just my opinion that this should not be implemented..

Consider public function setPassword($value) { $this->attributes['password'] = SecurityHelper::generatePassword($value); }

A developer writing $this->password = $rawPassword; will also expect ($this->password == $rawPassword)

It will be magic happening behind the scenes.

Now consider .net framework. For a property, say password, you can access it as this.password but when you create a getter and setter, you have to access it as this.Password so that the code writer knows that its a getter/setter with syntax sugar (On purpose design). You can easily acheive same behavior already. $this->Password will first check for property "Password", but "Password" != "password", so it will call the yii\base\Component' s set or get.

So with the code: $this->Password we can see that something is different and know that there is a setter/getter being used.

Of course, it will not work in mass assignment.