A clear all logs button
he_who_shall_no... - February 14, 2006 - 09:29
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | watchdog.module |
| Category: | feature request |
| Priority: | normal |
| Assigned: | stewsnooze |
| Status: | closed |
Description
A "clear all logs" button, available for the site administrators, is missing.
#1
#2
#3
The attached patch adds a clear button in a similar way it is present on the performance administration page. Note that it uses DELETE FROM and not TRUNCATE TABLE (which should be faster) because SQLite does not support the TRUNCATE TABLE command.
#4
I felt that the button should have explained what would happen so I've added just a few words of explanation.
#5
This seems redundant, considering that the two sentences can be easily combined into one:
+ '#description' => t('Remove all entries from the database log. This will permanently remove the log entries from the database log.'),#6
I think you are right. Rerolled with one sentence which reads.
"This will permanently remove the log entries from the database."
#7
#8
Just my 2p, does this merit a confirm_form as we're permanently deleting data?
#9
I think you are right. I'll do it tomorrow if I get chance.
#10
I've taken a shot at adding in a confirmation form, I'm not sure about the drupal_goto()'s I've used, maybe there's a better way but please take a look and let me know what you think.
Cheers,
Chris
#11
I would totally use this feature. Haven't reviewed the patch yet.
#12
That patch is good and works as expected e.t.c. I do feel slightly uncomfortable that the existing form only had one option and we've put a whole extra form in the middle of it which makes the UI look bad but the thing does work
#13
I have "truncate table watchdog" bookmarked in my SQL manager - so yes please!
#14
When I made the original patch I opted not to include a "confirm" form because the "clear cache" form doesn't use it either. It's only my second Drupal patch ever so I wanted to stay as close to existing implementations as possible. :-)
It's also something that I do often, right now from the MySQL interface, because I found myself dropping a lot of debug information into it when I developed the latest website I was working on and it would easily clutter the entire log database.
Secondly, I felt that, when you have the permissions to empty the log table, you most likely know what you are doing and accidentally truncating the table is not as "dangerous" as deleting a node or a user.
#15
I think where this differs is that when clearing a cache the data is essentially redundant and whilst on a high traffic site it would hurt your site you wouldn't lose data. This actually deletes data and I feel it is different.
#16
I side with the "no confirm" route.
I'll only be using it in testing, and I don't want a useless click there.
The 'data' this is deleting is about as trivial as data gets - in real life - in my opinion. I can't imagine regretting doing it.
Anyone messing around with the logs should know what they are doing, and it should be easy for them to do what they want. "Are You Sure" buttons are there to protect people who don't understand the consequences. A confirm page would be clutter to the folk that will ever use that button. Adding a confirm would make it less usable and useful for me.
But I'm not fussed.
#17
I take that point and am re uploading the patch from comment #6
#18
I've moved the clear form to below the save button as the original form was split in two by this change.
The UI here is better IMHO
#19
I think something went wrong here. Your patch in comment #18 also contains:
update.php:- $links[] = '<a href="' . base_path() . '">Main page</a>';
+ $links[] = '<a href="' . base_path() . '">Front page</a>';
#20
My bad. Removed the other stuff which is part of another ticket.
#21
I've added a test that will log on as admin and use the form to perform the clearing action. Patch rerolled accordingly.
Stew
#22
With the new DB syntax, you can write
db_delete('dblog')to clear the database in D7.Please rename
$count_afterto$count.I think you should simply write:
db_query('SELECT COUNT(*) FROM ...instead of doing theaddExpression('COUNT(*)')thing.You're also testing dblog_watchdog() -- I'm not sure there is an existing test but if not, it might be useful to rename the test function and to test dblog_watchdog() at the same time.
#23
#24
and replace :
+ //Test clearing of the dblog tablewith :
+ // Test clearing of the dblog table.#25
#24 lilou
done. Thanks
#22 dries
I've renamed $count_after to $count
db_delete('dblog') or db_delete('watchdog') just don't delete any rows for me. I've left the delete as
db_query("DELETE FROM {watchdog}");
Test function renamed to testDBLogAddAndClear() . Does an additional test to make sure that dblog_watchdog() actually adds a row to the db.
Is it that are we not interested in using the DBTNG stuff in the tests?
In the tests also swapped out the DBTNG with expression for a standard select count(*)
Are there already tests for db_delete???
#26
Thanks for you contribution, but the patch doesn't yet comply with our coding standards. Here is a review, keep up the good work!
Dries wrote "With the new DB syntax, you can write db_delete('dblog') to clear the database in D7.", which is slightly wrong: you *have to* use db_delete() to delete rows in D7. The syntax is: db_delete('dblog')->execute().
<?php+
+ // Test Addition and clearing of the dblog table
+ $this->testDBLogAddAndClear();
?>
This is neither needed nor useful. Functions prefixed with "test" are called automatically by the testing framework.
<?php+ $count = db_result(db_query('SELECT COUNT(*) FROM {watchdog}'));
?>
db_result() is deprecated. Please use ->fetchField().
<?php+ $log = array(
+ 'type' => 'custom',
+ 'message' => 'Log entry added to test the doClearTest clear down.',
+ 'variables' => array(),
+ 'severity' => WATCHDOG_NOTICE,
+ 'link' => NULL,
+ 'user' => $this->big_user,
+ 'request_uri' => $base_root . request_uri(),
+ 'referer' => $_SERVER['HTTP_REFERER'],
+ 'ip' => ip_address(),
+ 'timestamp' => REQUEST_TIME,
+ );
?>
The closing parenthesis should be aligned with $log.
<?php+ // Add a watchdog entry
+ dblog_watchdog($log);
?>
The comment needs to end with a proper period.
<?php+ $this->assertTrue($count+1 == db_result(db_query('SELECT COUNT(*) FROM {watchdog}')), t('dblog_watchdog added an entry to the dblog %count', array('%count' => $count)));
?>
- $count+1 should be $count + 1 per coding standards
- assertTrue() should be assertEqual()
- same remark about db_result()
- dblog_watchdog in the message should be spelled "dblog_watchdog()".
<?php+ // Now post to clear the db table
+ $edit = array();
+ $this->drupalPost('admin/settings/logging/dblog', $edit, t('Clear database logs'));
?>
- there are two spaces in the comment. Plus, it needs to end with a proper period.
- you can just put array() in $this->drupalPost() no need for the intermediate $edit
<?php+ // Count rows in watchdog that previously related to the deleted user.
+ $count = db_result(db_query('SELECT COUNT(*) FROM {watchdog}'));
+ $this->assertTrue($count == 0, t('DBLog contains %count records after a clear, %message', array('%count' => $count)));
?>
Same remark about assertTrue that should be assertEqual
<?php+ // drupalLogout() will put an entry into dblog so if checking for number of rows drupalLogout() must be done later.
+ $this->drupalLogout();
+ }
+ //test_dblog_watchdog()
?>
All those lines can go: you don't need to explicitely logout the user, the testing framework will take care of this for you, and we don't put comments in closing blocks.
#27
The
<?phpdb_delete('dblog')->execute();
?>
had to be
<?phpdb_delete('watchdog')->execute();
?>
Apart from that I implemented the recommendations in #26. Thanks
#28
I think this patch looks good functionally, but it is still somewhat borked from a usability point of view. The position of the button looks very weird (see screenshot).
Also, I want to wipe the logs when I'm navigating the logs. It seems like I'd want that button to show up on the logs page itself, not on the settings page. Maybe we should add it below the logs table?
#29
Attached is a patch (and a preview image) which adds a fieldset with a clear button above the list of log messages, below the "filter log messages" form. I felt that putting it below the list of messages wasn't really an UI improvement.
#30
That looks good to me. I don't feel qualified enough yet to push to RTBC but perhaps the bot will test it.
#31
Looks good to me as well. Good code, useful feature. Let's wait a little longer for feedback, in case there is any.
#32
Committed to CVS HEAD. Thanks j.somers! :)
#33
Automatically closed -- issue fixed for two weeks with no activity.