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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

larowlan’s picture

Issue tags: +#pnx-sprint

Tagging

BrockBoland’s picture

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.

BrockBoland’s picture

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

nick_schuch’s picture

Assigned: Unassigned » nick_schuch

Updating assignee.

chertzog’s picture

Assigned: nick_schuch » Unassigned
Status: Active » Needs review
FileSize
954 bytes

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

fjd’s picture

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?

chertzog’s picture

Status: Needs work » Needs review
FileSize
953 bytes

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

fjd’s picture

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.

markpavlitski’s picture

Agreed, patch looks good.

alexpott’s picture

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.');
  }
fjd’s picture

Status: Needs work » Needs review
FileSize
983 bytes

Created patch based on feedback in comment #10.

star-szr’s picture

Status: Needs review » Needs work

Looks good other than trailing whitespace, please remove.

_12345678912345678’s picture

Status: Needs work » Needs review
FileSize
981 bytes

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

galooph’s picture

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

_12345678912345678’s picture

Thanks for checking :)

star-szr’s picture

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.

alexpott’s picture

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

_12345678912345678’s picture

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.