Imported from Trac ticket #882 reported by kris on 2009-12-22:
This refers to a type of design flaw in which exceptions are thrown to indicate failed validation of forms and/or models being saved.
Commonly-held design principles indicate that exceptions only be thrown for conditions that are uncommon or unlikely, e.g. failure to connect to the database, logical errors made by plugin or theme writers, etc. On the other hand, common or expected situations, such as forms failing to validate, should not result in thrown exceptions. For more information on why this is bad, google "exceptions control flow" and read some of the extensive literature on the topic.
Omeka violates this principle primarily with respect to validating forms and saving models to the database. At least two methods in Omeka_Record are designed to throw exceptions:
- Omeka_Record::saveForm() will throw an exception if the form submission failed to validate.
- Omeka_Record::forceSave() will throw an exception if the record failed to save properly.
In addition to that, most of Omeka's controller actions are designed to explicitly catch the exceptions thrown by these methods and use it to flash form validation errors.
For example, Omeka_Controller_Action::addAction() contains the following code:
try {
if ($record->saveForm($_POST)) {
$successMessage = $this->_getAddSuccessMessage($record);
if ($successMessage != '') {
$this->flashSuccess($successMessage);
}
$this->redirect->goto('browse');
}
} catch (Omeka_Validator_Exception $e) {
$this->flashValidationErrors($e);
} catch (Exception $e) {
$this->flashError($e->getMessage());
}
The problems with this design are many:
- always flashing an exception error message on a form, as opposed to allowing it to bubble up to the main error handler, makes it so that developers cannot debug the various types of exception messages that might arise.
- it always flashes every exception message back to the form when it may in fact be inappropriate to display these messages to users (security reasons or otherwise).
- exceptions that are being flashed back to the user are not being logged. If allowed to bubble up, these exceptions would be automatically logged.
The ideal solution would be something simpler, like:
if ($record->saveForm($_POST)) {
$successMessage = $this->_getAddSuccessMessage($record);
if ($successMessage != '') {
$this->flashSuccess($successMessage);
}
$this->redirect->goto('browse');
} else {
$this->flashError($record->getErrors());
}
Omeka_Record::forceSave() should continue to throw an exception when a record fails to save properly, but it should not be used to indicate failed form validation. If used properly, forceSave() is primarily a tool for developers to catch logic errors in plugins before such errors present to the user of the plugin. Following is an example of incorrect usage:
try {
$record->name = $_POST['name'];
$record->forceSave();
} catch (Exception $e) {
$this->flashValidationErrors($e->getMessage());
}
Here is a better example of how it might be used:
if (!$this->_validateName($_POST['name'])) {
$this->flashValidationErrors("Name is not valid.");
return;
}
// No validation errors can occur at this point.
// Only something catastrophic would cause this to fail.
$record->name = $_POST['name'];
$record->forceSave();
This is marked for 2.0 because changing the behavior of Omeka_Record and Omeka_Controller_Action will create incompatibility with existing plugins, including the way that plugins can currently invalidate a form submission by implementing a 'before_save_form' hook that throws an exception. There may be other incompatible changes required to implement this.