Improve the user experience of having your staged update deleted before it was applied

Tests look solid 👍

23 remarks on the MR. 8 of which were trivial nits, for which I already applied the fixes. Several were for test clarity, for which I already fixed 2 as an example. And 1 was for an obsolete @todo which I removed. That leaves 12 remarks for you to address 😊

227f07df was the last commit by @yash.rode (on January 10) before I reviewed it yesterday in #17.

Between those two dates, 📌 Improve FixtureManipulator DX: validate package name + ensure StageFixtureManipulator is committed + ensure `package_manager_bypass_composer_stager` is not set to FALSE Fixed landed on January 12.

It made StageFixtureManipulator much more strict. Which explains why ErrorMessageOnStageDestroyTest::testMessagesOnStageDestroy() is now failing but wasn't before. There were more of those examples in HEAD prior — see #3327391-35: Improve FixtureManipulator DX: validate package name + ensure StageFixtureManipulator is committed + ensure `package_manager_bypass_composer_stager` is not set to FALSE → and #3327391-37: Improve FixtureManipulator DX: validate package name + ensure StageFixtureManipulator is committed + ensure `package_manager_bypass_composer_stager` is not set to FALSE → .

But this one is more complicated, because we're testing that a StageFixtureManipulator's queued changes were applied but then we destroy the stage, but simultaneously we continue trying to interact with it. That apparently somehow causes service destruction to happen in a different order, which causes the DB connection destruction to happen earlier, which causes the state to fail to make its way back from the SUT to the test runner (or rather: it made its way back, but the test runner was reading it too late).

AFAICT a single line change fixes this… 😬

This was a great find, I have addressed other changes requested in the review. It is ready for review.

I'm not sure ::storeDestroyInfo() is the best method name, but that's nitpicking. Also, it's protected anyway. I like that the name contains "info" while it only stores a message, because that implies that more parameters could be added in the future.

Similarly, the obvious asymmetry with ::computeDestroyMessage() (which is private) is actually a good thing: right now, only a message is computed from the stored info, but in the future there may be more methods added related to the destroying of a stage.

Still LGTM, but still that remark in #23 for @tedbow when he reviews for commit :)

Some cleanup, needs review

Also 1 suggestions that could be done

After @yash.rode has addressed the sole tiny change that is still needed, he can re-RTBC this himself 😊

Fixed one more nit, RTBC++

I misunderstood @tedbow too!

I'll get this sorted out before my EOD so @tedbow can commit it today :)

Addressed 👍

(Currently unable to queue a 7.4 test, despite 🐛 phpcs stopped working since the switch to testing on Drupal 10.0.x by default Fixed definitely not conflicting with this. Perhaps because that was just merged? 🤔)

Well well well, 🐛 phpcs stopped working since the switch to testing on Drupal 10.0.x by default Fixed is already striking:

----------------------------------------------------------------------
FOUND 0 ERRORS AND 3 WARNINGS AFFECTING 3 LINES
----------------------------------------------------------------------
 699 | WARNING | Exceptions should not be translated
     |         | (DrupalPractice.General.ExceptionT.ExceptionT)
 704 | WARNING | Exceptions should not be translated
     |         | (DrupalPractice.General.ExceptionT.ExceptionT)
 712 | WARNING | Exceptions should not be translated
     |         | (DrupalPractice.General.ExceptionT.ExceptionT)
----------------------------------------------------------------------

… and it's wrong, because \Drupal\automatic_updates\Form\UpdateReady::buildForm() will catch this and present it to the end user:

  public function buildForm(array $form, FormStateInterface $form_state, string $stage_id = NULL) {
    try {
      $this->updater->claim($stage_id);
    }
    catch (StageOwnershipException $e) {
      $this->messenger()->addError($e->getMessage());
      return $form;
    }
…

… but of course, we do have to comply with Drupal core's phpcs rules.

Not sure how to proceed here. It seems that "exceptions for user-facing strings (that should be translated)" is a bad practice, but this module has embraced that. It's technically out of scope to fix here though.

So … worked around it, and tagging .

I have implemented by only MR suggestion so setting to needs review

Great catch! 😳🙈 I totally missed that!

Your change looks correct. The failing test is a test untouched by this issue/MR … so I debugged it, and the expected message should indeed change thanks to this issue 👍 So it's a simple fix, and only possible because @tedbow spotted this critical oversight 😱🎉

Nope — the metadata is associated with that specific stage, so that's not a risk at all! 👍

Follow-up created ( 📌 Drupal core's coding standards forbid translated exceptions, but does that anyway — f.e. FileValidationException Active ) and linked to (https://git.drupalcode.org/project/automatic_updates/-/merge_requests/63...).

I cannot reproduce that failure locally… 😳 Re-testing. Is this our first random failure?

That test passed for me too locally

It is consistently passing on DrupalCI now, so it does seem there is a rare race condition in this test… and I'm 90% certain that it relates to #3337049-7: CoreUpdateTest & ModuleUpdateTest do not correctly mock the update XML, ModuleUpdateTest does not correctly mock the core version → and #3337049-9: CoreUpdateTest & ModuleUpdateTest do not correctly mock the update XML, ModuleUpdateTest does not correctly mock the core version → .

Let's get this in? 🙏

🏆 Thanks @yash.rode and @Wim Leers this is big UX improvement!

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