yii-dream-team / yii2-upload-behavior

Yii2 file/image upload behavior for ActiveRecord
http://yiidreamteam.com/yii2/upload-behavior
MIT License
77 stars 45 forks source link

ActiveRecord is saved even when Exception is thrown #15

Closed vercotux closed 7 years ago

vercotux commented 8 years ago

The one thing this wonderful behavior is missing is the ability to reverse a save transaction in case the file fails to be stored (for example when the system throws an Exception due to incorrect permissions). Currently what appears to happen is there is an Exception thrown but the record gets stored in the database anyway without the file.

BioSin commented 7 years ago

@vercotux In this case you need to use DB transactions (just for example):

if ($model->load(Yii::$app->request->post()) && $model->validate()) {
            $dbTransaction = Yii::$app->db->beginTransaction();
            try {
                $model->save();
                $dbTransaction->commit();
            } catch (\Exception $e) {
                $dbTransaction->rollBack();
                throw $e;
            }
            return $this->refresh();
        }

PS: Sorry for a loooong reply :)

vercotux commented 7 years ago

You expect every programmer to do this? This should be done inside the behavior.

metalagman commented 7 years ago

@vercotux I think you could just add OP_INSERT/OP_UPDATE to your model transactions() method https://github.com/yiisoft/yii2/blob/master/framework/db/ActiveRecord.php#L383

vercotux commented 7 years ago

@russianlagman What, every time I use this behavior? Are you kidding me?

I know how to do it. The point is that I shouldn't need to. It should be done by the behavior.

It seems the only solution here is to fork the project into my own repository and fix it to use transactions.

metalagman commented 7 years ago

@vercotux you need to learn learn how to use transactional operations with scenarios: http://www.yiiframework.com/doc-2.0/guide-db-active-record.html#transactional-operations

vercotux commented 7 years ago

I told you I know how to do it. Read my message again.

metalagman commented 7 years ago

You should understand that not every ActiveRecord implementation could use transactions. For example, it just wont work for MyISAM, ElasticSearch connections.

BioSin commented 7 years ago

@vercotux  model saving with file loading is a a complex operation:

  1. Save the model
  2. Upload a file

Behavior shouldn't manage failures in this case because may have other operations in chain and etc. Only developer should decide how it should be handled. Behavior doing only one thing - file uploading, all the rest should be handled by the developer depending on environment, used DB, logic, etc.

If you want, you may extend behavior and override afterSave method in your project.

vercotux commented 7 years ago

@russianlagman You should not be using this behavior with a non-transactional DBMS in the first place. Unless you write shit code and you actually want it to fail. It depends on a transaction to work correctly.

@BioSin It may be complex, but this behavior needs to take care of it. It most certainly should handle its own failures. You cannot expect that from the developer who has no understanding of how your behavior works internally.

It is not the developer's responsibility to handle your internal exceptions, especially when your documentation does not even warn them about it.

metalagman commented 7 years ago

@vercotux please consider the situation when you need to process multiple models in a one transaction. Not all RDBMS support nested transactions. So it's up to you how to wrap saving into transaction: using transactions() or with a try/catch block (and how to handle exceptions). Documentation patches are welcome.

vercotux commented 7 years ago

@russianlagman Again, WHY would you be doing this on a non-transactional RDBMS? You want it to crash and fail?