The code provided in system.api.php includes an UpdateException on line 2797 within the hook_update_N function. The statement immediately before that line is a return statement which will ensure that the exception will never be thrown.

Either remove the Exception or reorder the operations so that the Exception is before the return statement.

Files: 
CommentFileSizeAuthor
#18 1788610-hook_update_N_exception-14.patch981 bytesbrainbender
#13 1788610-hook_update_N_exception-13.patch981 bytesbrainbender
PASSED: [[SimpleTest]]: [MySQL] 59,011 pass(es).
[ View ]
#11 1788610-hook_update_N_exception-11.patch983 bytesfjd
PASSED: [[SimpleTest]]: [MySQL] 55,827 pass(es).
[ View ]
#7 1788610-hook_update_N_exception.patch953 byteschertzog
PASSED: [[SimpleTest]]: [MySQL] 55,940 pass(es).
[ View ]
#5 1788610-hook_update_N_exception.patch954 byteschertzog
PASSED: [[SimpleTest]]: [MySQL] 55,462 pass(es).
[ View ]

Comments

Issue tags:+#pnx-sprint

Tagging

Issue tags:-#pnx-sprint

Since this is just sample code that never actually runs, it's probably not a big deal that it's kind of weird, but it certainly could be a better example.

I think that the example would be more helpful if the exception throw were inside of a conditional—something like, if this function call fails, throw an exception. At the moment, I can't think of a good example of what might be checked, though.

Also, just for convenience, this code can be seen in the docs here:

http://api.drupal.org/api/drupal/modules%21system%21system.api.php/funct...

Assigned:Unassigned» nick_schuch

Updating assignee.

Assigned:nick_schuch» Unassigned
Status:Active» Needs review
StatusFileSize
new954 bytes
PASSED: [[SimpleTest]]: [MySQL] 55,462 pass(es).
[ View ]

Here is a patch that moves the exception to before the return and puts in a check that would never pass.

Status:Needs review» Needs work

Patch applied cleanly. Verified the exception code now appears before the return statement.

I noticed the conditional does not have the standard formatting, there is no white space between the 'if' and opening brackets.
I'm not sure how to test for the thrown exception. Will the statement ever pass and throw the exception? If the exception is never thrown, is there any need for it?

Status:Needs work» Needs review
StatusFileSize
new953 bytes
PASSED: [[SimpleTest]]: [MySQL] 55,940 pass(es).
[ View ]

Its in the API docs, so the code never runs. The exception is just there to demonstrate usage. That conditional should never pass.

Status:Needs review» Reviewed & tested by the community

Patch applied cleanly. Verified the condition and exception appear before the return statement. Condition is formatted correctly.

Agreed, patch looks good.

Status:Reviewed & tested by the community» Needs work

+++ b/core/modules/system/system.api.phpundefined
@@ -2695,12 +2695,14 @@ function hook_update_N(&$sandbox) {
+  if ($sandbox['#finished'] > 1) {
+    // In case of an error, simply throw an exception with an error message.
+    throw new UpdateException('Something went wrong; here is what you should do.');
+  }

The check here looks spurious... some like this is better...

  if ($some_error_condition_met) {
    // In case of an error, simply throw an exception with an error message.
    throw new UpdateException('Something went wrong; here is what you should do.');
  }

Status:Needs work» Needs review
StatusFileSize
new983 bytes
PASSED: [[SimpleTest]]: [MySQL] 55,827 pass(es).
[ View ]

Created patch based on feedback in comment #10.

Status:Needs review» Needs work

Looks good other than trailing whitespace, please remove.

Status:Needs work» Needs review
StatusFileSize
new981 bytes
PASSED: [[SimpleTest]]: [MySQL] 59,011 pass(es).
[ View ]

As part of drupalcamp nh codesprint I rerolled this patch with spaces taken out.

Patch applied cleanly, verified that the trailing whitespace mentioned in #12 has been removed.

Thanks for checking :)

Status:Needs review» Reviewed & tested by the community

Thanks, looks good! An interdiff is also very helpful to create and upload when updating patches, so you know for next time.

Status:Reviewed & tested by the community» Fixed

Committed 9ba259b and pushed to 8.x. Thanks!

During commit set file permissions back to 644 - the patches in #11 & #13 changed them 755

StatusFileSize
new981 bytes

I fixed the permissions hopefully this is fine let me know if it needs any other changes. Thanks

Status:Fixed» Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.