Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
#538660: Move update manager upgrade process into new authorize.php file (and make it actually work) doesn't propagate errors in its batches properly, due to major flaws in the Batch API: #604902: Batch API only returns $success == FALSE for fatal PHP errors and uncaught exceptions
Even if #604902 lands, we're going to need to fix the batch error handling.
Comment | File | Size | Author |
---|---|---|---|
#10 | 605292-10.update-manager-batch-errors.patch | 2.91 KB | dww |
#10 | 605292-10.update-manager-batch-errors.png | 57.57 KB | dww |
#4 | 605292-4.update-manager-batch-errors.patch | 2.93 KB | dww |
#4 | 605292-4.update-manager-batch-errors.png | 58.37 KB | dww |
#2 | 605292-2.update-manager-batch-errors.patch | 2.95 KB | dww |
Comments
Comment #1
dww#604902: Batch API only returns $success == FALSE for fatal PHP errors and uncaught exceptions appears to be a documentation and DX WTF issue, not really the critical bug I thought it was. Basically, the $success parameter to your finished callback is really $had_no_fatal_php_errors_or_exceptions. If you want any other definition of $success, it's up to you. So, I'm going to take a stab at fixing this throughout the new Update manager batches. Stay tuned...
Comment #2
dwwHeh, I got sucked into other things. ;) Also, this is turning out to be a nastier job than I thought...
The attached patch cleans up the download batch that's all internal to update.manager.inc. Attaching some screenshots, both of the normal error case (605292-2.update-manager-batch-errors.download-errors.png) and what happens in case of a fatal error (an uncaught exception, etc). Note that we shouldn't hit this other case (we're catching all exceptions in the batch operations themselves), and the ugly is provided by Batch API. I'm just putting it here to show that the new patch actually does handle those kinds of errors, too.
However, when I started looking into the update/install batches that run at authorize.php, it's sort of a tangled mess. The low-level logic about the weird #abort hack and the specific layout of the batch results array is deeply entrenched at various levels of the code:
modules/update/update.authorize.inc:
- update_authorize_batch_copy_project()
- update_authorize_update_batch_finished()
- update_authorize_install_batch_finished()
includes/theme.maintenance.inc:
- theme_authorize_report()
The stuff in update.authorize.inc makes sense -- that's where this work-around should be localized. However, the fact that theme_authorize_report() knows about this is pretty evil. I'd like to revisit how that function works so that it doesn't force you into a very specific way of using Batch API.
Comment #3
dwwp.s. To clarify: I think patch #2 is a good fix for part of the problem this issue is trying to solve (the batch that downloads, extracts, and verifies updates -- where a large fraction of the possible errors are going to come, anyway). I think it can be reviewed/tested/committed as-is. Meanwhile, we can worry about cleaning up the batches that actually install the extracted code later in this issue. They're entirely separate batches in separate files. There's no need to wait for this to become a larger patch before we commit -- let's get the more obvious fix in first, and then we can focus on the other more complicated piece. Thanks.
Comment #4
dwwI had Bojhan look at this in IRC, and his only comment was to shrink the label down to just:
Downloading your missing updates failed:
Comment #5
Bojhan CreditAttribution: Bojhan commentedYup, sometimes we focus too much on making full sentences - where less is more, to get people to actually scan it.
Comment #6
JacobSingh CreditAttribution: JacobSingh commentedIs a set-message the appropriate action here? Previously, the reporting was handled by the page they get goto'd to.
Should we show people what succeeded if something failed? In this case, I don't think they could see that.
Comment #7
JacobSingh CreditAttribution: JacobSingh commentedSorry, I retract my original comments, I read this wrong, thought it was about the authorize.php step which I would handle differently. This should be fine.
Comment #8
dwwA) drupal_set_message() is right, since this is only for the download/extract/verify batch that runs in the full bootstrap in update.manager.inc. If anything fails at all, we just redirect right back to the project selector page. This isn't for the batches that run via authorize.php where we have a landing page with the results.
B) There are no messages printed at all on success. The only messages generated by this batch are for failures. If everything works, there's nothing to print, we just continue to the next step in the workflow (the "hey, now would be a nice time to backup your site! wanna go offline?" landing page). If a few things work and a few fail, we print everything we know about what failed and return to the project selector. I suppose we could mention the ones that worked in this case, but that seems like clutter in the UI...
Comment #9
JacobSingh CreditAttribution: JacobSingh commentedOkay, one comment. I don't understand why my "updates" are "missing".
Why not just:
Downloading updates failed.
Comment #10
dwwYup, good point. ;) Even better. Identical code, just the new message.
Comment #11
webchickCommitted to HEAD. Thanks!
According to dww, I'm supposed to set this back to active now. :)
Comment #12
dwwYup, back to active for what I spell out in #2. The batches triggered from update.authorize.inc, processed at authorize.php, and displayed via theme_authorize_report() are still a bit of a mess...
Comment #13
yched CreditAttribution: yched commentedCan we stop pointing at batch API's 'inability' plz ? Batch API provides the tools to let each batched process handle its own errors in the way that makes sense for it. Suggestions on how batch API could or should provide generic tools to help do this are welcome.
Comment #14
dww@yched: Sorry, that title was from a while ago, back when I thought #604902: Batch API only returns $success == FALSE for fatal PHP errors and uncaught exceptions was a critical bug in Batch API. Now that I understand it's just a critical bug in Batch API's documentation, I agree, this is a better title... ;)
Comment #15
dwwIt seems unlikely we'll be able to fix any of this in D7 and I've run out of time to try to do so. If someone else wants to make all this not suck, please go for it. Thanks.
Comment #16
Bojhan CreditAttribution: Bojhan commentedInstallation failed! See the log below for more information.
The ! violates, the "be friendly" approach we have to error messages.
We could probally rework this to just say "Installation failed." and pull in the error messages?
# Failed: Error installing / updating
# Failed: File Transfer failed, reason: Cannot create directory /home/bojhan01/domains/bojhan.com/public_html/leisa/sites/default/modules
Comment #17
rschwab CreditAttribution: rschwab commentedI closed #937700: No log when installation of modules fails as a duplicate, but if this gets fixed in time and we want a separate issue to work on the log specifically, that'd be the one.
Comment #18
dwwCrap, this fell off my radar and no one tagged it as a string freeze issue. I think we're probably going to be stuck with a whole ton of scary tech jargon error messages all over the place in the Update manager. :( I'd say this is full of pretty major UI bugs. But, it's 2:30am here now, and I don't have the capacity to do anything about this, other than escalate it and tag it appropriately. Maybe a miracle will happen and someone else will adopt this. Or perhaps Drieschick will allow some cryptic error messages to break the string freeze. I'm not even 100% all of these strings are even translatable...
Comment #19
rschwab CreditAttribution: rschwab commentedComment #20
Scott M. Sanders CreditAttribution: Scott M. Sanders commentedFollowing
Comment #21
sun1) Looks like we fixed the major bug already.
2) When we fixed that, we should have spawned a new issue for #2 (whereas it's not entirely clear to me what #2 remains to be about).
3) The UI/string issue raised by @Bojhan can likely be resolved much more quickly in a separate issue.
Comment #22
dwwWe need some clarity on how UX/UI bugs fit into normal/major/critical. Right now, there are all kinds of things that can go wrong in the update manager that print out entirely useless and cryptic error messages via exceptions thrown down in the bowels of various classes that transfer files, unpack them, and try to install them. These messages basically only make sense to the developer who wrote the exceptions, but they end up being presented directly to the end user who's having trouble. No one has yet audited all these exceptions and considered how to rewrite these in a way that actually helps the users having problems instead of further confusing them.
Anyway, this issue is a bit of a mess. There are now 3 inter-related things it's trying to deal with:
- Bugs in how the errors were propagated at all in some of the batches (already fixed)
- The UX bugs about the errors themselves
- The DX bugs and API WTFs about how the propagated error messages are displayed once you hit authorize.php
Probably the best course of action is to just split these other two into separate issues, and close this one. However, I'm unclear given the current major triage going on if the UX bugs still qualify as "major".
Comment #23
jhedstromThe IS could use some updating here.
Comment #41
smustgrave CreditAttribution: smustgrave at Mobomo commentedBelieve this can be closed out. 2015 was requested for an IS update and since there hasn't been a follow up assuming it's not needed.
If still a valid task please reopen.
Comment #42
smustgrave CreditAttribution: smustgrave at Mobomo commentedThis was brought up in #bugsmash https://drupal.slack.com/archives/C014QES6HSQ/p1706558152958059. @dww provided some background. Believe this can be marked Fixed but would prefer a committer make that call.
Comment #43
dwwYes, sorry I really mangled the issue scope on this one. 😅 As I wrote at #22, once #10 was committed (a pretty serious bug in Update Manager at the time), I should have closed this issue and opened follow-ups for the rest.
The cryptic exception messages might have been improved in some other issues, or they've been "this bad" for a dozen years. The remaining bugs are the UI woes from all the low level exceptions thrown by classes invoked by authorize.php, but that plumbing and the "Update Manager" parts of update.module are all but deprecated at this point. Definitely not worth spending time fixing any of that with auto_updates on the way.
Based on a Slack discussion about it, I think 'Fixed' is actually the most appropriate status for the original bug (the title here), and it was fixed in D7 so changing the version.
Crediting myself for the code, Bojhan for the UX review, JacobSingh for technical review, and webchick for core review + commit. Better late than never. 😂
Comment #44
dwwp.s. x-post, but in Slack, @larowlan said "Happy if you want to mark it fixed too dww", so leaving status. 😊