Yii2: afterSave for new models flushes unsaved data

[yujin1st]

After #13567 AR now flushed relational data, so there is new issue:

What steps will reproduce the problem?

1. Create simple app with Order and OrderItem models.
2. Load data in controller, create relational OrderItem records and populate relation 'items'

// In Order 
public function load($data, $formName = null) {
  if (!parent::load($data, $formName)) return false;
  $this->loadItems($data);
  return true;
}

/// loading and populating data
private function loadItems($data) {
  if (!isset($data['items'])) {
    return;
  }
  $items = $this->items;
  foreach ($data['items'] as $info) {
    $itemId = $info['itemId'];
    if (isset($items[$itemId])) {
      $items[$itemId]->load($info, '');
    } else {
      $item = new OrderItem([]);
      $item->load($info, '');
      $items[$itemId] = $item;
    }
  }
  $this->populateRelation('items',$items);
}

1. Validate, do something in beforeSave method and event
   3.1 Validate in Order model
   3.2 For example warehouse module can check item quantity before order creating in event
2. Save items in afterSave

public function afterSave($insert, $changedAttributes) {
  $this->saveItems();
  parent::afterSave($insert, $changedAttributes);
}

1. Do something with new items in afterSave method and event

What is the expected result?

All works fine, order data saved, relational data saved too

What do you get instead?

After #13567 items relation with unfinished data is flushed, as Order model changes it's id, so you lose data

Additional info

It simple workflow allows to validate and process relational form data as ActiveRecord models with all it features and controllers remains thing. And you could use scenarios for complex relations, so models can be easily tested.

I'm afraid this issue will be closed due contradiction with updated framework design, but i have a multiply applications that follows this workflow, so i don't know what to do ;(((

| Q | A
| ---------------- | ---
| Yii version | 2.0.14

[samdark]

@markmarco16, @rob006, @Kolyunya thoughts?

[cebe]

which operation causes the reset of relational data? A one-many relation has no attribute in the order that should cause resetting the relation when modified.

[samdark]

Indeed, that should not be related to #13567 as there are no relation attributes in the model you modify... @yujin1st are you sure your problem caused by #13567?

[yujin1st]

I'm sure. Debugging leads to this code:

ActiveRecord class:

protected function insertInternal($attributes = null){
//...
//lines 518
        foreach ($primaryKeys as $name => $value) {
            $id = static::getTableSchema()->columns[$name]->phpTypecast($value);
            $this->setAttribute($name, $id);
            $values[$name] = $id;
// ...
        }

BaseActiveRecord class: 
public function setAttribute($name, $value){
//...
//line 521
  $this->resetDependentRelations($name);
//...
}

internal property _relationsDependencies is set when loading items, and this line is executed to preload old saved data:

/// loading and populating data
private function loadItems($data) {
  $items = $this->items; 
}

At this moment relation is not set, so __get method executes:

BaseActiveRecord class: 
 public function __get($name){
///.. 296 line
 if ($value instanceof ActiveQueryInterface) {
            $this->setRelationDependencies($name, $value);
            return $this->_related[$name] = $value->findFor($name, $this);
        }
}

=======
now after debugging i think i can avoid this issue by adding if:

$items =  $this->isNewRecord ? [] : $this->items;

But will this be correct resolution?
It means every time you access relation you should keep in mind, if model is new, if it's foreign keys can be updated by this, and after all this means it's a lot places to rewrite.. =(

[yujin1st]

And i think i found another interesting case:
What happens if you load items with lazy loading ($query->with()) , so relation is populated in \yii\db\ActiveRelationTrait::populateRelation, so setRelationDependencies method is not called and internal property is not set too?

[samdark]

Hmm... let's get back to Order model. It has one to many relation so there's no column in Order that makes a relation. Is that correct? What action with the model triggers resetting a relation?

[yujin1st]

Doesn't id column compose relation?
$model->save() resets relation: save()->insert->primary key id updates (from null to value)->setAttribute()->resetDependentRelations() -> relation resets;

[samdark]

So the case is only about not yet saved model, right?

[yujin1st]

Yes, when id is changing after saving new model

[samdark]

I guess in this case we should not reset relations and your extra condition to the reset method or where using it may fix it.

[mg-code]

Same issue here.
I was forced to refactor many forms to fix this bug.

I really like the idea that we can populate relation manually and then use related models same way for create/update actions. After #13567 merge I get empty relations after saving parent model.

[samdark]

@mg-code do you have other ideas (not a flag to turn it on/off) on how to fix that?

[mg-code]

@samdark one solution would be to add a new parameter to function populateRelation which indicates whether to keep relation in sync. By default it would be false, but in all other places (e.g. ActiveQuery) we can set it to true.

Imho, probably not the best solution, but better than current.

[yujin1st]

Behaviour or trait? if you need - you manually enable it with params.

[yujin1st]

sorry, i missed that fact, that we can't override methods in behaviours.

[mg-code]

It would be better to create a behavior that resets relation, not visa versa.
Because, currently this merge breaks backward compatibility.

We could create an event (e.g. AFTER_ATTRIBUTE_CHANGE) and for those who need a functionality of reseting relations could use some behavior.

[rob006]

There is already a PR with fix: #16085. What is wrong with it?

[mg-code]

I think it might work.

I thought that ActiveQuery is calling populateRelation to fill relations. Now I see that it is not and there is no need for an additional parameter.

[samdark]

@mg-code it's in. Tests are fine so I think it will work well. Please test with master.

[mg-code]

@samdark yes, it works now. Thanks!

[cebe]

https://github.com/yiisoft/yii2/pull/16085#discussion_r183968666

the tests in https://github.com/yiisoft/yii2/pull/13618/files#diff-5c2b59d39a2ea1dc5443fb98d1325d11 do not cover eager loading. Pretty sure this gets broken by this change.

[cebe]

I thought that ActiveQuery is calling populateRelation to fill relations. Now I see that it is not and there is no need for an additional parameter.

it does in case of eager loading:
https://github.com/yiisoft/yii2/blob/c3c982068dc21697e1c167bf2d20e55813aa4f0d/framework/db/ActiveRelationTrait.php#L253

[mg-code]

@cebe Yes, you are right. So additional parameter can be a solution.

[cebe]

or a separate method, e.g. populateRelationInternal()

[mg-code]

I was thinking about that, but came to idea that internal methods are protected or private, but we need to call this method from outside.

[samdark]

@cebe Eager loading should not be affected anyway since it's not used for refreshing as far as I know.

[yujin1st]

is any possibility to use this fix in stable version?
we can't use any version above 2.0.13 because of it =(

[shirase]

Issue already fixed in master.

[samdark]

Yes. Will tag next release as soon as I'll have a bit more time.