From 65c7ab7a9362dcc623132d799553795d7247af54 Mon Sep 17 00:00:00 2001
From: Laurent Destailleur <eldy@destailleur.fr>
Date: Fri, 29 May 2015 17:11:42 +0200
Subject: [PATCH] Fix: Introduce error by default when function is used as a
true delete function.
---
htdocs/admin/mails.php | 2 +-
htdocs/admin/tools/purge.php | 2 +-
htdocs/compta/facture.php | 6 ++----
htdocs/contrat/card.php | 6 ++----
htdocs/core/class/hookmanager.class.php | 13 +++++++------
htdocs/core/lib/files.lib.php | 18 ++++++++++--------
.../tpl/document_actions_pre_headers.tpl.php | 7 ++-----
htdocs/install/upgrade2.php | 2 +-
test/phpunit/FilesLibTest.php | 11 ++++++++---
9 files changed, 34 insertions(+), 33 deletions(-)
diff --git a/htdocs/admin/mails.php b/htdocs/admin/mails.php
index 2b50008d58a..53937687b57 100644
--- a/htdocs/admin/mails.php
+++ b/htdocs/admin/mails.php
@@ -121,7 +121,7 @@ if (! empty($_POST['removedfile']) || ! empty($_POST['removedfilehtml']))
$pathtodelete=$listofpaths[$keytodelete];
$filetodelete=$listofnames[$keytodelete];
$result = dol_delete_file($pathtodelete,1);
- if ($result >= 0)
+ if ($result)
{
setEventMessage($langs->trans("FileWasRemoved"), $filetodelete);
diff --git a/htdocs/admin/tools/purge.php b/htdocs/admin/tools/purge.php
index 9003a92cddf..e40caa3e36e 100644
--- a/htdocs/admin/tools/purge.php
+++ b/htdocs/admin/tools/purge.php
@@ -89,7 +89,7 @@ if ($action=='purge' && ! preg_match('/^confirm/i',$choice) && ($choice != 'allf
// If (file that is not logfile) or (if logfile with option logfile)
if ($filesarray[$key]['fullname'] != $filelog || $choice=='logfile')
{
- $count+=dol_delete_file($filesarray[$key]['fullname']);
+ $count+=(dol_delete_file($filesarray[$key]['fullname'])?1:0);
}
}
}
diff --git a/htdocs/compta/facture.php b/htdocs/compta/facture.php
index 88d6bb587e4..f765284f27e 100644
--- a/htdocs/compta/facture.php
+++ b/htdocs/compta/facture.php
@@ -1759,10 +1759,8 @@ if (empty($reshook))
$upload_dir = $conf->facture->dir_output;
$file = $upload_dir . '/' . GETPOST('file');
$ret = dol_delete_file($file, 0, 0, 0, $object);
- if ($ret)
- setEventMessage($langs->trans("FileWasRemoved", GETPOST('urlfile')));
- else
- setEventMessage($langs->trans("ErrorFailToDeleteFile", GETPOST('urlfile')), 'errors');
+ if ($ret) setEventMessage($langs->trans("FileWasRemoved", GETPOST('urlfile')));
+ else setEventMessage($langs->trans("ErrorFailToDeleteFile", GETPOST('urlfile')), 'errors');
$action = '';
}
}
diff --git a/htdocs/contrat/card.php b/htdocs/contrat/card.php
index dd48b27684f..192a2d16b17 100644
--- a/htdocs/contrat/card.php
+++ b/htdocs/contrat/card.php
@@ -823,10 +823,8 @@ else if ($action == 'remove_file' && $user->rights->contrat->creer) {
$upload_dir = $conf->contrat->dir_output;
$file = $upload_dir . '/' . GETPOST('file');
$ret = dol_delete_file($file, 0, 0, 0, $object);
- if ($ret)
- setEventMessage($langs->trans("FileWasRemoved", GETPOST('file')));
- else
- setEventMessage($langs->trans("ErrorFailToDeleteFile", GETPOST('file')), 'errors');
+ if ($ret) setEventMessage($langs->trans("FileWasRemoved", GETPOST('file')));
+ else setEventMessage($langs->trans("ErrorFailToDeleteFile", GETPOST('file')), 'errors');
}
}
diff --git a/htdocs/core/class/hookmanager.class.php b/htdocs/core/class/hookmanager.class.php
index 47810e204bd..ee7742660c1 100644
--- a/htdocs/core/class/hookmanager.class.php
+++ b/htdocs/core/class/hookmanager.class.php
@@ -116,9 +116,9 @@ class HookManager
* @param array $parameters Array of parameters
* @param Object $object Object to use hooks on
* @param string $action Action code on calling page ('create', 'edit', 'view', 'add', 'update', 'delete'...)
- * @return mixed For doActions,formObjectOptions,pdf_xxx: Return 0 if we want to keep standard actions, >0 if we want to stop standard actions, <0 means KO.
- * For printSearchForm,printLeftBlock,printTopRightMenu,formAddObjectLine,...: Return HTML string. TODO Deprecated. Must always return an int and things to print into ->resprints.
- * Can also return some values into an array ->results.
+ * @return mixed For 'addreplace hooks (doActions,formObjectOptions,pdf_xxx,...): Return 0 if we want to keep standard actions, >0 if we want to stop standard actions, <0 if KO.
+ * For 'output' hooks (printLeftBlock, formAddObjectLine, formBuilddocOptions, ...): Return 0, <0 if KO. Things to print are returned into ->resprints and set into ->resPrint.
+ * All types can also return some values into an array ->results.
* $this->error or this->errors are also defined by class called by this function if error.
*/
function executeHooks($method, $parameters=false, &$object='', &$action='')
@@ -135,7 +135,8 @@ class HookManager
array(
'addMoreActionsButtons',
'addStatisticLine',
- 'doActions',
+ 'deleteFile',
+ 'doActions',
'formCreateThirdpartyOptions',
'formObjectOptions',
'formattachOptions',
@@ -147,7 +148,7 @@ class HookManager
'formatEvent'
)
)) $hooktype='addreplace';
- // Deprecated hook types
+ // Deprecated hook types ('returnvalue')
if (preg_match('/^pdf_/',$method) && $method != 'pdf_writelinedesc') $hooktype='returnvalue'; // pdf_xxx except pdf_writelinedesc are 'returnvalue' hooks. When there is 2 hooks of this type, only last one win. TODO Move them into 'output' or 'addreplace' hooks.
if ($method == 'insertExtraFields')
{
@@ -195,7 +196,7 @@ class HookManager
if (isset($actionclassinstance->results) && is_array($actionclassinstance->results)) $this->resArray =array_merge($this->resArray, $actionclassinstance->results);
if (! empty($actionclassinstance->resprints)) $this->resPrint.=$actionclassinstance->resprints;
}
- // Generic hooks that return a string or array (printSearchForm, printLeftBlock, formAddObjectLine, formBuilddocOptions, ...)
+ // Generic hooks that return a string or array (printLeftBlock, formAddObjectLine, formBuilddocOptions, ...)
else
{
// TODO. this should be done into the method of hook by returning nothing
diff --git a/htdocs/core/lib/files.lib.php b/htdocs/core/lib/files.lib.php
index daae392cd39..05fdcc96624 100644
--- a/htdocs/core/lib/files.lib.php
+++ b/htdocs/core/lib/files.lib.php
@@ -793,12 +793,12 @@ function dol_move_uploaded_file($src_file, $dest_file, $allowoverwrite, $disable
/**
* Remove a file or several files with a mask
*
- * @param string $file File to delete or mask of file to delete
- * @param int $disableglob Disable usage of glob like *
+ * @param string $file File to delete or mask of files to delete
+ * @param int $disableglob Disable usage of glob like * so function is an exact delete function that will return error if no file found
* @param int $nophperrors Disable all PHP output errors
* @param int $nohook Disable all hooks
* @param object $object Current object in use
- * @return boolean True if file is deleted (or if glob is used and there's nothing to delete), False if error
+ * @return boolean True if no error (file is deleted or if glob is used and there's nothing to delete), False if error
*/
function dol_delete_file($file,$disableglob=0,$nophperrors=0,$nohook=0,$object=null)
{
@@ -823,19 +823,20 @@ function dol_delete_file($file,$disableglob=0,$nophperrors=0,$nohook=0,$object=n
$reshook=$hookmanager->executeHooks('deleteFile', $parameters, $object);
}
- if (empty($nohook) && isset($reshook) && $reshook != '') // 0:not deleted, 1:deleted, null or '' for bypass
+ if (empty($nohook) && $reshook != 0) // reshook = 0 to do standard actions, 1 = ok, -1 = ko
{
- return $reshook;
+ if ($reshook < 0) return false;
+ return true;
}
else
{
$error=0;
//print "x".$file." ".$disableglob;exit;
- $ok=true;
$file_osencoded=dol_osencode($file); // New filename encoded in OS filesystem encoding charset
if (empty($disableglob) && ! empty($file_osencoded))
{
+ $ok=true;
$globencoded=str_replace('[','\[',$file_osencoded);
$globencoded=str_replace(']','\]',$globencoded);
$listofdir=glob($globencoded);
@@ -853,6 +854,7 @@ function dol_delete_file($file,$disableglob=0,$nophperrors=0,$nohook=0,$object=n
}
else
{
+ $ok=false;
if ($nophperrors) $ok=@unlink($file_osencoded);
else $ok=unlink($file_osencoded);
if ($ok) dol_syslog("Removed file ".$file_osencoded, LOG_DEBUG);
@@ -958,9 +960,9 @@ function dol_delete_preview($object)
if (file_exists($file) && is_writable($file))
{
- if ( ! dol_delete_file($file,1) )
+ if (! dol_delete_file($file,1))
{
- $object->error=$langs->trans("ErrorFailedToOpenFile",$file);
+ $object->error=$langs->trans("ErrorFailedToDeleteFile",$file);
return 0;
}
}
diff --git a/htdocs/core/tpl/document_actions_pre_headers.tpl.php b/htdocs/core/tpl/document_actions_pre_headers.tpl.php
index 36b6ce75fe1..40bd0a5368e 100644
--- a/htdocs/core/tpl/document_actions_pre_headers.tpl.php
+++ b/htdocs/core/tpl/document_actions_pre_headers.tpl.php
@@ -59,11 +59,8 @@ if ($action == 'confirm_deletefile' && $confirm == 'yes')
if ($urlfile)
{
$ret = dol_delete_file($file, 0, 0, 0, $object);
- if ($ret) {
- setEventMessage($langs->trans("FileWasRemoved", $urlfile));
- } else {
- setEventMessage($langs->trans("ErrorFailToDeleteFile", $urlfile), 'errors');
- }
+ if ($ret) setEventMessage($langs->trans("FileWasRemoved", $urlfile));
+ else setEventMessage($langs->trans("ErrorFailToDeleteFile", $urlfile), 'errors');
}
elseif ($linkid)
{
diff --git a/htdocs/install/upgrade2.php b/htdocs/install/upgrade2.php
index 70ebd8550c5..913b21bdd14 100644
--- a/htdocs/install/upgrade2.php
+++ b/htdocs/install/upgrade2.php
@@ -3680,7 +3680,7 @@ function migrate_delete_old_files($db,$langs,$conf)
print ' '.$langs->trans("RemoveItManuallyAndPressF5ToContinue").'</div>';
}
else
- {
+ {
//print $langs->trans("FileWasRemoved",$filetodelete);
}
}
diff --git a/test/phpunit/FilesLibTest.php b/test/phpunit/FilesLibTest.php
index 4477784ae7d..e9fb713b857 100644
--- a/test/phpunit/FilesLibTest.php
+++ b/test/phpunit/FilesLibTest.php
@@ -339,10 +339,15 @@ class FilesLibTest extends PHPUnit_Framework_TestCase
print __METHOD__." result=".$result."\n";
$this->assertTrue($result,'delete file');
- // Again to test no error when deleteing a non existing file
- $result=dol_delete_file($conf->admin->dir_temp.'/file2.csv');
+ // Again to test there is error when deleting a non existing file with option disableglob
+ $result=dol_delete_file($conf->admin->dir_temp.'/file2.csv',1,1);
+ print __METHOD__." result=".$result."\n";
+ $this->assertFalse($result,'delete file that does not exists with disableglo must return ko');
+
+ // Again to test there is no error when deleting a non existing file without option disableglob
+ $result=dol_delete_file($conf->admin->dir_temp.'/file2.csv',0,1);
print __METHOD__." result=".$result."\n";
- $this->assertTrue($result,'delete file that does not exists');
+ $this->assertTrue($result,'delete file that does not exists without disabling glob must return ok');
// Test copy with special char / delete with blob
$result=dol_copy($file, $conf->admin->dir_temp.'/file with [x] and é.csv',0,1);
--
GitLab