Making error handling for Model::save more beautiful in CakePHP
Posted on 3/2/07 by Felix Geisendörfer
Deprecated post
The authors of this post have marked it as deprecated. This means the information displayed is most likely outdated, inaccurate, boring or a combination of all three.
Policy: We never delete deprecated posts, but they are not listed in our categories or show up in the search anymore.
Comments: You can continue to leave comments on this post, but please consult Google or our search first if you want to get an answer ; ).
Hi folks,
when looking through other peoples code, I often see actions like this:
{
$this->Task->set($this->data);
if ($this->Task->validates())
{
$this->Task->save()
// Display a success message
}
else
{
// Display an error message that validation failed
}
}
Looks good, wouldn't you say? Well it is good code for most cases. However, it doesn't handle an important aspect of saving something to the database: checking if the actual DB operation succeeded. Now I've written actions like the one above in the past as well. It's just that I've not had many MySql errors since I've switched to CakePHP. The Model class usually handles all the DB operations flawlessly and it's probably been over a year that I've written a custom MySql statement in my code somewhere. However, even CakePHP or, what's more likely, the database can fail or deny operations.
So in order to not get unpleasant / surprising results, one should always use a structure like this:
{
$this->Task->set($this->data);
if ($this->Task->validates())
{
if ($this->Task->save())
{
// Display a success message
}
else
{
// Display an error message that there was a save error
}
}
else
{
// Display an error message that validation failed
}
}
I don't know about you, but to me there is beauty missing. The code above is readable and understandable, no question about it. However, I think it can be replaced with something better. Now, if you look at the following code and your reaction goes along the line "same difference, who cares, etc." then I can understand that. In this case you might still find an interesting sequence of code so you didn't waste your time reading this. But I hope some of you can confirm my inner feeling regarding the beauty of the code.
Alright, here comes a code that does exactly what the code above does, just with what I'd call the beauty-factor ; ).
{
$result = $this->saveData();
switch ($result)
{
case 'success':
// Display a success message
break;
case 'validation_failed':
// Display an error message that validation failed
break;
case 'save_failed':
// Display an error message that validation failed
break;
}
}
So before I'll ask if anybody can confirm that I've not turned crazy, here comes the code that is required to make the one above work. First of all, you need an AppController function like the one below:
loadComponent('Common');
class AppController extends Controller
{
/**
* AppController::saveData is a generic save function that performs validations and saving for any given $data
* on a given $model. It returns a string which indicates the result of the operation. Possible result values
* are:
*
* 'missing_model', 'save_failed', 'validation_failed', 'success'
*
* @param mixed $data If skipped, Controller::data will be used
* @param mixed $model If skipped, Controller::modelClass will be used
* @return string
*/
function saveData($data = null, $model = null, $cleanUpFields = false)
{
// If $this->action has the name of this function, it was requested by the Dispatcher
if ($this->action=='saveData')
{
// Make sure this does not work
return false;
}
// The $data parameter defaults to $this->data if none was provided
Common::defaultTo($data, $this->data);
// The $model parameter defaults to $this->modelClass if none was provided
Common::defaultTo($model, $this->modelClass);
// If our $cleanUpFields variable is set to true
if ($cleanUpFields===true)
{
// Clean up this controllers fields
$this->cleanUpFields($model);
}
// If no $model parameter was given, but Controller::modelClass is available
if (isset($this->{$model}))
{
// Try to use the Model instance from our Controller
$Model =& $this->{$model};
}
else
{
// Try to load this Model using Common::getModel
$Model =& Common::getModel($model);
}
// If our $Model variable is no object
if (!is_object($Model))
{
// Return the 'missing_model' result value
return 'missing_model';
}
// Set our $Model::data
$Model->set($data);
// See if our $Model validates
if ($Model->validates())
{
// If we couldn't save our $Model $data
if (!$Model->save($data))
{
// Return the 'save_failed' result value
return 'save_failed';
}
}
else
{
// If it didn't validate, return the 'validation_failed' result value
return 'validation_failed';
}
// If everything worked out, return the 'success' result value
return 'success';
}
}
Ok, you might have noticed the usage of a class called Common in here. I was thinking to write about this in another post, but it fits in really well with this one, so here is what I use it for. One thing I often feel I don't know the right place for in CakePHP are generic functions that modify dates, restructure arrays or do similar things. In the past they usually ended up as "private" functions (the ones starting with '__' in CakePHP) in some controller. What I didn't like about this however, was that I couldn't share those across the application easily and that they just didn't seem to belong there in first place. So my recent solution to this problem was to create a class called 'Common' and place it in /app/components. Now technically it's not a real CakePHP component as it mainly serves as a name space and it's also not named 'CommonComponent'. However, I still think it's a good place to put this class, at least I couldn't think of a better one.
Alright, before I start to bore you, here comes the code for it. (You might remember my old friend the getModel function)
* This class serves as a namespace for functions that need to be globally available within this application.
* All of it's functions can be called statically, i.e. Common::defaultTo(...), etc.
*
* Warning: It's name violates against the CakePHP naming conventions which demand it to be named CommonComponent.
* For the sake of convenience I decided against the conventions in this case (also because it's not a component in
* the classic / CakePHP sense of things)
*/
class Common extends Object
{
/**
* Tries a couple of approaches to return an instance of a given $model. If none of them succeed,
* 'false' is returned instead.
*
* @param string $model
* @return mixed
*/
function &getModel($model)
{
// Make sure our $modelClass name is camelized
$modelClass = Inflector::camelize($model);
// If the Model class does not exist and we cannot load it
if (!class_exists($modelClass) && !loadModel($modelClass))
{
// Can't pass false directly because only variables can be passed via reference
$tmp = false;
// Return false
return $tmp;
}
// The $modelKey is the underscored $modelClass name for the ClassRegistry
$modelKey = Inflector::underscore($modelClass);
// If the ClassRegistry holds a reference to our Model
if (ClassRegistry::isKeySet($modelKey))
{
// Then make this our $ModelObj
$ModelObj =& ClassRegistry::getObject($modelKey);
}
else
{
// If no reference to our Model was found in trhe ClassRegistry, create our own one
$ModelObj =& new $modelClass();
// And add it to the class registry for the next time
ClassRegistry::addObject($modelKey, $ModelObj);
}
// Return the reference to our Model object
return $ModelObj;
}
/**
* Simple, yet very convenient function to set a given $variable to it's $defaultValue if it is empty
*
* @param mixed $variable
* @param mixed $defaultValue
*/
function defaultTo(&$variable, $defaultValue)
{
// If our $variable is empty
if (empty($variable))
{
// Overwrite it with it's $defaultValue
$variable = $defaultValue;
}
}
}
Alright, now you got all the code to get my initial sample from above running and the opportunity to question the state of my sanity ; ). For those of you who'd like to send me to a mental institute I got some rational arguments left that might be able to save me:
AppController::saveData ...
- ... makes it possible to save data for any given Model (even those not included in the current Controller) easily.
- ... always makes sure Model::save was successful
- ... makes the code even more readable (imho)
And last but not least it also provides you with an easier way to only save ModelB if saving ModelA (really) succeeded:
{
$success = false;
$user_saved = $this->saveData(null, 'User');
if ($user_saved=='success')
{
$profile_saved = $this->saveData(null, 'Profile');
if ($profile_saved=='success')
{
$success == true;
}
else
{
// Roll back our first DB operation
$this->User->delete();
}
}
if ($success==true)
{
// Display success message
}
else
{
// Use the contents of $user_saved and $profile_saved to display the appropiate error
}
}
Therefor I'd like to end this post by pleading innocent regarding eventual accusations regarding my sanity ; ).
-- Felix Geisendörfer aka the_undefined
Did you like this blog post? If so, please consider subscribing to the Blog RSS feed.
You can skip to the end and add a comment.
Daniel, i think the key element of felix' approach is that he wants one variable which contains information about the success of the save operation. There are 3 situations: complete success, validation failed, or validiation succeeded but a problem at database level arose (making the save action fail)
Personally i use code like in block #2 to do such things, which is fine, but felix wants it more beautifully. That's why he introduces all this code so he can store all the needed information about success or the multiple kinds of failure in 1 variable.
I think it's overkill, especially for an application. But i might be a nice addition for core functionality in the framework.
I like it but instead of 'success', 'validation_failed' and 'save_failed', it should be great to return int :
1 for success
-1 for validation_failed
-2 for save_failed
define('SAVE_DATA_SUCCESS', 1);
define('SAVE_DATA_VALIDATION_FAILED', -1);
define('SAVE_DATA_SAVE_FAILED', -2);
function add()
{
$result = $this->saveData();
switch ($result)
{
case SAVE_DATA_SUCCESS:
// Display a success message
break;
case SAVE_DATA_VALIDATION_FAILED:
// Display an error message that validation failed
break;
case 'SAVE_DATA_SAVE_FAILED:
// Display an error message that save failed
break;
}
}
So, it allow too :
function add()
{
$result = $this->saveData();
if($result)
{
// Display a success message
}else{
// Display an error message
}
}
define('SAVE_DATA_MISSING_MODEL', -3);
Nao: I'm not a huge fan of defining magic numbers / error codes as constants these days. Because if I use debug($result), I want to get something that a human-being can actually read and not a number ; ).
Yes, but error codes alow to make a simple if-else on it.
To make error "human friendly" with debug mode on, just add trigger error in your function :
// If we couldn't save our $Model $data
if (!$Model->save($data))
{
trigger_error(sprintf('Internal error on %s::%s(): Save failed, __class__, __FUNCTION__), E_USER_WARNING);
// Return the 'save_failed' result value
return SAVE_DATA_SAVE_FAILED;
}
I agree with Felix in that the second example is not very "beautiful" for the following reasons:
1. unneccessary indentation
2. Point of failure and corresponding error message aren't next to each other.
For this reason I write such code like this:
function add()
{
$this->Task->set($this->data);
if (!$this->Task->validates())
{
// Display an error message that validation failed
return;
}
if (!$this->Task->save())
{
// Display an error message that there was a save error
return;
}
// Display a success message
}
This has some advantages in comparison to variant 2:
1. It's less code, thus simpler, easier to read an maintain.
2. It is made clear that validation and save errors are exceptional conditions that usually don't occur; the main action of the method (in this case displaying a success message) is on the first indent level and not hidden inside a two-level if.
3. The disadvantages from above are eliminated.
Such a code layout of course assumes that you can do the return after every failure condition. I have found this to be true in practice.
Shouldn't this be in AppModel rather?
The advantage to put it in controller, is you need to ask question i model is loaded or not. Pass nothing for default model of controller or just pass it at string form, Felix's function automatically load it for you if necessary.
Now, it's possible to split it in two part, one for controller which clean up field and load model, and other part for model which valid and save.
So in your controller, you could do :
$this->saveData($data = null, $model = null, $fieldList = array(), $cleanUpFields = false);
or
$this->Model->save2($data = null, $validate = true, $fieldList = array());
Difference between Model::save and Model::save2 is save2 return nature of failed ('missing_model', 'save_failed', 'validation_failed', 'success').
Sorry : The advantage to put it in controller, is you DONT'T need to ask if model is loaded or not
Interesting idea, but it seems like blurring the lines between the controller and the model. Its also a bit restrictive as written: there are 3 parameters for save. At the very least (!$Model->save($data)) should be (!$Model->save($data,false)) to apply DRY to the executed code.
What situations are you considering whereby data would be valid but cause a db error..? Typically that only happens in development in my experience (as in, in an unexpected or uncontrolled manner). As it doesn't matter to the user (or quite often in the code, such as the last code example) why a submission didn't work, why not make use of the model onError method (admittedly introduced in 1.2 iirc) to log the problem and/or add to the model validation errors. Maybe cake will do that anyway before the next release.
Incidentally regarding the last example: Using transactions makes handling multiple updates much easier, and I never understood why $Model->query("START TRANSACTION;"); etc. wasn't the first thing thought of when the need for a transaction arises. Unless it's an app to be distributed, "problem solved".
PS. why not follow the cake style of having less new lines (i.e. "} else {" on a single line)?
Thanks for all the feedback. From all the suggestions above I really like Markus Bertheau's approach of using return to avoid too much nested if blocks. In fact I changed the code in my current application to this pattern right away.
Mladen Mihajlovic: I don't think it would belong in AppModel as it's Controller logic that is being dealt with here.
AD7Six: Yes, the other parameters for Model::save are not accessible with this method, that's a problem, I agree. About the question what could cause a db error? Well if you use a DB that has it's own validation rules built in it's very easy for a save command to fail. In those cases you certainly don't want to display a "Your item was added"-page to the user. Even if that is never the case in your it would still be sloppy programming to not check if the Model::save call actually worked. About transactions: I haven't really worked with them much in the past and just thought the saveData method would make for a good example of how to build "fake" transaction support. This was not ment as a transaction how-to. About your question regarding the braces style: I actually really hate the style that the cake core was switched to a while back. When I write code for the foundation or client projects that require it, I can life with it. But I would never ever use it for my own stuff ... it's a strong personal preference I have.
Again, thanks for every bodies feedback. Even so I'll probably go with Markus Bertheau's approach from now on, you might still find some of the posting interesting, even if it's just the little Common::defaultTo function ... ; ).
-- Felix
@Felix : I like too Markus Bertheau’s approach. You can do something very similar with your function, like this :
function add()
{
$result = $this->saveData();
switch ($result)
{
case VALIDATION_FAILED:
// Display an error message that validation failed
return;
case SAVE_FAILED:
// Display an error message that there was a save error
return;
}
// Display a success message
}
@AD7six :Too glue to Model::save, add $fieldList parameter to Controller::saveData
function saveData($data = null, $model = null, $fieldList = array(), $cleanUpFields = false)
{
...
if (!$Model->save($data, false, $fieldList))
...
Numeric return values?
That's soo 1972 (birth of C language) :-)
Here are the code conventions btw:
https://trac.cakephp.org/wiki/Developement/CodingStandards
Like felix, I like the previous ones better, but hey: convention over.. taste, right :-)
"it would still be sloppy programming to not check if the Model::save call actually worked"
I don't recall saying or suggesting that :)
In any event thanks for sharing ideas Felix, discussions are a great way to prompt innovation.
Dieter@be: Nobody forces you to use the CakePHP coding standards for your CakePHP projects. I think they are really good and people should use them, but the braces style is something I'll put my taste above them. So for my personal projects / things I publish on here I'll use the old style. For things I write for the foundation I'll play by the official rules ; ).
AD7six: Yeah, I think that discussions like this are always interesting and good stuff comes from them ; ).
Whilst using numeric constants such as:
define("SAVE_DATA_SUCCESS", 1);
isn't great for debugging purposes (what the hell does 1 mean again!), you've just replaced 'magic numbers' with 'magic strings', neither of which are great, they're both hard to remember and prone to errors.
Also you don't get any help from IDEs with code-completion.
Recently I've started to prefer:
define("SAVE_DATA_SUCCESS", "succeeded");
You get all the benefits of constants (code-completion, documentation of possible results in a single place) without the drawbacks of random numbers (harder debugging).
It's not a big deal, but since PHP supports strings in switch statements, it's always nice to do things you can't do in c/c++
c/c++ doesn't support strings in switch statements? i knew java didn't but c/c++ is new for me.
good to know that :-)
Felix Geisendorfer's Blog: Making error handling for Model::save more beautiful in CakePHP...
...
I think that using exceptions would be far more beautyfull.
Example:
try {
$this->User->set($this->data);
$this->User->save();
//handle successfull operation
} catch (CakeValidationException $e) {
// handle validation exception
} catch (CakeModelSaveException $e) {
// handle savemodel exception
} catch (Exception $e) {
// hadnle other exceptions
}
This post is too old. We do not allow comments here anymore in order to fight spam. If you have real feedback or questions for the post, please contact us.
Hm, to me it looks like you are trying to solve a problem where no problem is ;-) What I usually do is the following:
if ($this->User->save($this->data)) {
// success
} else {
// failure
}
And that works fine for me. But maybe I don't understand what you try to accomplish...