Update.php doesn't display errors correctly

#3: update_multiple_errors.patch queued for re-testing.

The issue here is actually somewhat more broad.

The current code in D7 is correct to only use the first array element in $_SESSION['updates_remaining'], because you can only trigger, e.g., one fatal PHP error on the page. The other elements in the array represent update functions that never had a chance to run, not update functions that triggered errors themselves, so we don't want to display them as errors. For that reason, the patch in #3 doesn't seem correct to me.

The biggest problem, though, is that the current code only works correctly in the case of PHP fatal errors or other situations where the batch process was interrupted. In D7, however, we except hook_update_N() implementations to throw exceptions, and those exceptions are caught and handled, so the batch API and $_SESSION['updates_remaining'] don't know anything about them.

Putting it all together it means that not only multiple update failures, but if fact any update failures corresponding to a handled exception, are ignored and treated as almost indistinguishable from the 'success' condition by update.php. I think to fix that we need a larger cleanup such as in the attached. This makes sure that all update failures, whether handled exceptions or not, get prominently highlighted on the update results page.

I'm also attaching some screenshots.

In the code comments of the patch, I referred to user interruption (in addition to a PHP fatal error) as one way the current error condition can be triggered. Trying that out, I'm actually not sure I can reproduce it - e.g., by trying to hit the stop button in my browser or navigating away from the page while update.php is running. I thought in general it was possible to interrupt a batch process that way, though.

In any case, PHP fatal errors definitely do it :)

Agree that we need better feedback from the batch process.
I spent part of yesterday testing out D6 -> D7 updates. First a did not get what went wrong with the db. updates until I realized that if node_update_7002 failed node_update_7003, 7004, 7005 did not run. We should inform the user that one update failed and list the ones that did not run caused of decencies rules. Now if it fails you have to dig into module.install files to figure out if/what other updates that did not run if one of them failed.

@kwinters, I tried your example code but modified it to add this line at the top of each function:

syslog(LOG_NOTICE, __FUNCTION__ . " was called");

So I could see for sure which functions actually were run. When I did this, only one function appeared in the syslog, which is what I expected. The batch API bails out when it hits a fatal error; I don't see how you can ever hit two of them.

I think your patch showed two errors on the screen because it reported all functions the batch API didn't finish running, but that includes ones it never tried to run also. Using @steinmb's example, it would have reported node_update_7003, 7004, 7005 as having "errors", even though those functions never ran.

I agree we might want to display somewhere the functions that never ran (maybe in the detailed report at the bottom of the update page) and also agree that that may be best left to a separate issue :)

For reporting errors, though, we should only report the actual functions that were known to have problems when they ran, and that is what I was trying to do with my patch.

Ah, yes, that could explain it :) I believe it is the case that some drastic changes were made to the internals of the batch API in the last several months.

Using trigger_error() with E_USER_ERROR, I can reproduce the same thing as you. I am not sure if that is a bug or not; it is documented in the hook_update_N() docs that exceptions should be thrown in case of an error.

Un-assigning @kwinters Not sure if this issue is still out in the wild or we have fixed it by touching on similar issues around the installer?

The code changes in the above patch only touch update code, and from looking at the current codebase I would guess a lot of this is still valid although maybe not all of it (#1013808: update.php displays misleading messages touched some of the same code but isn't quite the same thing).

I'm sure the nearly three-year-old patch above needs a reroll, though...

Looking at the D9 code it doesn't look like this has changed all that much, but I'm not really sure what the target of this issue is given #3 says the origin bug wasn't reproducible and there are a bunch of other threads in the comments.

Anyone more familiar with this able to review it and determine whether this needs to be addressed in 8/9 or can be knocked back to 7?

We really need some steps to reproduce this in D9. The IS has steps for a non core update in D6, and there is some discussion about steps in the comments about what so people expect in certain situations but we need something more concrete I think before work can done here

This has been triaged twice in 3 years by the Bug Smash initiative by different people. One pointed out that there was a comment saying the problem was not reproducible. In both comments more information is asked for so that the problem can be reproduced. Since that information hasn't been supplied, the latest request was 4 months ago, it is time to close the long standing issue.

Since #3 stated the original problem was not reproducible I am closing as cannot reproduce. you are experiencing this problem on a supported version of Drupal reopen the issue, by setting the status to 'Active', and provide complete steps to reproduce the issue (starting from "Install Drupal core").