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.

That patch did not do anything, I suspect the problem is older than that, but can't quite figure it out.

Ignore my reasons for wanting this, that just muddies the issue. Take this example instead:

A module is currently set to schema version 5999. Update_6000 returns array('success' => false), which could be the result of a failed sql query or anything else where the update returns a failure message. Update.php then happily sets the schema version to 6000, as though the update was successful. The next time you go to update.php it will say there are no more updates, but update 6000 never happened. If there is also an update_6001, then what will happen? It will go ahead and do that update, too.

So the question is, is that what ought to happen? It doesn't seem intuitive. If we have gone to all this trouble of creating a way to report a failure, why do we treat it as though it was a success? Shouldn't we be using a report of a failure as a message to stop the update? And shouldn't we have a way to abort an update if an important component that affects later updates has failed?

And the reason why this will more of an issue than before is because these updates used to only run on modules that were installed, so you could control them, a little bit, by not enabling the module until you are ready to update it, but now I get all updates running automatically for every single module on my server, whether or not they are installed, and whether or not the modules they depend on are installed, so there are more ways they might go wrong.

Maybe this is all by design and I missed the discussion, I'm just raising the issue because it seems odd to me. And that's why instead of saying 'This is a bug!' I said 'Is this intended behavior?'.

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:

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.

This looks like a good solution, except I would reword the flag from '#cannot update' to '#abort', which is only one word and is really clear.

If an update is aborted, the system is going to go on and try the next one, and if it works, update the schema version, undoing this effort, but there is a work-around for that. Since the schema version doesn't get reset on the aborted update, I can test in later updates for the schema version and abort those updates, too, until the underlying one is successfully completed. That's a little messy, but should work, and it wouldn't work without this patch.

So I think this solution would help out quite a bit, if gabor is agreeable to adding it.

I'm going to go back and test this, just to be sure.

OK, works like a charm. The updates I use this new flag on do not get their schemas updated and do not run, and display on the next visit to update.php as updates that have not yet been done, everything I was trying to accomplish.

And here's my slightly altered patch, changing to the use of '#abort' and adding a big more documentation to the code.

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

A one-word term would be better than two, because of problems figuring out if there is supposed to be a space or an underscore between them. The patch uses a space ('#cannot update'), which is inconsistent with other FAPI-like properties, I would use an underscore.

I agree with webchick that reversing the words makes more sense, I thought that '#cannot update' sounded odd but wasn't sure why, but she's right that it's the word order that's odd.

I still think #abort is clear enough, but can see the issue, '#update_error' is OK, or how about '#skipped' or '#failed' or '#omitted', or just '#error'?

Plus I added some extra documentation in #9 that might still be useful so developers understand that skipping one update will not stop the others from running:

Oops, cross-posted with pwolanin. My points in #15 still stand, but #14 is OK with me.

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.

Are you agreeable to adding that in at this point? I agree it would be a better solution, but assumed that was too much of a change for late in the cycle.

I agree that there may be reasons not to abort on every failure, some failures are not fatal, but we certainly should have a way to force an abort.

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

I think I found a way to do it. Patch attached. I tested this by trying to abort a content module update and it seems to work fine.

I tested that this works to stop both the current and later updates for an individual module with multiple updates, and that it doesn't prevent other modules from updating, and it works fine. I did find one place where I needed to fix an undefined index for updates that are not aborted, so a revised patch is attached.

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.

I think the fatal error in the book install is because it aborts in a batch before the batch has returned finished=1. Any batch update that returns finished < 1 will give you a fatal error in the current update code. I'm not sure if this patch should change that behavior, or how.

I see no reason for a message at the top of the results page, the message will show up in the list at the bottom showing the update failed with whatever text you put in 'query'. To me that seems like the logical way to do it.

And there's another problem with testing abort in the system install, which is that if the system install fails, the whole update should abort, not just the system update, so this is a different case than an individual module update. The abort patch above is designed to only abort a specific module, not the whole system. Aborting a system install is going to take something different.

I misread your post in my comment above, I thought you were testing the book.install when you had a failure, not a system install. The fatal error there would be that an important system component did not get finished which would obviously break dependent module updates later.

So the question is whether or not this should be intended to work for the system update. I would say not, because the system update is so integral to everything else.

If it should, that patch will need to be adjusted to check if it is a system update that has failed, and behave completely differently. If it is not, we should test other installs than the system install.

Change 'install' to 'update' in my comments above. I was typing too quickly.

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.

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

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.

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