From d8377a1e3cf8f71ba87434cf541d4ff08ff659cb Mon Sep 17 00:00:00 2001 From: gBokiau Date: Tue, 18 Feb 2014 10:59:30 +0100 Subject: [PATCH 1/3] Add 'forceDelete' setting to CouplerBehavior If set to true, record deletion will proceed even if original media file was not found in baseDirectory. --- Model/Attachment.php | 3 ++- Model/Behavior/CouplerBehavior.php | 9 +++++++-- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/Model/Attachment.php b/Model/Attachment.php index dd081a0..3b721a5 100644 --- a/Model/Attachment.php +++ b/Model/Attachment.php @@ -52,7 +52,8 @@ class Attachment extends MediaAppModel { ), 'Media.Polymorphic', 'Media.Coupler' => array( - 'baseDirectory' => MEDIA_TRANSFER + 'baseDirectory' => MEDIA_TRANSFER, + 'forceDelete' => false ), 'Media.Meta' => array( 'level' => 2 diff --git a/Model/Behavior/CouplerBehavior.php b/Model/Behavior/CouplerBehavior.php index dc6edb8..89e64e1 100644 --- a/Model/Behavior/CouplerBehavior.php +++ b/Model/Behavior/CouplerBehavior.php @@ -57,11 +57,15 @@ class CouplerBehavior extends ModelBehavior { * * baseDirectory * An absolute path (with trailing slash) to a directory which will be stripped off the file path + * forceDelete + * If set to true, record deletion will proceed even if original media file was not + * found in baseDirectory. Defaults to false. This will not delete generated versions * * @var array */ protected $_defaultSettings = array( - 'baseDirectory' => MEDIA_TRANSFER + 'baseDirectory' => MEDIA_TRANSFER, + 'forceDelete' => false ); /** @@ -180,7 +184,8 @@ public function beforeDelete(Model $Model, $cascade = true) { $file .= DS . $result[$Model->alias]['basename']; $File = new File($file); - return $File->delete(); + $success = $File->delete(); + return ($this->settings[$Model->alias]['forceDelete']) ? true : $success; } /** From 26479a0b20ae08c0fc0a4ce4716b1abf19126cb3 Mon Sep 17 00:00:00 2001 From: gBokiau Date: Tue, 25 Feb 2014 10:35:15 +0100 Subject: [PATCH 2/3] Group deletion errors into single exception --- Model/Behavior/CouplerBehavior.php | 66 +++++++++++++++++++++++++----- 1 file changed, 56 insertions(+), 10 deletions(-) diff --git a/Model/Behavior/CouplerBehavior.php b/Model/Behavior/CouplerBehavior.php index 89e64e1..aea667c 100644 --- a/Model/Behavior/CouplerBehavior.php +++ b/Model/Behavior/CouplerBehavior.php @@ -19,6 +19,17 @@ App::uses('File', 'Utility'); App::uses('ModelBehavior', 'Model'); + +/** + * Exceptions + * To be moved upon consistent rewrite of error and exception reporting + */ + +class MediaException extends CakeException {} +class MediaDeleteException extends MediaException { + protected $_messageTemplate = 'Some Media file(s) could not be deleted : %s.'; +} + /** * Coupler Behavior Class * @@ -57,7 +68,7 @@ class CouplerBehavior extends ModelBehavior { * * baseDirectory * An absolute path (with trailing slash) to a directory which will be stripped off the file path - * forceDelete + * alwaysDeleteRecord * If set to true, record deletion will proceed even if original media file was not * found in baseDirectory. Defaults to false. This will not delete generated versions * @@ -65,9 +76,21 @@ class CouplerBehavior extends ModelBehavior { */ protected $_defaultSettings = array( 'baseDirectory' => MEDIA_TRANSFER, - 'forceDelete' => false + 'alwaysDeleteRecord' => false ); +/** + * File Deletion Errors + * + * If coupled files could not be deleted upon record deletion in beforeDelete(), + * the errors are stored here before being thrown as an exception in the afterDelete() + * callback. This is set to public to facilitate error reporting if the Exception is caught + * in the controller. + * + * @var Array + */ + public $deleteErrors = array(); + /** * Setup * @@ -158,9 +181,8 @@ public function beforeSave(Model $Model, $options = array()) { } /** - * Callback, deletes file (if there's one coupled) corresponding to record. If - * the file couldn't be deleted the callback will stop the delete operation and - * not continue to delete the record. + * Callback, deletes file (if there's one coupled) corresponding to record. + * See alwaysDeleteRecord setting above for how failure to delete the file is managed * * @param Model $Model Model using this behavior * @param boolean $cascade If true records that depend on this record will also be deleted @@ -182,12 +204,36 @@ public function beforeDelete(Model $Model, $cascade = true) { $file = $baseDirectory; $file .= $result[$Model->alias]['dirname']; $file .= DS . $result[$Model->alias]['basename']; - - $File = new File($file); - $success = $File->delete(); - return ($this->settings[$Model->alias]['forceDelete']) ? true : $success; + + if (!@unlink($file)) { + if (!array_key_exists($Model->alias, $this->deleteErrors)) { + $this->deleteErrors[$Model->alias] = array(); + } + $error = error_get_last(); + $this->deleteErrors[$Model->alias][$file] = $error['message']; + + if (!$this->settings[$Model->alias]['alwaysDeleteRecord']) { + // The Notice is triggered as a precaution, but in production it is + // advised to check Coupler->deleteErrors when a record fails to be deleted + // to see if the error originated here and why + trigger_error($error['message'], E_USER_NOTICE); + return false; + } + } + return true; + } + +/** + * Callback, throws Exception if files could not be deleted + * + */ + public function afterDelete(Model $Model) { + if (array_key_exists($Model->alias, $this->deleteErrors)) { + if (count($errors = $this->deleteErrors[$Model->alias])) { + throw new MediaDeleteException(array('files' => join(array_values($errors), ', '))); + } + } } - /** * Callback, adds the `file` field to each result. * From 6f2de34896a3bde3c59890e98f50f0d0fa1b7640 Mon Sep 17 00:00:00 2001 From: gBokiau Date: Wed, 26 Feb 2014 12:47:33 +0100 Subject: [PATCH 3/3] convenience method to access deletion errors --- Model/Behavior/CouplerBehavior.php | 39 ++++++++++++++++++++---------- 1 file changed, 26 insertions(+), 13 deletions(-) diff --git a/Model/Behavior/CouplerBehavior.php b/Model/Behavior/CouplerBehavior.php index aea667c..d10153b 100644 --- a/Model/Behavior/CouplerBehavior.php +++ b/Model/Behavior/CouplerBehavior.php @@ -84,12 +84,11 @@ class CouplerBehavior extends ModelBehavior { * * If coupled files could not be deleted upon record deletion in beforeDelete(), * the errors are stored here before being thrown as an exception in the afterDelete() - * callback. This is set to public to facilitate error reporting if the Exception is caught - * in the controller. + * callback. Errors can be accessed with deletionErrors() method. * * @var Array */ - public $deleteErrors = array(); + protected $_deleteErrors = array(); /** * Setup @@ -206,16 +205,17 @@ public function beforeDelete(Model $Model, $cascade = true) { $file .= DS . $result[$Model->alias]['basename']; if (!@unlink($file)) { - if (!array_key_exists($Model->alias, $this->deleteErrors)) { - $this->deleteErrors[$Model->alias] = array(); + if (!array_key_exists($Model->alias, $this->_deleteErrors)) { + $this->_deleteErrors[$Model->alias] = array(); } $error = error_get_last(); - $this->deleteErrors[$Model->alias][$file] = $error['message']; + $this->_deleteErrors[$Model->alias][$file] = $error['message']; if (!$this->settings[$Model->alias]['alwaysDeleteRecord']) { // The Notice is triggered as a precaution, but in production it is - // advised to check Coupler->deleteErrors when a record fails to be deleted - // to see if the error originated here and why + // advised to use $Model->Behaviors->Coupler->deletionErrors() + // when a record fails to be deleted to see if the error + // originated here and why trigger_error($error['message'], E_USER_NOTICE); return false; } @@ -228,10 +228,8 @@ public function beforeDelete(Model $Model, $cascade = true) { * */ public function afterDelete(Model $Model) { - if (array_key_exists($Model->alias, $this->deleteErrors)) { - if (count($errors = $this->deleteErrors[$Model->alias])) { - throw new MediaDeleteException(array('files' => join(array_values($errors), ', '))); - } + if ($errors = $this->deletionErrors($Model)) { + throw new MediaDeleteException(array('files' => join(array_values($errors), ', '))); } } /** @@ -277,5 +275,20 @@ public function checkRepresent(Model $Model, $check) { $value = current($check); return !empty($value); } - +/** + * Checks if errors were encountered while trying to delete files coupled with records + * + * @param Model $Model Model using this behavior + * @return mixed false if no errors occured, an array containg files that failed to delete + * with associated errors in the format + * array( + * '/path/to/file' => 'Full php error message' + * ) + */ + public function deletionErrors(Model $Model) { + if (array_key_exists($Model->alias, $this->_deleteErrors) && count($errors = $this->_deleteErrors[$Model->alias])) { + return $errors; + } + return false; + } }