Yii2: Dirty attributes. Strict comparison of the values ​​of numeric type.

[6pblcb]

With a strict comparison
https://github.com/yiisoft/yii2/blob/master/framework/db/BaseActiveRecord.php#L561

$value !== $this->_oldAttributes[$name]

numerical data coming from the form, always not equal.
I'm not sure, but the question may be wider than a single line.

update: There is some mention of it here #622, but I have not found continue.

[qiangxue]

Related to this change: https://github.com/yiisoft/yii2/pull/2263

Non-strict comparison is not acceptable as this may lose data. Strict comparison may falsely return some clean attributes as dirty, but shouldn't cause much trouble.

[samdark]

Which data could be lost in case of ints?

[qiangxue]

0 vs "" for example.

[samdark]

Right. @6pblcb if you have an idea how to deal with it please comment.

[6pblcb]

Thanks! Indeed, I do not see the beautiful output. Well, that will process the data before checking.

[Agrumas]

What about

if (isset($names[$name]) && (!array_key_exists($name, $this->_oldAttributes) || ($value != $this->_oldAttributes[$name] && ($value || $value !== $this->_oldAttributes[$name])))) {
    $attributes[$name] = $value;
} 

[6pblcb]

@samdark As you solution above?

[samdark]

Won't help with 0 vs "".

[6pblcb]

Third attempt:
Need to add condition is_numeric
If in one line:

if (isset($names[$name]) && (!array_key_exists($name, $this->_oldAttributes) || (is_numeric($value) && is_numeric($this->_oldAttributes)) ? $value != $this->_oldAttributes[$name] : $value !== $this->_oldAttributes[$name])) {
...
}

Everything will remain the same, only with numbers will not do a strict comparison. Do not pretend to the most elegant solution (probably need to split one string.). And do not be in every model, follow this.

[samdark]

Well, I don't think we can take such change without a full set of unit tests for each case including edge cases like 0 and "". If you want to work on the test set, let me know.

[Agrumas]

@samdark could you specify the case with 0 vs ""
Because my solution use non strict compare when value evaluates to true and strict when it false.

[samdark]

Well, the value was "" and we changed it to 0 and want to save it.

// $this->_oldAttributes[$name] = `""`;
// $value = 0;


if (isset($names[$name])) { // OK
    if (!array_key_exists($name, $this->_oldAttributes) || ($value != $this->_oldAttributes[$name] && ($value || $value !== $this->_oldAttributes[$name])))) { // fail
         $attributes[$name] = $value;
     } 
}

$value != $this->_oldAttributes[$name] evaluates to false making it impossible to save.

[samdark]

// $this->_oldAttributes[$name] = `""`;
// $value = 0;

if (isset($names[$name]) { // OK
  if (!array_key_exists($name, $this->_oldAttributes) ||
      (is_numeric($value) && is_numeric($this->_oldAttributes)) ? $value != $this->_oldAttributes[$name] : $value !== $this->_oldAttributes[$name])) {
...
}

This one is OK for the case and probably is a solution. Need it unit tested still.

[qiangxue]

Should consider these cases at least:

• "" (an empty string)
• 0 (0 as an integer)
• 0.0 (0 as a float)
• "0" (0 as a string)
• NULL
• FALSE
• array() (an empty array)

Also refer to http://www.php.net/manual/en/types.comparisons.php to enumerate all possible cases.

[Agrumas]

https://gist.github.com/Agrumas/f990c668f8597a35afaa
Any suggestions?

[tvdavid]

Right, thanks for pointing me to the existing issue. I went for a different approach entirely:

public function afterValidate()
{
    $schema = static::getTableSchema();

    foreach ($this->attributes as $attribute => $value) {
        if (!$this->hasErrors($attribute)) {
            $typecasted = $schema->getColumn($attribute)->phpTypecast($value);

            if ($value !== $typecasted) {
                $this->setAttribute($attribute, $typecasted);
            }
        }
    }

    return parent::afterValidate();
}

It's very convenient that your attributes will always be of the same (correct) type, regardless of whether the record was loaded from the database or from user input. Perhaps something like this makes sense in the core?

[cebe]

should be done on setAttribute then instead of afterValidate.

[tvdavid]

I wanted it to happen _after_ validation, otherwise it would break scenarios where people use validators/filters to convert input to the proper format (e.g. using implode to convert array input to a comma separated string)

[cebe]

May I ask for the problem that arises when we keep current behavior?
Imo re-storing a number is not a big problem and if you really care, you should convert your input data for example using input validation filters. So what are the issues to fix with this?

[tvdavid]

Because the yii functions will report that certain attributes have changed while they really haven't? If you use afterSave() to keep an audit log of the changes on a model that would be a serious problem.

I must say I still like my solution better than the one proposed in the PR. With my proposal you can use strict comparison everywhere without having to worry if the model was loaded from user input or having to add filters for every number input manually.

E.g. this would not work with the PR solution and could cause quite a headache:

class Model {
const TYPE_PHONE = 1;
const TYPE_CAR = 2;
}

$model = new Model;
$model->load(Yii::$app->request->post()); // $_POST['model']['type'] = '1'
$model->save();

if ($model->type === Model::TYPE_PHONE) { ... }

[samdark]

Decided to keep current behavior as discussed in #4399

[docsolver]

@samdark
@qiangxue
Would it be possible to make the arrays $_attributes and $_oldAttributes in BaseActiveRecord protected instead of private? This would enable developers to extend the class and overwrite the built-int getDirtyAttributes()-method.

[cebe]

@docsolver you can use getAttributes() and getOldAttributes() to override the method.

[docsolver]

right, thanks

[synatree]

@tvdavid is right, it does break Auditable behaviors. I'll try fixing it for my situation by overriding getOldAttributes.