AfterSave can not rollback saving transaction

[pgunold-evc]

What steps will reproduce the problem?

• Implement afterSave() to save any sub-models depending of the primary key of the main model
• return false to afterSave() when saveing of ony of the sub-models fails

What is the expected result?

The transaction of the main model should be rolled back without throwing an exception. (when using transactions). And should return save() of BaseActiveRecord should return false when not using transactions.

What do you get instead?

transaction is not rolled back. and BaseActiveRecord::save() returns true

Additional info

There was already an similar issue #8392 and a comment suggesting a change that would fix the problem see https://github.com/yiisoft/yii2/issues/609#issuecomment-20873800

Suggestion how to fix

change the call of afterSafe()-call in ActiveRecord::insertInternal and BaseActiveRecord::updateInternal to return its result e.g. like this:

protected function insertInternal($attributes = null)
// .... other code
    if (!$this->beforeSave(true, $changedAttributes)) {
        return false;
    }
}

UPDATE This ensures that only when returnung bool false the saving is interupted

protected function insertInternal($attributes = null)
// .... other code
    if ($this->beforeSave(true, $changedAttributes) === false) {
        return false;
    }
}

why other ideas to solve this won`t work

• save or validate the related models in beforeSave()
  • won`t work, because they need the saved primaryKey of the main model
• overwriting save() methode and add an own transaction that can be rolled back when save of submodels fails
  • this works, but it is a bad way because needs a duplication of the most try/catch code from ActiveRecord::insert
• throw an exception within afterSave() like suggested in https://github.com/yiisoft/yii2/issues/8392#issuecomment-101576185
  • 1. this would result in an exception thrown by the save method instead of returning false
  • 2. it is a inconsistend behavior that beforeSave() can rollback the save with returning false, and afterSave() must throw
  • 3. method signature of afterSave() does not say that it could throw exceptions, what could lead to unexpected behavior if you throw exceptions there

[mtangoo]

Can you provide sample code of your overriden afterSave? Did you call parent::afterSave before returning false?

[bizley]

AfterSave was clearly designed to not changed the result of insertInternal() and updateInternal(). As this is purely a matter of creators' decision I can understand that some people might see it differently than planned. Even though this could be changed, it's BC breaking change, so not in Yii 2.

[mtangoo]

AfterSave was clearly designed to not changed the result of insertInternal() and updateInternal()

That is how I personally use it for I just never bothered to check whether it rolls back in case of failure. In this case OP might need to wrap his code under his own top level transactions

[pgunold-evc]

Can you provide sample code of your overriden afterSave? Did you call parent::afterSave before returning false?

Yes I can.

public function afterSave($insert, $changedAttributes)
{
    $savedSuccessful = parent::afterSave($insert, $changedAttributes);

    // walk over labels in all languages
    foreach ($this->labels as $lang => $label) {
        $label->productID = $this->productID;

        if (!($savedSuccessful&= $content->save())) {
            $this->addErrorsFromChild($label, "$lang.label");
        }
    }

    return $savedSuccessful;
}

This is one of many examples where we need to save attached sub-models that require to be saved with the ID of themain model (so it can only be done AFTER saving the main model).

In case of insert errors this should not throw an exception, it should add its error-messages to the error-list in the main model, that these messages can be returned to the frontend and can be displayed on the incorrect form fields

[pgunold-evc]

AfterSave was clearly designed to not changed the result of insertInternal() and updateInternal(). As this is purely a matter of creators' decision I can understand that some people might see it differently than planned. Even though this could be changed, it's BC breaking change, so not in Yii 2.

Why this is a breaking change??

If we assume that currently afterSave() returns nothing in the most cases... Adding the suggested change would change NOTHING for all currently implemented code,. So there is nothing that could break.

And even if somebody does not like the "returning" behavior, he could easly overwrite afterSave in his own model to return nothing.

But in the oposide way, there is no chance to overwrite saving bevior to respect the result of afterSave without writing a full own implementation of save, InsertInternal, UpdateInternal, what would result in skipping all future updates that would be made by yii team in one of this methods.

[rob006]

Why this is a breaking change??

This will change how method works and in some way changes its signature - for 10 years afterSave() was expected to return nothing and all code works like this. Changing this expectation may result errors in existing code. Also, this makes sense only if you start a transaction (and rollback on error), which Yii does not automatically (and IMO should not). So having afterSave() that return bool and ignore this value by framework does not look right from design perspective.

I really do not understand what is the problem with throwing an exception in afterSave() and overwriting save() to catch these exceptions and handle transaction. This really looks like straightforward solution that requires like 10 lines of code (that could be extracted to trait or base class).

[pgunold-evc]

Why this is a breaking change??

This will change how method works and in some way changes its signature - for 10 years afterSave() was expected to return nothing and all code works like this. Changing this expectation may result errors in existing code. Also, this makes sense only if you start a transaction (and rollback on error), which Yii does not automatically (and IMO should not). So having afterSave() that return bool and ignore this value by framework does not look right from design perspective.

I really do not understand what is the problem with throwing an exception in afterSave() and overwriting save() to catch these exceptions and handle transaction. This really looks like straightforward solution that requires like 10 lines of code (that could be extracted to trait or base class).

1. if you dont start a transaction returning false from afterSave would also return false in the save method.
2. Throwing an exception is the wrong bevaior in that case, because in "regular" errors the model should add messages to its error list and only throw Exceptions in case something unexpected is happening (e.g. any db error that was not checked in the validation rules)
3. throwing an exception and catching it in an own method would mean that you need to write a new exception type, and only catch this, that bevaior e.g. for DB exceptions would staay the same

[rob006]

• if you dont start a transaction returning false from afterSave woul also return false in the save method.

You mean that save() will return false in that case?

• Throwing an exception is the wrong bevaior in that case, because in "regular" errors the model should add messages to its error list and only throw Exceptions in case something unexpected is happening (e.g. any db error that was not checked in the validation rules)

1. That is actually a wrong assumption IMO. You expect that afterSave() should validate related models, but IMO it is too late for that - you should validate them in afterValidate(), and afterSave() should skip validation and only save already validated data. In this way you have more natural flow: first you validate everything, and if validation pass you save everything. I you do validation right, afterSave() shouldn't throw exception (except some super rare cases you probably don't want to cover).
2. How do you handle these exception is up to you: you can convert exceptions to validation errors and return false in save().

• throwing an exception and catching it in an own method would mean that you need to write a new exception type, and only catch this, that bevaior e.g. for DB exceptions would staay the same

And? I'm not sure what is the problem here.

[mtangoo]

 foreach ($this->labels as $lang => $label) {
        $label->productID = $this->productID;

        if (!($savedSuccessful&= $content->save())) {
            $this->addErrorsFromChild($label, "$lang.label");
        }
    }

I think you are abusing after save here. I would do something along these lines

class SomeModel extends ActiveRecord
{

    public function updateLabelsOrWhatever()
    {
        foreach ($this->labels as $lang => $label) {
            $label->productID = $this->productID;

            if (!($savedSuccessful&= $content->save())) {
                throw Exception('Whatever error I want to throw here')
            }
        }
    }
}

And then on the controller do something like

class SomeController extends Controller 
{
    public function actionWhatever()
    {
        $model = new SomeModel(...);
        $transaction = SomeModel::getDb()->beginTransaction();
        try
        {
            //......
            if(!$model->save()){
                throw Exception('Could not save Some model blah blah');
            }

            $model->updateLabelsOrWhatever();
            $transaction->commit()
        } catch(Exception $e){
            $transaction->rollBack();
            //do something with $e->getMessage()
        }
    }
}

It does not have to be this one but you get idea that this is not the use case for afterSave

[pgunold-evc]

• if you dont start a transaction returning false from afterSave woul also return false in the save method.

You mean that save() will return false in that case?

Yes, according the documentation of the safe method, this is what it should do.

• Throwing an exception is the wrong bevaior in that case, because in "regular" errors the model should add messages to its error list and only throw Exceptions in case something unexpected is happening (e.g. any db error that was not checked in the validation rules)

1. That is actually a wrong assumption IMO. You expect that `afterSave()` should validate related models, but IMO it is too late for that - you should validate them in `afterValidate()`, and `afterSave()` should skip validation and only save already validated data. In this way you have more natural flow: first you validate everything, and if validation pass you save everything. I you do validation right, `afterSave()` shouldn't throw exception (except some super rare cases you probably don't want to cover).

I agree that it would be a better workflow if it would be possible to validate everything before saving, but that is technical impossible until the ID of the main model is known.

Because only if the ID of the main-model is known we can realy check the validation of the sub model.

2. How do you handle these exception is up to you: you can convert exceptions to validation errors and return `false` in `save()`.

• throwing an exception and catching it in an own method would mean that you need to write a new exception type, and only catch this, that bevaior e.g. for DB exceptions would staay the same

And? I'm not sure what is the problem here.

The major problem would be that this is much more effort then just implementing afterSave and secord is, that we lose all connection to the validation messages in the sub-model if we throw an exception.

What if e.g. more than one field in the submodel is invalid? Then it should pass all messages to the main model.

e.g.

• label.de: Text can must be longer than 5 chars.
• label..en: This field connot be empty
• label.es: Text must be less than 255 chars

[pgunold-evc]

 foreach ($this->labels as $lang => $label) {
        $label->productID = $this->productID;

        if (!($savedSuccessful&= $content->save())) {
            $this->addErrorsFromChild($label, "$lang.label");
        }
    }

I think you are abusing after save here. I would do something along these lines

class SomeModel extends ActiveRecord
{

    public function updateLabelsOrWhatever()
    {
        foreach ($this->labels as $lang => $label) {
            $label->productID = $this->productID;

            if (!($savedSuccessful&= $content->save())) {
                throw Exception('Whatever error I want to throw here')
            }
        }
    }
}

Throwing an Exception there whould be bad, because you would leave the function, without checking all other labels if they countain errors too.

And then on the controller do something like

class SomeController extends Controller 
{
    public function actionWhatever()
    {
        $model = new SomeModel(...);
        $transaction = SomeModel::getDb()->beginTransaction();
        try
        {
            //......
            if(!$model->save()){
                throw Exception('Could not save Some model blah blah');
            }

            $model->updateLabelsOrWhatever();
            $transaction->commit()
        } catch(Exception $e){
            $transaction->rollBack();
            //do something with $e->getMessage()
        }
    }
}

It does not have to be this one but you get idea that this is not the use case for afterSave

And that is bad in 2 ways.

1. implementing this in the countroller would mean that we need to re-write the code in each action that saves this model. When using REST-Api this would further mean we need to rewrite the build-in REST-Actions.
2. Your code would rise an hard error, withour returning messages to the fromnend form which fields missed the validation.

[rob006]

• if you dont start a transaction returning false from afterSave woul also return false in the save method.

You mean that save() will return false in that case?

Yes, according the documentation of the safe method, this is what it should do.

Returning false by save() in case when record was saved to database (it is already saved when afterSave() is executed) is definitely not correct behavior.

Because only if the ID of the main-model is known we can realy check the validation of the sub model.

Why would you need such ID to validate sub model? It is generated automatically by your code, so in what case it could be incorrect?

The major problem would be that this is much more effort then just implementing afterSave and secord is, that we lose all connection to the validation messages in the sub-model if we throw an exception.

This is literally like 5 lines of code (+3 more if you want to add transaction):

    public function save($runValidation = true, $attributeNames = null) {
        try {
            return parent::save($runValidation, $attributeNames);
        } catch (MyValidationException $exception) {
            $this->addErrors($exception->getValidationErrors());
            return false;
        }
    }

[mtangoo]

Throwing an Exception there whould be bad, because you would leave the function, without checking all other labels if they countain errors too.

You can put your logic there. I just made a random example to make a point.

Throwing an Exception there whould be bad, because you would leave the function, without checking all other labels if they countain errors too.

Again you do not have to, put whatever is fit for your logic. This is not a copy-paste solution, but rather illustration of a point. I hope you get the actual point am trying to make here.

And that is bad in 2 ways. implementing this in the countroller would mean that we need to re-write the code in each action that saves this model.

updateLabelsOrWhatever is there to show that you can put biz logic related stuffs there in the model and call it with a single line. But it is not the only solution. You can use traits as your copy-paste assistant.

When using REST-Api this would further mean we need to rewrite the build-in REST-Actions.

Why not when they do not fulfill what you want?

Your code would rise an hard error, withour returning messages to the fromnend form which fields missed the validation.

Again you miss the point because you take it as copy-paste full solution rather than illustration of a point

[pgunold-evc]

• if you dont start a transaction returning false from afterSave woul also return false in the save method.

You mean that save() will return false in that case?

Yes, according the documentation of the safe method, this is what it should do.

Returning false by save() in case when record was saved to database (it is already saved when afterSave() is executed) is definitely not correct behavior.

Acording the documentation of save, insert and update the save method should return false if any validations fail. So if the main entry was inserted, and the required depending data of the sub models was not saved, the save should be rolled back.

In total this means, the entry is not saved, because validations failed, So in my eyes that is a case where returning false would match the expectations.

Because only if the ID of the main-model is known we can realy check the validation of the sub model.

Why would you need such ID to validate sub model? It is generated automatically by your code, so in what case it could be incorrect?

The ID is required because the exist-rule should check if the given ID exists in the linked table, otherwise you could add anything as ID, and the database would reject the insert/update because of an incorrect foreign key.

That is what the exists rule should validate.

The major problem would be that this is much more effort then just implementing afterSave and secord is, that we lose all connection to the validation messages in the sub-model if we throw an exception.

This is literally like 5 lines of code (+3 more if you want to add transaction):

  public function save($runValidation = true, $attributeNames = null) {
      try {
          return parent::save($runValidation, $attributeNames);
      } catch (MyValidationException $exception) {
          $this->addErrors($exception->getValidationErrors());
          return false;
      }
  }

Your example looks like a nice idea, but it would require to write a special exception class, what could store multidimensional array that could contain multiple validation errors of multiple fields.

of course this could work...

But first this is an unnecessary complicated solution, when there is a much more intuitive way using afterSave().

And in my eyes it would be very confusion to invent a fully new technic to abort saving with validation errors, when beforeSave and afterSave would be a perfect pair that should share similar logic and handling.

Or do you realy like to write a documentation, to explain users how they can abort saving in that way you suggest it above?

[rob006]

Acording the documentation of save, insert and update the save method should return false if any validations fail.

And that is true right now. You're the person which propose to change this, because you want to save some models before entire validation is completed. ;)

So if the main entry was inserted, and the required depending data of the sub models was not saved, the save should be rolled back.

We were talking about situation without transaction, since automatic transaction in save() is IMO really bad idea. And without transaction save() would save object to database and say it didn't do it.

The ID is required because the exist-rule should check if the given ID exists in the linked table, otherwise you could add anything as ID, and the database would reject the insert/update because of an incorrect foreign key.

You create this parent-model just before saving sub-model, why would you need to check if parent-model really exists? You can easily skip this rule using scenarios (and I would check if it is needed at all - you probably don't need validation for such field if it is always filled programmatically).

But first this is an unnecessary complicated solution, when there is a much more intuitive way using afterSave().

IMO the main problem is that you're using afterSave() in a wrong way - you're validating sub-models after main model was saved. If your proposal makes it more easy or intuitive to write code like that, then that is an argument against this change, because it will promote bad habits and patterns.

[pgunold-evc]

Acording the documentation of save, insert and update the save method should return false if any validations fail.

And that is true right now. You're the person which propose to change this, because you want to save some models before entire validation is completed. ;)

Its right that this change would move some validations to afterSave, but the results comming out of the save function would be as espected.

So if the main entry was inserted, and the required depending data of the sub models was not saved, the save should be rolled back.

We were talking about situation without transaction, since automatic transaction in save() is IMO really bad idea. And without transaction save() would save object to database and say it didn't do it.

Okay thats correct, in non-transaction situation you could end with an partitional saved state and save methode would return false. I agree, that this is a bad sitiation that we should avoid.

The ID is required because the exist-rule should check if the given ID exists in the linked table, otherwise you could add anything as ID, and the database would reject the insert/update because of an incorrect foreign key.

You create this parent-model just before saving sub-model, why would you need to check if parent-model really exists? You can easily skip this rule using scenarios (and I would check if it is needed at all - you probably don't need validation for such field if it is always filled programmatically).

Good point.

But first this is an unnecessary complicated solution, when there is a much more intuitive way using afterSave().

IMO the main problem is that you're using afterSave() in a wrong way - you're validating sub-models after main model was saved. If your proposal makes it more easy or intuitive to write code like that, then that is an argument against this change, because it will promote bad habits and patterns.

As far as I understand, you yust yust say any form of validation, that could / should be done after saving the main model is considered to be a wrong way, correct?

In this case I have no futher arguments, and afterSave becomes a nearly useless construct.

[rob006]

As far as I understand, you yust yust say any form of validation, that could / should be done after saving the main model is considered to be a wrong way, correct?

I say that all validation should be done when validate() is called (save() calls it before saving record to database). If in afterSave() you're not sure whether data is valid or not, then there is something wrong with your validation flow and I would focus on fixing/improving it. And if it is not possible, then there are examples in this thread how to handle it using exceptions, but IMO framework should not promote such solution.

[pgunold-evc]

As far as I understand, you yust yust say any form of validation, that could / should be done after saving the main model is considered to be a wrong way, correct?

I say that all validation should be done when validate() is called (save() calls it before saving record to database). If in afterSave() you're not sure whether data is valid or not, then there is something wrong with your validation flow and I would focus on fixing/improving it. And if it is not possible, then there are examples in this thread how to handle it using exceptions, but IMO framework should not promote such solution.

Okay, than I am done here.

I already found an solution for my own. I yust liked to share my idea to solve a problem with the afterSave method. If this is considered as wrong hebavior, than this issue could be closed

[handcode]

This will change how method works and in some way changes its signature - for 10 years afterSave() was expected to return nothing and all code works like this.

it's BC breaking change, so not in Yii 2

I think that says it all.

If you want to make such changes to the basic methods of the framework, you have to write a custom ActiveRecord class and extend your models from it, or implement one of the proposed “solutions” mentioned here or in https://github.com/yiisoft/yii2/issues/8392.

In the framework itself, however, such signature changes within yii2 are not (or no longer) an option.