Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dww’s picture

Assigned: Unassigned » 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...

dww’s picture

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

dww’s picture

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

dww’s picture

I had Bojhan look at this in IRC, and his only comment was to shrink the label down to just:

Downloading your missing updates failed:

Bojhan’s picture

Yup, sometimes we focus too much on making full sentences - where less is more, to get people to actually scan it.

JacobSingh’s picture

+++ modules/update/update.manager.inc	27 Oct 2009 08:36:12 -0000
@@ -326,14 +326,21 @@ function update_manager_update_form_subm
-      drupal_set_message($message, 'error');

Is a set-message the appropriate action here? Previously, the reporting was handled by the page they get goto'd to.

+++ modules/update/update.manager.inc	27 Oct 2009 19:50:49 -0000
@@ -326,14 +326,21 @@ function update_manager_update_form_subm
+  elseif ($success) {
+    $_SESSION['update_manager_update_projects'] = $results['projects'];

Should we show people what succeeded if something failed? In this case, I don't think they could see that.

JacobSingh’s picture

Status: Needs review » Reviewed & tested by the community

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

dww’s picture

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

JacobSingh’s picture

Okay, one comment. I don't understand why my "updates" are "missing".

Why not just:
Downloading updates failed.

dww’s picture

Yup, good point. ;) Even better. Identical code, just the new message.

webchick’s picture

Status: Reviewed & tested by the community » Active

Committed to HEAD. Thanks!

According to dww, I'm supposed to set this back to active now. :)

dww’s picture

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

yched’s picture

Title: Work-around BatchAPI's inability to propagate failure in update manager » propagate failure in update manager

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

dww’s picture

Title: propagate failure in update manager » propagate failure during batches in update manager

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

dww’s picture

Assigned: dww » Unassigned

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

Bojhan’s picture

Installation 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

rschwab’s picture

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

dww’s picture

Priority: Normal » Major
Issue tags: +String freeze

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

rschwab’s picture

Version: 7.x-dev » 8.x-dev
Scott M. Sanders’s picture

Following

sun’s picture

Category: bug » task
Priority: Major » Normal
Issue tags: -String freeze +Needs backport to D7

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

dww’s picture

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

jhedstrom’s picture

Issue summary: View changes
Status: Active » Postponed (maintainer needs more info)
Issue tags: +Needs issue summary update

The IS could use some updating here.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

  • webchick committed b1dc42e on 8.3.x
    #605292 by dww: Work-around BatchAPI's inability to propagate failure in...

  • webchick committed b1dc42e on 8.3.x
    #605292 by dww: Work-around BatchAPI's inability to propagate failure in...

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

  • webchick committed b1dc42e on 8.4.x
    #605292 by dww: Work-around BatchAPI's inability to propagate failure in...

  • webchick committed b1dc42e on 8.4.x
    #605292 by dww: Work-around BatchAPI's inability to propagate failure in...

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

  • webchick committed b1dc42e on 9.1.x
    #605292 by dww: Work-around BatchAPI's inability to propagate failure in...

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.2.x-dev

Drupal 8 is end-of-life as of November 17, 2021. There will not be further changes made to Drupal 8. Bugfixes are now made to the 9.3.x and higher branches only. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.15 was released on June 1st, 2022 and is the final full bugfix release for the Drupal 9.3.x series. Drupal 9.3.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.4.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.9 was released on December 7, 2022 and is the final full bugfix release for the Drupal 9.4.x series. Drupal 9.4.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.5.x-dev branch from now on, and new development or disruptive changes should be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

smustgrave’s picture

Status: Postponed (maintainer needs more info) » Closed (outdated)

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

smustgrave’s picture

Status: Closed (outdated) » Reviewed & tested by the community

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

dww’s picture

Version: 9.5.x-dev » 7.x-dev
Category: Task » Bug report
Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs backport to D7 +Bug Smash Initiative

Yes, 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. 😂

dww’s picture

p.s. x-post, but in Slack, @larowlan said "Happy if you want to mark it fixed too dww", so leaving status. 😊

Status: Fixed » Closed (fixed)

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