Let’s say we have two simple tables:
The boolean column is_paid of the order table is used to mark whether the order is paid. Whenever an order is paid, we insert a record into the log table. An order could be paid by running the order/pay action:
/**
* Controller
*/
class OrderController extends \yii\web\Controller
{
public function actionPay($id)
{
$this->findModel($id)->pay();
Yii::$app->session->setFlash('success', 'You have paid the order.');
return $this->goHome();
}
}
/**
* Model
*/
class Order extends \yii\db\ActiveRecord
{
/**
* Pay an order
*/
public function pay()
{
$this->is_paid = 1;
// whenever the order is paid, log the payment
$this->on(self::EVENT_AFTER_UPDATE, [$this, 'log'], "paid order # {$this->id}");
if (!$this->save()) {
throw new \yii\db\Exception('Failed');
}
}
/**
* After the order was paid, log the payment.
*/
public function log($event)
{
$message = $event->data;
$log = new Log(['message' => $message]);
if (!log->save()) {
throw new \yii\db\Exception('Failed');
}
}
}
1 Reproduce the problem
Suppose that there is an unpaid order in the order table, the end user first visited order/index on a PC, then he revisited order/index on a mobile phone. Now, the status of the order on both devices was unpaid.
The end user clicked the ‘pay’ button on PC to pay the order, after the operation, the is_paid value of the order was changed to 1, and there was a new record in the log table. After a while, the user reopened the order/index page on the mobile phone, as the page was no reloaded, the order’s status was still UNPAID, if the user clicks the ‘pay’ button, there will be a duplicate log record, dirty data occurs.
2 How to avoid
2.1 Solution A: Pre-Check in every action
Before executing $model->pay(), we could do a pre-check:
class OrderController extends \yii\web\Controller
{
public function actionPay($id)
{
$model->$this->findModel($id);
// pre-check
if($model->is_paid) {
Yii::$app->session->setFlash('warning', 'This order has been paid.');
return $this->redirect(Yii::$app->request->referrer);
}
$model->pay();
Yii::$app->session->setFlash('success', 'You have paid the order.');
return $this->goHome();
}
}
One of the cons of solution A is that it makes the action less clean, What’s more, such checks are frequent in many cases, writing similar code in every action is a verbose job.
2.2 Solution B: Action Filter
We could create an generic action filter called StatusGuardFilter:
namespace backend\behaviors;
use Yii;
use yii\web\NotFoundHttpException;
class StatusGuardFilter extends \yii\base\ActionFilter
{
public $modelClass;
// find the specified AR record
protected function findModel()
{
$modelClass = $this->modelClass;
$keys = $modelClass::primaryKey();
$values = [];
foreach ($keys as $key) {
$values[] = Yii::$app->request->get($key);
}
$model = $modelClass::findOne(array_combine($keys, $values));
if (isset($model)) {
return $model;
} else {
throw new NotFoundHttpException("Object not found.");
}
}
public function beforeAction($action)
{
$model = $this->findModel();
// check if $action is allowed, method detail is following
$errorMsg = $model->getStatusGuardErrorMessage($action->id);
if ($errorMsg) {
Yii::$app->session->setFlash('warning', $errorMsg);
$action->controller->redirect(Yii::$app->request->referrer);
return false;
}
return true;
}
}
/**
* Model
*/
class Order extends \yii\db\ActiveRecord
{
// ...
/**
* Check whether an order could run $action
*
* @param string $action action name
*
* @return string|null if the action is forbidden, an error message will be returned, or null if the action is allowed.
*/
public function getStatusGuardErrorMessage($action)
{
switch ($action) {
case 'pay':
if ($this->is_paid) {
return 'This order has been paid.';
}
break;
}
return null;
}
}
Based the aboved code, now we rewrite OrderController as following:
/**
* Controller
*/
use backend\behaviors\StatusGuardFilter;
class OrderController extends \yii\web\Controller
{
/**
* @inheritdoc
*/
public function behaviors()
{
return [
'guard' => [
'only' => ['pay'],
'class' => StatusGuardFilter::className(),
'modelClass' => '\backend\models\Order',
],
];
}
public function actionPay($id)
{
$this->findModel($id)->pay();
Yii::$app->session->setFlash('success', 'You have paid the order.');
return $this->goHome();
}
}
2.2.1 Pros and cons of solution B
Pros
The action code looks clean; The action filter is reusable, for similar cases, what we need to do is: config behaviors(), then fill logic code in getStatusGuardErrorMessage(). For example, when updating an order, we want to ensure that the order is unpaid and it is created by current user. We change codes in the following three classes:
-
add
updateaction into theguardbehavior:public function behaviors() { return [ 'guard' => [ 'only' => ['pay', 'update'], // ... ], ]; } -
fill check logic of update operation in Order class:
public function getStatusGuardErrorMessage($action)
{
switch ($action) {
// ...
case 'update':
if ($this->is_paid) {
return 'Only unpaid order updating is allowed.';
}
if ($this->created_by != Yii::$app->user->id) {
return 'Only order's owner can update the order';
}
break;
}
return null;
}
Cons: Extra DB query
The first query is in StatusGuardFilter:
class StatusGuardFilter extends \yii\base\ActionFilter
{
protected function findModel()
{
// first DB query: findOne()
$model = $modelClass::findOne(array_combine($keys, $values));
}
}
If an order is allowed to pay, a duplicate DB query is executed in findModel() of OrderController:
class OrderController extends \yii\web\Controller
{
public function actionPay($id)
{
// in findModel(), we execute Order::findOne() twice
$this->findModel($id)->pay();
}
}
It seems that findOne() could not be cached, so solution B generates an extra DB query cost.
3 Question
Solution A vs Solution B, Which is better? or is there anther better way?