Skip to content
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

Unable to check if ActiveRecord own properties isset or empty using array access notation #17717

Open
beroso opened this issue Dec 6, 2019 · 6 comments

Comments

@beroso
Copy link
Contributor

beroso commented Dec 6, 2019

What steps will reproduce the problem?

Created a ActiveRecord model class, for example:

class FinancialTransaction extends \yii\db\ActiveRecord
{
    /**
     * Custom property to be set manually
     */
    public $annotation;

    /**
     * @inheritdoc
     */
    public static function tableName()
    {
        return '{{%account}}';
    }

    // ... 
}

And run code like this:

$model = new FinancialTransaction();
$model->annotation = 'Some annotation';

var_dump($model['annotation']);
var_dump(isset($model['annotation']));
var_dump(empty($model['annotation']));

What is the expected result?

string(15) "Some annotation"
bool(true)
bool(false)

What do you get instead?

string(15) "Some annotation"
bool(false)
bool(true)

Suggestion:

Change implementation of BaseActiveRecor::offsetExists from:

public function offsetExists($offset)
{
    return $this->__isset($offset);
}

to:

public function offsetExists($offset)
{
    return isset($this->$offset);
}

So it will cover own ActiveRecord properties and fallback to __isset when needed.

Additional info

Q A
Yii version 2.0.28, 2.0.30
PHP version 7.3, 7.4
Operating system Mac OSX 10.15.1
@samdark samdark added the type:bug Bug label Dec 9, 2019
@samdark samdark added this to the 2.0.31 milestone Dec 9, 2019
@samdark samdark added status:ready for adoption Feel free to implement this issue. type:bug Bug and removed type:bug Bug labels Dec 12, 2019
@samdark samdark removed this from the 2.0.31 milestone Dec 12, 2019
@jwlewisiii
Copy link

jwlewisiii commented Dec 20, 2019

BaseActiveRecord extends yii\base\Model which implements the suggestion above. We can remove offsetExists method from the BaseActiveRecord class and rely on the Model classes implementation.

yii\base\Model::offsetExists

public function offsetExists($offset)
    {
        return isset($this->$offset);
    }

@samdark samdark added status:under discussion and removed status:ready for adoption Feel free to implement this issue. labels Dec 20, 2019
@samdark
Copy link
Member

samdark commented Dec 20, 2019

Potentially that may break things. @yiisoft/core-developers, @yiisoft/reviewers what do you think? Any breaks that may happen?

@xepozz
Copy link
Contributor

xepozz commented Dec 20, 2019

If you need to check properties, please check from $model->attributes ($model->attributes['name'].
You can't check any indexes from object. By default it will throw an exception.


The fact that ActiveRecord "hides" this exception is very frustrating.
It would be nice to remove this strange behavior.

@samdark samdark added this to the 2.0.32 milestone Dec 25, 2019
@samdark samdark modified the milestones: 2.0.32, 2.0.33 Jan 21, 2020
@samdark samdark removed this from the 2.0.33 milestone Mar 23, 2020
@tQuant
Copy link

tQuant commented Feb 18, 2022

Since recent versions yii starts to check isset on attribute when loading relations in \yii\db\ActiveRelationTrait::filterByModels

    private function filterByModels($models)
    {
            ...
            foreach ($models as $model) {
                $value = isset($model[$attribute]) ? $model[$attribute] : null;

Until recent versions, relations of this kind worked

class FinancialTransaction extends \yii\db\ActiveRecord
{
    /**
     * Custom property to be set manually
     */
    public $annotationId;

    /**
     * @inheritdoc
     */
    public static function getAnnotation()
    {
        return $this->hasOne(className, ['id' => 'annotationId']);
    }
}

Now they don't .


\yii\db\BaseActiveRecord::offsetUnset() method checks for the existence of the property:

    public function offsetUnset($offset)
    {
        if (property_exists($this, $offset)) {
            $this->$offset = null;
        } else {
            unset($this->$offset);
        }
    }

Maybe just add the same check to \yii\db\BaseActiveRecord::offsetExists() method?

    public function offsetExists($offset)
    {
        return property_exists($this, $offset) || $this->__isset($offset);
    }

@wheldongori
Copy link

@samdark Was this issue sorted or how do we debunk this problem?

@samdark
Copy link
Member

samdark commented Feb 2, 2023

The original one descibed in the initla post? No. In order to solve it, a pull request would be great to see if it breaks any existing tests.

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

Successfully merging a pull request may close this issue.

6 participants