Update numbers are set to max even when update fails

I didn't write that patch, but sounds like a bug that a return of FALSE doesn't have an effect

I don't see anywhere that this patch would have made that change: http://drupal.org/node/194310 Also, I see no evidence from D5 update.php that such a flag was respected. Are you sure about this?

I'm not quite sure about the solution you're suggesting. What if a module has multiple updates - should all be aborted if one fails?

Attached patch adds an explicit flag to tell update.php that the function can't run. Tested using the second patch which adds a dummy update function (please don't commit it!). If later functions for the same module succeed, then I guess you have the same problem as before.

I think this is in part by design.

Errors in a single query are not too uncommon in updating a real DB, so I think it would be incorrect to abort the process if a single query fails.

The patch above is intended to give you a way to prevent updates from running (as you desired) for example, add this sort of code to each update function:

function mymodule_update_6000() {
  $ret = array()

  if (!module_exists('mymodule')) {
    $ret['cannot update'] = array('success' => FALSE, 'query' => "Mymodule must be enabled for it to update.");
    return $ret;
  }
  ...

  return $ret;
}

Though this borders a feature request I can see that the new update behaviour might cause problems so this is quite OK. The change is almost 100% backwards compatible, I can't imagine someone already setting this in their updates. The added code is minimal and it's not even loaded on every page, so I say let's go with this.

On second thought, we use $ret['#finished'] so I added a few hashmarks to pwolanin's patch.

@KarenS - I considered the word "abort", but I thought something like "cannot update" is much clearer and more self-documenting. A developer returning "abort" might reasonably expect that the ENTIRE update process would cease at that point, rather than it being a flag that that single update did not run.

#6 is still valid though it seems.

yes #6 is fine with me.

Then let's do #update_failed or #update_error or something. I also think #abort is fine. "#cannot_update" is too ambiguous, imo, and it's weird to begin a two-word property name with a verb.

"#update_failed" sounds fine.

here's a re-roll with expanded comments along the lines suggested by KarenS

Let's try RTBC now. webchick suggested, pwolanin coded, KarenS agreed and I am fine with whatever so all participants agree on the latest #14 patch.

since you guys are working on update.php, perhaps you can answer my UI question at http://drupal.org/node/105302.

+ // Note that this will not prevent later updates for the same module from running
+ // and updating the schema version. Thus, you will need to add code to check the
+ // schema version in later updates and abort them, too.

Why? To me this sounds like: we have an incomplete abort feature here, which you will need to complement with custom code all on your own. Why not abort the whole update process for this function if an update "completely" fails? As Karen explained above, it means that further updates will fail or lead to an inconsistent state, so developers would need to implement this version check in all their subsequent update functions "for life" once they use this failure possibility in any of them.

@Gabor - I was going for a workable (if not ideal) solution that touched as little code as possible. Perhaps there should be a way (even this flag) to abort all updates for a particular module, though I don't think it makes sense to allow one module abort ALL site updates.

pwolanin: I am talking aborting all updates for the current module, not all site updates. As it is now, module updates are treated as being on their own, without cross-dependencies on each other (this is part of why we have a mostly linear system.install only flow of update functions). So aborting a module update should not abort other module updates.

@Gabor - I'll have to dig more deeply, but from my brief look it seemed as though ALL of the update functions were queued independently with the batch API. Doing what you suggest will require more serious hacking.

@Gabor - I'll have to dig more deeply, but from my brief look it seemed as though ALL of the update functions were queued independently with the batch API. Doing what you suggest will require more serious hacking.

Well, if you do find that it requires serious hacking, then it would be good to document the findings here, so we can have that info in the future, and see what we can do for now.

IMHO this is the desired behavior and the patch does not look like changing much :) So let's get this reviewed and tested a bit more.

ok - doesn't look bad in terms of LOC changed.

Did some basic testing with the patch and $ret['#abort'] = array('success' => FALSE, 'query' => 'Canceled!!!'); at various spots.

Ran update with patch applied and no 'abort' --> OK

Ran update with patch applied and 'abort' in system_update_6005() --> Fatal error in book.install ???

Ran update with patch applied and 'abort' in system_update_6042() --> Finishes, runs updates of other modules. Re-run update.php, it offers: system 6043+ (OK), already run updates of other modules (comment, book, blogapi)...

Also, when it aborts - there is no message at the top of the results page.

This all sounds sane to me, makes no sense for this patch to try to deal with system_update failures. Back to CNR.

Since we are not about to deal with cross-dependency in update functions, and the latest code looked code and proved good in the tests, I went ahead and committed it. Thanks!

This needs to be documented now on the handbook and also the 5.x -> 6.x update page

This broke updates.

Update a 5.x site to 6.x -- check status report, and it reports that the DB is out of date because none of the updates that were performed were recorded in the DB.

Well that's certainly not good...

Well, looks like the schema version update was wrapped in a bad conditional. We should update it is the #abort flag *is* empty, but before the attached patch it is only updated when the #abort flag is not empty. So the conditional should be flipped IMHO. Please test.

Tested OK.

Revised patch to clarify the code comment.

Thanks, committed.

The documentation about hook_update_N does not reflect these changes, although update.php relies on the hook implementations to support this new behaviour.

This is no longer critical, so I'm changing status, but it should be fixed nonetheless.

i just copied and pasted karrens's comments into the hook_update_N docs.

Back to original title.