My site has lots of directories. The function logs too much garbage: for me, successful deletion is not important enough event to log it every time. IMO we should only log unsuccessful deletions, and provide single line summary for success ones.
The bug makes provision log so big that browser hangs for while when I open it in Aegir interface.

CommentFileSizeAuthor
#6 prevent-recursive-delete-notices.patch1.41 KBchertzog
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

crea’s picture

Title: _provision_recursive_delete() should not log successful deletion » _provision_recursive_delete() should not log individual successful deletions
crea’s picture

Quick & dirty solution:

function _provision_recursive_delete($path, $log_success = TRUE) {
  $ret = 1;
  // is_dir() follows symlinks, so it can return true on a symlink
  if (is_dir($path) && !is_link($path)) {
    $d = dir($path);
    while (($entry = $d->read()) !== FALSE) {
      if ($entry == '.' || $entry == '..') {
        continue;
      }
      $entry_path = $path .'/'. $entry;
      $ret &= _provision_recursive_delete($entry_path, FALSE);
    }
    $d->close();

    provision_file()->rmdir($path);
    if ($log_success) {
      provision_file()->succeed('Deleting @path directory successful.');
    }
    $rm = provision_file()->fail('Deleting @path directory failed.')->status();

    $ret = $ret && $rm;
  }
  else {
    $rm = provision_file()->unlink($path)
      ->fail('Deleting @path file failed.')
      ->status();
    $ret = $ret && $rm;
  }
  return $ret;
}

This way, success is only logged for top level directory.

Steven Jones’s picture

Status: Active » Closed (won't fix)

I think it's better to fully log the deletion of all directories, and no-one else has complained about this so marking as won't fix.

crea’s picture

Oh this is one of the reasons I forked Aegir for my own use and no longer report issues here.
Reasonable, sane requests are ignored at best.

The patch wouldn't harm anyone. It only removes notice-level debug messages noone cares about anyway, and it fixes problems for big sites with lots of directories. The fact that it wasn't requested before doesn't mean the problem doesn't exist.
Btw, in all good software notice-level debug messages are not logged by default.

Steven Jones’s picture

Version: 6.x-1.x-dev » 6.x-2.x-dev
Status: Closed (won't fix) » Active

Okay okay, fair enough. I'm just trying to thin down the issue queue to something more manageable. Would you be able to provide a patch for this?
I can see why this might be useful both ways, so would commit something that would set the logging level to something less verbose by default.

chertzog’s picture

Status: Active » Needs review
FileSize
1.41 KB

Here is a patch with the code from #2

anarcat’s picture

Status: Needs review » Needs work
+++ b/provision.incundefined
@@ -202,26 +202,25 @@ function provision_save_platform_data() {
     $d = dir($path);
-    if (!empty($d)) {

hmmm... that's not right. i believe that !empty() is actually important error checking.

anyways, if we want to introduce changes in the verbose mode, we shouldn't touch anything else - those changes here seem to be unrelated...

anarcat’s picture

Issue summary: View changes

correction

ergonlogic’s picture

Version: 6.x-2.x-dev » 7.x-3.x-dev
Issue summary: View changes

New features need to be implemented in Aegir 3.x, then we can consider back-porting to Aegir 2.x.