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 :)

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

swentel’s picture

FileSize
2.64 KB

Better patch, was missing some documentation on the dblog_clear_log() function.

swentel’s picture

FileSize
3.55 KB

Of course, this first two failed because dblog.test pointed to wrong path for clearing log messages.

Damien Tournoud’s picture

Actually, most (all) of our action buttons on admin pages don't ask for confirmation. This would be inconsistent with the rest.

swentel’s picture

Saving 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 :)

yoroy’s picture

Yup, destructive operations all use a confirmation and it'd be good to be consistent with that.

swentel’s picture

Assigned: Unassigned » swentel
Category: bug » feature
Status: Needs review » Reviewed & tested by the community

Assigning and marking rtbc, it's up to the core comitters to decide what todo with it.

Dries’s picture

Instead of doing a redirect, wouldn't it be better to set '#action'? Something like:

$form['#action'] = url('admin/reports/dblog/clear');

swentel’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
3.6 KB

Makes sense, less code, always nice :)

mattyoung’s picture

~

Jody Lynn’s picture

Version: 7.x-dev » 8.x-dev
swentel’s picture

FileSize
3.1 KB

Reroll to new core structure.

Status: Needs review » Needs work

The last submitted patch, 936704-11.patch, failed testing.

swentel’s picture

Status: Needs work » Needs review
FileSize
3.63 KB

Better one

Status: Needs review » Needs work

The last submitted patch, 936704-13.patch, failed testing.

swentel’s picture

Status: Needs work » Needs review

#13: 936704-13.patch queued for re-testing.

Matt V.’s picture

Status: Needs review » Needs work
FileSize
3.75 KB

I 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.

Matt V.’s picture

Status: Needs work » Needs review
FileSize
3.76 KB

Here's an updated patch that fixes the redirect issue.

Matt V.’s picture

17: confirm_clear-936704-17.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 17: confirm_clear-936704-17.patch, failed testing.

Matt V.’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
3.78 KB

Here's an updated patch, that resolves the merge conflicts. I ran the "DBLOG FUNCTIONALITY" tests on it locally and they all passed.

Status: Needs review » Needs work

The last submitted patch, 20: confirm_clear-936704-20.patch, failed testing.

longwave’s picture

This 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.

jimi-o’s picture

Status: Needs work » Needs review
FileSize
5.05 KB

Patch for update form API

Status: Needs review » Needs work

The last submitted patch, 23: Confirm_delete_dblog-936704-23.patch, failed testing.

Matt V.’s picture

Status: Needs work » Needs review
FileSize
6.38 KB

I've rerolled the patch and also updated the tests and some of the formatting.

Status: Needs review » Needs work

The last submitted patch, 25: confirm_delete_dblog-936704-25.patch, failed testing.

Jalandhar’s picture

Assigned: swentel » Unassigned
Status: Needs work » Needs review
FileSize
9.22 KB

Updating patch with reroll and required changes.

Please review.

Status: Needs review » Needs work

The last submitted patch, 27: drupal8-confirm_delete_dblog-936704-27.patch, failed testing.

Matt V.’s picture

Status: Needs work » Needs review
FileSize
5.76 KB

I 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.

Matt V.’s picture

Here's an updated patch that adds the redirect back to /admin/reports/dblog after the logs have successfully been cleared.

tim.plunkett’s picture

+++ b/core/modules/dblog/dblog.routing.yml
@@ -5,7 +5,15 @@ dblog.overview:
+    _content: '\Drupal\dblog\Controller\DbLogController::confirm'

This 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.

Matt V.’s picture

Here'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.

Matt V.’s picture

I 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.

Status: Needs review » Needs work

The last submitted patch, 33: confirm_delete_dblog-936704-33.patch, failed testing.

Matt V.’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 33: confirm_delete_dblog-936704-33.patch, failed testing.

Matt V.’s picture

Here's another try with some corrected paths and cleaned up spacing.

Matt V.’s picture

Status: Needs work » Needs review
tim.plunkett’s picture

Cleaned 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.

longwave’s picture

+    _title: 'Confirm Delete Recent log messages'

Capitalisation looks a bit off here.

+++ b/core/modules/dblog/src/Form/DblogClearLogConfirmForm.php
...
+ * Contains \Drupal\dblog\Form\DblogClearConfirmLogForm.
...
+class DblogClearLogConfirmForm extends ConfirmFormBase {
...
+   * Constructs a new DblogClearConfirmLogForm.

DblogClearLogConfirmForm or DblogClearConfirmLogForm? :)

Matt V.’s picture

Here's an updated patch that addresses the issues longwave pointed out.

longwave’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me now.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 41: dblog-confirm-936704-41.patch, failed testing.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed f0cb85a and pushed to 8.x. Thanks!

  • alexpott committed f0cb85a on 8.x
    Issue #936704 by Matt V., swentel, tim.plunkett, Jalandhar, jimi-o:...
David_Rothstein’s picture

Version: 8.x-dev » 7.x-dev
Status: Fixed » Patch (to be ported)
Anonymous’s picture

Well, 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.

  • alexpott committed f0cb85a on 8.3.x
    Issue #936704 by Matt V., swentel, tim.plunkett, Jalandhar, jimi-o:...

  • alexpott committed f0cb85a on 8.3.x
    Issue #936704 by Matt V., swentel, tim.plunkett, Jalandhar, jimi-o:...

  • alexpott committed f0cb85a on 8.4.x
    Issue #936704 by Matt V., swentel, tim.plunkett, Jalandhar, jimi-o:...

  • alexpott committed f0cb85a on 8.4.x
    Issue #936704 by Matt V., swentel, tim.plunkett, Jalandhar, jimi-o:...