Magic getter in BaseActiveRecord has readability and extensibility issues

[arthibald]

The magic __get($name) override in BaseActiveRecord is confusing, and its combination of private _attributes and _related array access and strange use of $this->hasAttributes($name) renders it inextensible.

public function __get($name)
{
    if (isset($this->_attributes[$name]) || array_key_exists($name, $this->_attributes)) {
        return $this->_attributes[$name];
    } elseif ($this->hasAttribute($name)) {
        return null;
    } else {
        if (isset($this->_related[$name]) || array_key_exists($name, $this->_related)) {
            return $this->_related[$name];
        }
        $value = parent::__get($name);
        if ($value instanceof ActiveQueryInterface) {
            return $this->_related[$name] = $value->findFor($name, $this);
        } else {
            return $value;
        }
    }
}

This line..

... elseif ($this->hasAttributes($name)) {return null;}

.. might return the correct value for the framework, but it makes no sense in a getter. Also if the hasAttributes($name) function is extended to include extra attributes, returning null here is not expected.

I would suggest the following replacement, which as far as I can tell produces the same logic, but allows for much greater extensibility:

public function __get($name)
{
    if ($this->hasAttribute($name)) {
        return $this->getAttribute($name);
    }  else { 
        if (isset($this->_related[$name]) || array_key_exists($name, $this->_related)) {
            return $this->_related[$name];
        }
        $value = parent::__get($name);
        if ($value instanceof ActiveQueryInterface) {
            return $this->_related[$name] = $value->findFor($name, $this);
        } else {
            return $value;
        }
    }
}

[cebe]

if ($this->hasAttribute($name)) {
    return $this->getAttribute($name);

This is exactly the same as current yii code, but current code is optimized for speed, so what is the benefit of your change?

[arthibald]

Thanks cebe, the difference is that in the original code:

... elseif ($this->hasAttributes($name)) {return null;}

..will always return null, even if $this->hasAttributes($name) returns something that the model can access through $this->getAttributes($name).

It works for the base definitions of $this->hasAttributes($name) and $this->getAttributes($name) in an ActiveRecord, but doesn't make sense as soon as these attribute methods are overridden.

[dynasource]

I agree with this. Two things can be further improved:

• logic: the return value of null is weird after a positive hasAttribute inside a getter
• performance: hasAttribute() performs isset($this->_attributes[$name]) for the second time

[samdark]

Here's my variant of it with comments. A bit more performant and maybe a bit more readable:

public function __get($name)
{
    // if there's a value for such attribute, return it
    if (isset($this->_attributes[$name])) {
        return $this->_attributes[$name];
    }

    // if there's no value or value is null and attribute exists, return null
    if (in_array($name, $this->attributes(), true)) {
        return null;
    }

    // if there's such relation, return it
    // isset here is for performance, array_key_exists is here because there
    // could be valid nulls
    if (isset($this->_related[$name]) || array_key_exists($name, $this->_related)) {
        return $this->_related[$name];
    }

    $value = parent::__get($name);
    if ($value instanceof ActiveQueryInterface) {
        return $this->_related[$name] = $value->findFor($name, $this);
    }

    return $value;
}

[samdark]

Overall, it's not a critical thing to hold release. Moving to next milestone.

[Tigrov]

Related with #18

[Tigrov]

Done #318