Closed (fixed)
Project:
Drupal core
Version:
11.3.x-dev
Component:
other
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Issue tags:
Reporter:
Created:
11 Aug 2025 at 13:00 UTC
Updated:
10 Dec 2025 at 01:59 UTC
Jump to comment: Most recent
Show commands
changes
Comments
Comment #3
quietone commentedLeave this to me for a week or two, thanks.
Comment #4
quietone commentedComment #5
quietone commentedComment #6
quietone commentedthe scope expanded a bit to include core/tests/Drupal/KernelTests/Core/Hook/HookAlterOrderTest.php and core/tests/Drupal/KernelTests/Core/Hook/HookOrderTest.php. These are the tests using the files in [A-D]AlterHooks.php and [A-D]Hooks.php, anmd it made sense to update them as I figured what the tests were doing.
Comment #7
quietone commentednicxvan and donquixote, thanks for the comments in Slack about this.
Your are correct that the commenting is rather explicit and sometimes repetitive. This is the first time I have looked closely at these tests and I added notes as I went through each to help me keep track of what was going on. Key for me was adding the 'default' ordering in the test doc block followed by a list of the ordering/removal operations. That made a huge difference to understanding each test. At the same time I had notes on each hook implementation noting where it got changed. That helped to see how all the hooks in a module 'fit' into a test. Then I tidied all those notes into comments.
The typo for 'its' has been fixed.
Comment #8
nicxvan commentedAs mentioned in 7 I reviewed this and discussed in slack, just took another look and all feedback has been addressed.
It's a bit repetitive, but so is the actual test an implementations.
These are really hard to keep straight so the extra documentation is super helpful for tracking the expected order.
Comment #9
longwaveThese tests have always been very confusing, so adding the explicit explanations really helps. I read through the diff and they all make more sense than before!
Committed and pushed d211b2521f9 to 11.x and 484b56abd13 to 11.3.x. Thanks!
Comment #14
quietone commentedI am glad to see this committed. It was challenging and time consuming to add the comments especially as I have not been involved in the many hook conversion issues. From my perspective, this is a good example of a case where test methods should be documented before commit.
Comment #15
nicxvan commentedIn general I agree, but we made a couple of attempts in the original issue and we couldn't get consensus on the best way so we decided to do it in a follow up, sorry I missed creating it and thank you for pushing this across.
I just took a peek at the originating issue to see if I could find the discussion, but I couldn't find it in the 923MR comments lol.