Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
A "clear all logs" button, available for the site administrators, is missing.
Comment | File | Size | Author |
---|---|---|---|
#29 | jsomers_49333_image.png | 10.24 KB | j.somers |
#29 | jsomers_49333_clearlog.patch | 3.24 KB | j.somers |
#28 | clear.png | 42.67 KB | Dries |
#27 | stewsnooze_49333_clearlog_27.patch | 3.35 KB | stewsnooze |
#25 | stewsnooze_49333_clearlog_25.patch | 3.8 KB | stewsnooze |
Comments
Comment #1
magico CreditAttribution: magico commentedComment #2
LAs4n CreditAttribution: LAs4n commentedComment #3
j.somers CreditAttribution: j.somers commentedThe 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.
Comment #4
stewsnoozeI felt that the button should have explained what would happen so I've added just a few words of explanation.
Comment #5
keith.smith CreditAttribution: keith.smith commentedThis seems redundant, considering that the two sentences can be easily combined into one:
Comment #6
stewsnoozeI think you are right. Rerolled with one sentence which reads.
"This will permanently remove the log entries from the database."
Comment #7
stewsnoozeComment #8
matason CreditAttribution: matason commentedJust my 2p, does this merit a confirm_form as we're permanently deleting data?
Comment #9
stewsnoozeI think you are right. I'll do it tomorrow if I get chance.
Comment #10
matason CreditAttribution: matason commentedI'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
Comment #11
Dries CreditAttribution: Dries commentedI would totally use this feature. Haven't reviewed the patch yet.
Comment #12
stewsnoozeThat 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
Comment #13
dman CreditAttribution: dman commentedI have "truncate table watchdog" bookmarked in my SQL manager - so yes please!
Comment #14
j.somers CreditAttribution: j.somers commentedWhen 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.
Comment #15
stewsnoozeI 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.
Comment #16
dman CreditAttribution: dman commentedI 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.
Comment #17
stewsnoozeI take that point and am re uploading the patch from comment #6
Comment #18
stewsnoozeI'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
Comment #19
j.somers CreditAttribution: j.somers commentedI think something went wrong here. Your patch in comment #18 also contains:
Comment #20
stewsnoozeMy bad. Removed the other stuff which is part of another ticket.
Comment #21
stewsnoozeI've added a test that will log on as admin and use the form to perform the clearing action. Patch rerolled accordingly.
Stew
Comment #22
Dries CreditAttribution: Dries commentedWith the new DB syntax, you can write
db_delete('dblog')
to clear the database in D7.Please rename
$count_after
to$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.
Comment #23
stewsnoozeComment #24
lilou CreditAttribution: lilou commentedand replace :
+ //Test clearing of the dblog table
with :
+ // Test clearing of the dblog table.
Comment #25
stewsnooze#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???
Comment #26
Damien Tournoud CreditAttribution: Damien Tournoud commentedThanks 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().
This is neither needed nor useful. Functions prefixed with "test" are called automatically by the testing framework.
db_result() is deprecated. Please use ->fetchField().
The closing parenthesis should be aligned with $log.
The comment needs to end with a proper period.
- $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()".
- 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
Same remark about assertTrue that should be assertEqual
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.
Comment #27
stewsnoozeThe
had to be
Apart from that I implemented the recommendations in #26. Thanks
Comment #28
Dries CreditAttribution: Dries commentedI 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?
Comment #29
j.somers CreditAttribution: j.somers commentedAttached 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.
Comment #30
stewsnoozeThat looks good to me. I don't feel qualified enough yet to push to RTBC but perhaps the bot will test it.
Comment #31
Dries CreditAttribution: Dries commentedLooks good to me as well. Good code, useful feature. Let's wait a little longer for feedback, in case there is any.
Comment #32
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks j.somers! :)