Refactor exception architecture

yash.rode → made their first commit to this issue’s fork.

This also blocks 📌 Allow Validators/Excluders to store info on an individual stage use.. Closed: outdated — see #3328985-11: [PP-1] Allow Validators/Excluders to store info on an individual stage use.. → .

We need to figure out the test failures. Unassigning from myself if somebody else wants to give that a try

The two test failures remaining

1. The first in Tests\automatic_updates\Functional\UpdaterFormTest is caused because of Call to undefined method Drupal\package_manager_bypass\Beginner::destroy() the error causing line was added by @wim.leers in the code for cleanup but seems to be deleted now, so does the code need to be restored or refactored to not use this?
2. The second is in Kernel\StatusCheck\ScaffoldFilePermissionsValidatorTest which is caused by Drupal\package_manager\Exception\ApplyFailedException i think it is because the Exception is not caught and re thrown as a stageEventException and we are expecting that and that's why it fails, but i'm not sure where we should do it as only a call to dispatch() in Stage.php is doing that for now (the re throwing).

#15.1: Can you point to the exact line you're talking about? I didn't make any commits to this issue's MR, so I'm confused 😅 (Plus, no mentions of either or in the MR or in this issue except for your comment — so hard to find it 😬)

Yep i was talking about /automatic_updates/tests/src/Functional/UpdaterFormTest.php:482

Thanks!

📌 Explore simplifying our fixture manipulation classes Fixed deleted Beginner::destroy(). It looks like that was not correctly merged into this MR! So I think you can quite easily address that #15.1 😊

Assigning to @ted.bowman for #15.2 📌 Refactor exception architecture Fixed

We can continue working on this but I think it should land in 3.x not 2.x see 🌱 Tag 8.x-2.7, and move development to new 3.x branch to focus on Core inclusion Fixed for reasons

Based on #22 … which means this is making 🐛 Unhandled Composer Stager exceptions leave the update process in an indeterminate state Fixed more blocked.

Per #26, this is now unpostponed again.

And the title can now omit that prefix too 😅

The 3.0.x branch is open, let's continue this!

@omkar.podey, could you please open a MR that applies to 3.0.x? 🙏

Down to 1 failing test.

Well done!

Differences between old and new MR:

1. The original MR is +297, -646, the new one is +297, -595. That's because since the original MR new validators have been added whose tests also need to be updated (for example package_manager/tests/src/Kernel/ComposerPatchesValidatorTest.php).
2. I spotted a few small problems: some use statements should have been removed and were removed in the original MR, but not anymore in the new one. Fixed that.
3. ScaffoldFilePermissionsValidatorTest is now correctly handling ApplyFailedException👍
4. \Drupal\Tests\automatic_updates\Kernel\UpdaterTest::testCommitException() is no longer being removed; @tedbow had originally removed this. But … AFAICT it's sensible to keep this, so 👍
5. A clear remaining missing piece: documentation. Both a change record and package_manager.api.php updates are still missing.

I also spotted that @tedbow had left 2 detailed, thoughtful comments with observations/questions/suggestions on the old MR. I quoted those into the new MR and responded to them in detail.

This is more accurate, but it's definitely blocked on @tedbow still like described in #33.

@Wim Leers I closed out all issues except one on the MR. In that issue I just accepted one of you suggestions for a class name.

Let me know if you need anything else

Nope, I don't! 👍

I'll let @omkar.podey handle that last bit, since he worked on this before — I've just been a reviewer on this issue really 😊

Given that we're in the final phase here and @tedbow is still on board, removing the tag, despite the summary still saying … 🙈

🚀

Looking good now, with the exception of a single trivial remark: make StageException::$stage public readonly, just like ✨ Make StageEvent::$stage a public readonly property, and remove getStage() Fixed did a ~week ago 😊 (i.e. long after that code was written for this MR.)

Also:

+341, -640

👍

Only things left:

1. change record
2. update for package_manager.api.php

So: 99% done!

created Change Record →

Wim Leers → closed merge request !656

1. Thanks for the change record looks good, just added refinements 👍
2. The documentation updates are extremely minimal, but that's because apparently ApplyFailedException is not at all documented yet ⇒ out of scope here ⇒ adding to scope of 📌 Define the Package Manager API (package_manager.api.php is outdated) Fixed .
3. Found one last thing that could use public readonly just like I spotted in #38, but … I'll let the committer decide, and I left a suggestion the reviewing committer can apply to make that happen 👍

Let's get this across the finish line! 🚀

This is a partial review. I think I would like final review from @tedbow...

Just applied the suggestion I had mentioned in #43, since @phenaproxima made similar remarks. (After checking that he +1'd it.) That allowed me to close 3 threads (1 of mine, 2 of his).

This will cause tests to fail, because some ->getEvent() occurrences will need to change to ->event. I'll leave that (as well as @phenaproxima's other feedback) to @omkar.podey to solve 😊

I'm really glad @phenaproxima is pointing out gaps in the docs in his review! 😊

Docs have been updated 👍

But one piece of @phenaproxima's feedback has not yet been addressed: https://git.drupalcode.org/project/automatic_updates/-/merge_requests/71...

… and then in re-checking that, I spotted what I think is a bug 🐛😅

Let's see if @phenaproxima can spot something else — I can't! 🤓👍

Found a few small things.

However...the word "epic" is often overused with regard to the code changes being done in any particular merge request, but in this case, it's TRULY epic. 39 changed files, hundreds of lines removed, and so many bad, confusing decisions undone. I can't wait to merge this. 😍

Where I'm leaving this:

There are three things that need to be clarified. A couple just need a comment, and one should probably be lightly refactored to avoid serious confusion down the line.

Beyond that, I think this is good to go. I made a few changes in CronUpdaterTest and PackageManagerKernelTestBase to remove repetition, but nothing too disruptive.

I have tried to answer your questions @phenaproxima, back to you

My work is done here. I'll commit when someone re-RTBCs 🚢

Thanks all the work everyone , thanks for all the reviews too, finally 🎉 🚢

FINALLY!!!

So good to have this done. 🎉 🤓 🤩 🚢

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