Problem/Motivation

I discovered this while working on #3338666: Add functional test that proves there is reasonable UX whenever a stage event subscriber has an exception. If a PostDestroyEvent subscriber throws an exception, the batch processor redirects you to the UpdateReady form. The first thing that form tries to do is claim the stage, but since the stage has already been destroyed, you get an exception saying "This operation was already canceled." That's no good.

Proposed resolution

If the subscribers of either PreDestroyEvent or PostDestroyEvent throw an exception, redirect to the finish page, which will report the error, and also notify the user that the update completed successfully (or needs to run update.php). In the case of PreDestroyEvent, if the user goes back to the start page and tries to start a new update, they won't be allowed to do it because the previous stage still exists...but at least they'll have the ability to delete it.

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

phenaproxima created an issue. See original summary.

phenaproxima’s picture

I've discovered how we can fix this.

In BatchProcessor::clean(), we should specifically deal with StageEventException carrying PostDestroyEvent. If we catch that, we know the update finished successfully, so rather than recording and re-throwing the exception, we should ensure its message is added to the messenger, then return a RedirectResponse to the updater form (/admin/reports/updates).

tedbow’s picture

Just clarifying that we need a "Proposed resolution" in the summary before we work on this

phenaproxima’s picture

Issue summary: View changes
phenaproxima’s picture

phenaproxima’s picture

Assigned: phenaproxima » wim leers
Status: Active » Needs review

Issue summary is accurate and reflects what I implemented here. I'm happy with it; tests should be passing! I think this is ready for review; assigning to Wim for that.

wim leers’s picture

Assigned: wim leers » Unassigned
Status: Needs review » Reviewed & tested by the community
Issue tags: +sprint, +Usability
Related issues: +#3338666: Add functional test that proves there is reasonable UX whenever a stage event subscriber has an exception

Zero remarks, looks perfect!

  • phenaproxima committed e9acf9d4 on 3.0.x
    Issue #3354003 by phenaproxima: If a PostDestroyEvent subscriber throws...
phenaproxima’s picture

Status: Reviewed & tested by the community » Fixed

Excellent. Nice to have found this with our new test, and fixed it with the same test!

Status: Fixed » Closed (fixed)

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

tedbow’s picture

Issue tags: +core-mvp