Problem/Motivation

When an exception occurs meanwhile a VBO operation is processing the following error screen is visible and its content is not too meaningful. A simple user has no clue what went wrong and how it could be fixed.

Background story: I added some custom logic to a hook_node_delete() implementation to prevent removal of certain nodes.

Proposed resolution

Catch exceptions meanwhile a VBO operation is processing items and report them to user after operation is finished.

CommentFileSizeAuthor
#4 interdiff-2-4.txt988 bytesmxr576
#4 views_bulk_operations-properly-report-exceptions-2856207-4.patch2.62 KBmxr576
#2 views_bulk_operations-properly-report-exceptions-2856207-2.patch2.57 KBmxr576
Screen_Shot_2017-02-27_at_9_31_43.png65.96 KBmxr576
Screen Shot 2017-02-27 at 8.59.07.png98.59 KBmxr576
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mxr576 created an issue. See original summary.

mxr576’s picture

mxr576
Primary language Hungarian
Location Hungary
CreditAttribution: mxr576 at Pronovix commented
FileSize
views_bulk_operations-properly-report-exceptions-2856207-2.patch2.57 KB

My patch also fixes how $log variable's type checked in multiple places. The original implementation also handled $log = array() than $log = NULL therefore almost always watchdog() was in use to log messages.

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet at The University of British Columbia commented
Status: Active » Needs work

Here's a quick review, this looks like an interesting idea, I agree with it on some angles but need to consider it and get a bit more reviews before accepting the patch.

  1. +++ b/views_bulk_operations.module
@@ -1048,7 +1048,7 @@ function views_bulk_operations_active_queue_process($queue_name, $operation, $to
- * @param $log
+ * @param array|null $log

@@ -1096,7 +1096,7 @@ function views_bulk_operations_queue_item_process($queue_item_data, &$log = NULL
       );
...
-      if ($log) {
+      if (is_array($log)) {

    This looks like an unrelated change. 

  2. +++ b/views_bulk_operations.module
@@ -1113,9 +1113,25 @@ function views_bulk_operations_queue_item_process($queue_item_data, &$log = NULL
+      $message = 'Skipped %operation on @type %title due to an exception occurred. Message: !message';
...
+        $log[] = t($message, $arguments);
...
+        watchdog('views bulk operations', $message, $arguments, WATCHDOG_ERROR);

    Though the the idea is correct, the parser that reads t() doesn't work with variables because it statically reads the PHP source to build the translations, so you have to duplicate the message. 

  3. +++ b/views_bulk_operations.module
@@ -1113,9 +1113,25 @@ function views_bulk_operations_queue_item_process($queue_item_data, &$log = NULL
+      ) + _drupal_decode_exception($e);

    Probably shouldn't be calling Drupal core internal functions and if so maybe _drupal_render_exception_safe() would be better for security

mxr576’s picture

mxr576
Primary language Hungarian
Location Hungary
CreditAttribution: mxr576 at Pronovix commented
Status: Needs work » Needs review
FileSize
views_bulk_operations-properly-report-exceptions-2856207-4.patch2.62 KB
interdiff-2-4.txt988 bytes

Thanks for the quick review.

1. It is not completely unrelated, because without this fix VBO sends almost every messages to watchdog instead of displaying them on the UI as messages.

My patch also fixes how $log variable's type checked in multiple places. The original implementation also handled $log = array() than $log = NULL therefore almost always watchdog() was in use to log messages.

2. Yes you are correct, but I've just re-used this "pattern" from VBO's current codebase. Similar issues in the current codebase should be fixed independently, I've fixed the related lines in my patch.
http://cgit.drupalcode.org/views_bulk_operations/tree/views_bulk_operati...

3. I do not want to render a full exception to a user as a message. I choosed to use _drupal_decode_exception() (just as watchdog_exception() does) to generate a proper !message variable from an exception (the same way as Drupal core does) and display only the "human readable" part of an exception to a user as a message on the UI.