In CronUpdateStage if postApply() fails no update email will be sent

Reverted the commit because I will first add test coverage.

looks good, nice work.

Self-assigning to review and commit.

I'm wondering if this is the approach we want. It seems a little complicated, to be honest.

Here's my question: couldn't we also solve this problem by simply sending the success email after the apply phase completes, but before we call postApply()?

I discussed this with @tedbow and I suspect we might be able to do this in a simpler way.

What if we simply moved the line where we send the "update successful!" message into postApply() (or handlePostApply(), I guess), before we dispatch PostApplyEvent?

By that point, the update has successfully applied. If any problem happens during post-apply, it shouldn't break the site (or at least, not unrecoverably). If a post-apply event subscriber throws an exception, we can just catch and log it, same as we do (IIRC) for the destroy phase.

Could we try implementing this approach in a separate MR, to see what breaks and whether this would actually work? We should certainly keep the new test coverage as well.

agree with #11 better to send the email before we dispatch postAppy event

Ok, I will try to implement the approach suggested in #11 🐛 In CronUpdateStage if postApply() fails no update email will be sent Fixed in a separate MR.

I just saw we dispatch postApply event in \Drupal\package_manager\StageBase::postApply() and I don't think we should change this method to send successful update mail before dispatching postApply event as it will affect all other stages.

Relating to 📌 Document when PostApplyEvent should be used instead of hook_update_n() or post update RTBC because since we are deciding in #11 not to send an email if postApply fails this will affect how we document postApply

re #15 yes. lets not change StageBase. Instead send the email in `\Drupal\automatic_updates\CronUpdateStage::handlePostApply` before we call $this->postApply(); since we won't change the email either way. Very unlikely since we use a try/catch
around $this->postApply(); that we won't send an email after but safer to send it before.

We undo the changes to hook_mail.

I removed a superfluous try-catch (the mail manager doesn't throw, as far as I'm aware, so there's no point in having that in a try block), and moved a few lines around, but otherwise? This looks great to me, honestly.

Assigning to @tedbow for final review.

phenaproxima → closed merge request !922

Great!

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