Problem/Motivation
Critical due to ordering regression with extra types and mixed implementations.
Currently there are three different ways to order hooks roughly in lifecycle order:
- During build time HookCollectorPass calls
hook_module_implements_alteris called for procedural hooks for backwards compatibility reasons. This is not enough. It needs to be called for all hooks because once Drupal core has all hooks ported (goal is D11.1) then code still using procedural code (likely most of contrib and custom) won't be able to order against core hooks. - In #3442009: OOP hooks using attributes and event dispatcher a new mechanism was added to order hook implementations: service providers can change the priority
of event listeners and a HookOrder helper class is provided to make the common operations (first/last/before/after) easy. The issue is it does not work with alter extra hooks. - Run time
ModuleHandler::reOrderModulesForAltercallshook_module_implements_alterwith the module names of OOP hook implementations. This only happens for extra hooks like form_FORM_ID_alter and form_FORM_BASE_ID_alter. This is why HookOrder is confusing:hook_module_implements_alteralways works or will after this patch but HookOrder currently doesn't work for extra hooks and it's out of scope for this issue to fix that.
Steps to reproduce
Proposed resolution
- Call
hook_module_implements_alterwith every hook not just procedural ones. - #3485658: Add an advisory AltersImplementations attribute Add an advisory
ImplementsAlterattribute which can be used on <hook_module_implements_alterimplementations to indicate which hook this alter is using. - #3485659: Remove HookOrder Remove HookOrder temporarily
Remaining tasks
From the above list #1 is done. It should be noted that in addition to the tests here, this is tested here #3483599: Convert all procedural hook implementations to Hook classes with the ckeditor reordering that happens.
These can be followups
#2 AFAIK needs to be done and then these two tested meanwhile
#3.1 Removal is RTBC.
#3.2 Longer term resolution is here #3485896: Hook ordering across OOP, procedural and with extra types i.e replace hook_module_implements_alter
User interface changes
Introduced terminology
API changes
Data model changes
Release notes snippet
Issue fork drupal-3484754
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:
- 11.x
compare
- 3484754-ordering-of-alter
changes, plain diff MR !10039
Comments
Comment #2
ghost of drupal pastComment #3
ghost of drupal pastComment #4
ghost of drupal pastComment #5
nicxvan commentedComment #6
nicxvan commentedComment #8
ghost of drupal pastComment #9
ghost of drupal pastComment #10
ghost of drupal pastComment #11
nicxvan commentedComment #12
nicxvan commentedComment #13
nicxvan commentedComment #14
nicxvan commentedComment #15
nicxvan commentedComment #16
nicxvan commentedComment #17
nicxvan commentedComment #18
nicxvan commentedComment #19
nicxvan commentedComment #20
godotislate1 small nit and a question.
Comment #21
nicxvan commentedApplied and replied, let me know what you think!
Comment #22
godotislateTest failure is probably unrelated and needs a re-run. Fine to go to RTBC after test passes.
Comment #23
nicxvan commentedTests passed rtbc per 22
Comment #24
nicxvan commentedI rebased and fixed the deprecation test. I'll keep an eye on the tests.
With #3482618: ModuleHandler->alterDeprecated does not properly handle deprecated OOP hooks the alter on these hooks is now respected so the order in the deprecation is inverted from the other issue.
Leaving RTBC since that's the only change and it was expected once the mentioned issue was merged. If it needs another review let me know.
Comment #25
nicxvan commentedComment #26
nicxvan commentedComment #29
catchI think this is good to get us to a point of minimal change for 11.1.x and then we can figure out hook ordering for all-OOP hooks will look like a bit further along.
Committed/pushed to 11.x and cherry-picked to 11.1.x, thanks!
Comment #31
fabianx commentedRTBC + 1 from me - post merge
This implementation is much cleaner.