Consider this controller action that:
[list=A]
[*] validates a form model
[*] on successful validation, inside a transaction
-
saves a new token AR model
-
updates the user AR model
-
sends an email
[/list]
<?php
namespace app\controllers;
class SiteController extends \yii\web\Controller
{
public function actionPasswordResetRequest()
{
$form = new \app\models\forms\PasswordResetRequestForm();
if ($form->load(\Yii::$app->request->post()) && $form->validate()) {
$user = \app\models\User::findByEmail($form->email);
$user->last_reset_request = new \yii\db\Expression('UTC_TIMESTAMP()');
$token = new \app\models\AuthToken($form);
$transaction = \Yii::$app->db->beginTransaction();
try {
if (!$token->save(false)) {
throw new \Exception('Failed to save token model');
}
if (!$user->save(false)) {
throw new \Exception('Failed to save user model');
}
\app\helpers\MailHelper::passwordResetRequest($token);
$transaction->commit();
} catch (\Exception $e) {
$transaction->rollBack();
throw $e;
}
}
return $this->render('password_reset_request', [
'model' => $form,
]);
}
}
All three tasks in the transaction use external services (DB and MTA) that can fail and if any does, we rollback.
But I have a problem with the two instances of this boilerplate
if (!$model->save(false)) {
throw new \Exception('...');
}
Let’s say I know that neither model has a beforeSave that can return false, so I know these conditions will never be true and the branches are dead code that I can’t test.
But if I remove those branches, I am violating the separation of responsibilities between classes by disregarding the API spec.
public boolean save ( $runValidation = true, $attributeNames = null )
My controller design must not rely on knowing if a model’s beforeSave can return false. Nor should it rely on the model or the framework not changing. Hence I need the discipline to respect the API spec and test the return, even though I know it’s BS.
I don’t need such a test to send the email because that method has void return and if something goes wrong, it will throw an exception. I designed it like that.
I think this points to a flaw in ActiveRecord’s API design. If an AR model has no validation or I do not need to run it, I need a method that will either succeed or throw an exception.
I could extend AR with things like
public function reallySave()
{
if (!$this->beforeSave(true)) {
throw new BeforeSaveFailedException(get_class($this) . '::beforeSave() returned false');
}
$this->save(false);
}
but I expect it’s more complex than that and I would prefer if the framework gave me this.
It’s normal to validate with yii\base\Model and then save AR models without validation. So I think the framework should give AR methods that:
-
save/insert/update without validating
-
have void return
-
throw an exception if, for whatever reason, the save doesn’t happen.
What are your practices and opinions with saving AR models after input validation already passed?