active-record
active-record copied to clipboard
yiisoft
Getters and setters for Activerecord
If I understand correctly, here it is possible to leave wishes to the third version of the framework?
I very much need possibility to register the getter or the setter redefining default for any property of a model storing in a database Now you have to do it through filters or afterFind(). This is not convient That's how it looks like, for example, laravel: https://laravel.com/docs/5.6/eloquent-mutators#accessors-and-mutators
Если я правильно понимаю, здесь можно оставить пожелания к третьей версии фреймворка?
Очень не хватает возможности прописать свой геттер или сеттер переопределяющий дефолтный для какого-то свойства модели хранящего в бд Приходится изворачиваться через фильтры или afterFind Вот так это выглядит например, у laravel: https://laravel.com/docs/5.6/eloquent-mutators#accessors-and-mutators
Something like this in BaseActiveRecord
/**
* PHP getter magic method.
* This method is overridden so that attributes and related objects can be accessed like properties.
*
* @param string $name property name
* @throws InvalidArgumentException if relation name is wrong
* @return mixed property value
* @see getAttribute()
*/
public function __get($name)
{
//---------
$method_name = 'get' . ucwords($name, '_') . 'Attribute';
if (method_exists($this, $method_name)) {
return $this->$method_name();
}
//---------
if (isset($this->_attributes[$name]) || array_key_exists($name, $this->_attributes)) {
return $this->_attributes[$name];
}
if ($this->hasAttribute($name)) {
return null;
}
if (isset($this->_related[$name]) || array_key_exists($name, $this->_related)) {
return $this->_related[$name];
}
$value = parent::__get($name);
if ($value instanceof ActiveQueryInterface) {
$this->setRelationDependencies($name, $value);
return $this->_related[$name] = $value->findFor($name, $this);
}
return $value;
}
/**
* PHP setter magic method.
* This method is overridden so that AR attributes can be accessed like properties.
* @param string $name property name
* @param mixed $value property value
*/
public function __set($name, $value)
{
//---------
$method_name = 'set' . ucwords($name, '_') . 'Attribute';
if (method_exists($this, $method_name)) {
return $this->$method_name($value);
}
//---------
if ($this->hasAttribute($name)) {
if (
!empty($this->_relationsDependencies[$name])
&& (!array_key_exists($name, $this->_attributes) || $this->_attributes[$name] !== $value)
) {
$this->resetDependentRelations($name);
}
$this->_attributes[$name] = $value;
} else {
parent::__set($name, $value);
}
}
OK. Let's have a pull request (can you make it?) and see what unit tests will show us. Also I'm a bit concerned about performance since method_exists is reflection.
May be it is better to define some list of mapping or getter/setter functions for attributes? This shouldn't hit performance(as we search function by hash) and can be pretty flexible. May be used as
class Users extends ActiveRecord {
....
public function mappings() {
return [
'id' => function($original) {
return 'id' . $original;
}
];
}
....
}
The only concern in that approach is to agree, whether it should be mappings list or getters+setters list, or sophisticated solution with both.
getters+setters list, I think. AR can't be ORM because query language will rely on field names anyway.
I mean that mapping won't change what you use in ->andWhere(['field_name' => 42]) so I prefer a mapping for getters and setters only.
Then, I think, it is convenient to allow getter be cacheable out of the box to avoid multiple same computations
@samdark I don't think there is a need to reinvent a wheel here. method_exists is almost not performance hit, there's no need to compare it with reflection (see https://stackoverflow.com/a/26732717/735307), also it's already heavily used in framework's all core classes.
Nevermind, I agree that caching/storing map will be better anyways (as it will save resources later on), I have already prepared PR without caching, will submit it and finish the caching.
@MattRh @samdark You proposed mapping, but it adds further complexity to use the feature. I'm for caching (which I described under PR) and not defining mapping. So only first call of getter and/or setter will use method_exists and as I said already it is actually very small performance hit even if we call it each time.
I see there and in PR begins confusion. May be we have to define, for what concrete PURPOSE that feature is usefull, and then thing about exact implementation?
@MattRh I see this feature as a very good way to do several things:
- Do some formatting before using the attribute in the view (yes, there is already a way to do that, defining some getter and using it, but I think it is more intuitive for the getter to have the same name as the attribute.
- Do some post processing before saving to database. Those usually go in
beforeSave, but it's more readable and intuitive to have setters for each separate attribute instead of pollutingbeforeSave. I had some projects wherebeforeSavewas very ugly due to attribute transformations there. - Working with date attributes with getter and setters (again, there is already a way to define some getter and doing setter transformations defining it in validator, but again it's more intuitive to have setter for it instead of doing it in validators.)
And finally it is just normal to have a way to define setters and getters for any class property and I think AR attributes have equal right to have them. As it doesn't really add any big overhead (not only in performance,, but also in code base) I see all reasons valid to add this feature to the framework, especially as it is just another way of doing things and doesn't make anybody to change their habits. You can use it, or you can simply ignore this feature..
Also developers coming from Laravel, or those who are also working in Laravel, have requested this feature several times already and 3.0 is very good opportunity to finally add it.
Please do not overengineer and if possible no new conventions (e.g. hard-coded callable). Performance should be of concern here - we do huge amount of inserts/batch inserts in projects.
Currently the same goal can be reached with following:
- use default column value from table schema cache
- set AR public
column attributewhich is automatically initiated for new objects - use AR::beforeSave() event
- implement your own handling of default values
@lubosdz I've already answered some of your concerns in discussion earlier here and under the PR, but let me clarify it again for everybody.
First of all I see this functionality primarily aimed at beginners and newcomers. I know that I can have my own ActiveRecord parent for my models or the same functionality in trait (with many own downsides) as was mentioned under the PR. But I'm sure newcomers and beginners just use and do only the things that are documented in docs and are in core. I'm sure majority of beginners don't even use some great core (and not only) extensions, because they are not documented in Yii guide.
For the already present functionality (data transformation):
The difference here would be, that if I generally need value that is in db and in some cases I need some formatting or transformation, I will surely use the existing technique. But if I know that I will always need db data somehow formatted or transformed than I will use the technique used in this pull request. Also there is no existing way to have setter except doing that in some before event, the downsides of which were already mentioned.
I don't see how the same goal can be reached using the default column value from table schema.
I have already commented on the downsides of beforeSave earlier here.
For the all performance talk I have 2 proposals. First let me do some benchmarks and publish it here, I'm sure the change is negligible in the terms of performance, but I agree anyways that such changes can and must have real benchmarks under the hood. And second we can simply hide all this functionality under some flag and you won't have any performance hit if you don't activate this functionality, but we will a new good feature for the beginners.