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.
Log messages can now be cleared with a single click on the 'Clear log messages' button. The functionality is great, however, I think we should be consistent without ourselves and present a confirm screen before actually removing the log messages. We do this throughout the rest of Drupal, I expect the same here.
I filed it as a bug. This is not a potential CSRF vulnerability, however I sometimes hit enter by accident and the clear log messages button was triggered and everything was gone while I really didn't want that :)
Comment | File | Size | Author |
---|---|---|---|
#44 | dblog-936704-44.patch | 5.22 KB | tim.plunkett |
Comments
Comment #1
swentel CreditAttribution: swentel commentedBetter patch, was missing some documentation on the dblog_clear_log() function.
Comment #2
swentel CreditAttribution: swentel commentedOf course, this first two failed because dblog.test pointed to wrong path for clearing log messages.
Comment #3
Damien Tournoud CreditAttribution: Damien Tournoud commentedActually, most (all) of our action buttons on admin pages don't ask for confirmation. This would be inconsistent with the rest.
Comment #4
swentel CreditAttribution: swentel commentedSaving data isn't something requiring a confirmation page of course, deleting (interesting) data does imho. Deleting roles, fields, uninstalling modules all require confirmation because you are losing data, so I think it's valid :)
Comment #5
yoroy CreditAttribution: yoroy commentedYup, destructive operations all use a confirmation and it'd be good to be consistent with that.
Comment #6
swentel CreditAttribution: swentel commentedAssigning and marking rtbc, it's up to the core comitters to decide what todo with it.
Comment #7
Dries CreditAttribution: Dries commentedInstead of doing a redirect, wouldn't it be better to set '#action'? Something like:
$form['#action'] = url('admin/reports/dblog/clear');
Comment #8
swentel CreditAttribution: swentel commentedMakes sense, less code, always nice :)
Comment #9
mattyoung CreditAttribution: mattyoung commented~
Comment #10
Jody LynnComment #11
swentel CreditAttribution: swentel commentedReroll to new core structure.
Comment #13
swentel CreditAttribution: swentel commentedBetter one
Comment #15
swentel CreditAttribution: swentel commented#13: 936704-13.patch queued for re-testing.
Comment #16
Matt V. CreditAttribution: Matt V. commentedI had trouble getting #13 to apply cleanly on the latest 8.x, so I re-rolled it.
The only issue I'm noticing after the re-roll is that the redirect back to 'admin/reports/dblog' isn't working once the log gets cleared; instead, it's staying on the 'admin/reports/dblog/clear' page with the confirmation button.
Comment #17
Matt V. CreditAttribution: Matt V. commentedHere's an updated patch that fixes the redirect issue.
Comment #18
Matt V. CreditAttribution: Matt V. commented17: confirm_clear-936704-17.patch queued for re-testing.
Comment #20
Matt V. CreditAttribution: Matt V. commentedHere's an updated patch, that resolves the merge conflicts. I ran the "DBLOG FUNCTIONALITY" tests on it locally and they all passed.
Comment #22
longwaveThis should probably be postponed until #2112675: Convert dblog_filter_form and dblog_clear_log_form to FormInterface, as this should use a new-style form class.
Comment #23
jimi-o CreditAttribution: jimi-o commentedPatch for update form API
Comment #25
Matt V. CreditAttribution: Matt V. commentedI've rerolled the patch and also updated the tests and some of the formatting.
Comment #27
Jalandhar CreditAttribution: Jalandhar commentedUpdating patch with reroll and required changes.
Please review.
Comment #29
Matt V. CreditAttribution: Matt V. commentedI was able to get the patch in #27 to apply, but I couldn't get it to work. Instead, I backtracked to the patch in comment #25 and rerolled that. I'm attaching a patch that appears to work and pass all the tests.
The only thing I noticed is that after clicking "OK" to confirm the deletion of the logs, the user isn't redirected back to /admin/reports/dblog and instead stays on /admin/reports/dblog/confirm. The logs do get cleared though.
Comment #30
Matt V. CreditAttribution: Matt V. commentedHere's an updated patch that adds the redirect back to /admin/reports/dblog after the logs have successfully been cleared.
Comment #31
tim.plunkettThis can just be
_form: Drupal\dblog\Form\DblogClearLogConfirmForm
, and skip the whole controller.But, do we actually want to do this?
When I'm using dblog, I'm on my local dev, and I want to get things done. Clicking clear should just clear, IMO.
Comment #32
Matt V. CreditAttribution: Matt V. commentedHere's an updated patch that incorporates tim.plunkett's suggested change to the routing.
As for whether we want to do this, it definitely makes sense to me. I came to find and work on this patch after I accidentally cleared the logs on a production server when I meant to click the button to reset the filter. I had been using a browser with Javascript disabled, which caused the collapsible fieldsets on the page to be open, making the "Clear log messages" easier to mistake for the "Reset" button. I was in a hurry to debug a problem and I effectively cleared out the very information I needed to work with.
You might ask what sane person tries to use a browser without Javascript enabled or you might say that I shouldn't be running dblog on a production server, but I still say that it shouldn't be that easy to accidentally destroy data. We already require confirmation before deleting nodes, which in my mind sets the expectation that destructive actions are going to require confirmation. If you still want one-step deletion of the logs on your local development environment, that should be easy to accomplish with a quick query through Drush.
Comment #33
Matt V. CreditAttribution: Matt V. commentedI had someone test the patch yesterday at the code sprint and it wasn't applying cleanly, apparently due to the changes in #2247991: [May 27] Move all module code from …/lib/Drupal/… to …/src/… for PSR-4. I rerolled the patch again, made the necessary changes, and also cleaned up some issues pointed out by the Coder module. Here's an update patch.
Comment #35
Matt V. CreditAttribution: Matt V. commented33: confirm_delete_dblog-936704-33.patch queued for re-testing.
Comment #37
Matt V. CreditAttribution: Matt V. commentedHere's another try with some corrected paths and cleaned up spacing.
Comment #38
Matt V. CreditAttribution: Matt V. commentedComment #39
tim.plunkettCleaned this up a little bit, there were some methods overridden that didn't need to be, a leftover controller method, and the tests were cheating by bouncing to the confirm form directly.
Comment #40
longwaveCapitalisation looks a bit off here.
DblogClearLogConfirmForm or DblogClearConfirmLogForm? :)
Comment #41
Matt V. CreditAttribution: Matt V. commentedHere's an updated patch that addresses the issues longwave pointed out.
Comment #42
longwaveLooks good to me now.
Comment #44
tim.plunkettRerolled after #2267053: breadcrumb is wrong on single log event pages.
Comment #45
alexpottCommitted f0cb85a and pushed to 8.x. Thanks!
Comment #47
David_Rothstein CreditAttribution: David_Rothstein commentedComment #48
Anonymous (not verified) CreditAttribution: Anonymous commentedWell, in reality this "fix" is really annoying. The button is hidden within a collapsed fieldset(or detail in d8) and that(click to un-collapse) IMHO is sufficient 'middle step', if you will, between wanting to perform an anction and actually performing the action.