In #3485896: Hook ordering across OOP, procedural and with extra types i.e replace hook_module_implements_alter we introduced attributes and order classes to reorder hook implementations.
New OOP hook implementations are targeted with [$class, $method].
Procedural implementations are targeted with [ProceduralCall::class, $function], which is awkward.
The ProceduralCall class should rather be seen as something internal, which we had to introduce to work properly with event dispatcher. We might even remove it in #3506930: Separate hooks from events or a follow-up issue.
We must be able to target procedural hooks without using the ProceduralCall class name.
Either allow to pass (just) a function name to the respective attributes and order classes.
OR have dedicated attributes and classes.
Internally, for now, we can convert these to insert ProceduralHook::class.
That class can be marked as internal (EDIT: This is already the case).
Show commandsStart 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:
changes
Comments
Comment #2
nicxvan commentedI have been thinking about this since I created this and I'm really not sure we should do this.
Comment #3
donquixote commentedFor now we need to target procedural hooks by setting ProceduralCall as the class name, which is awkward.
With #3506930: Separate hooks from events, which includes #3522211: Drop/replace the ProceduralCall class for hooks, we drop the class ProceduralCall.
We then either need #[ReOrderHook(function: ...)], or we need a separate attribute #[ReOrderProceduralHook($function)] or #[ReOrderHookFunction($function)]. Or we could pass $module instead of $function.
The main argument for a separate attribute would be to keep the other arguments required in the regular #[ReOrderHook] attribute.
In the same way we would need OrderBefore(function: ...) or OrderBeforeProcedural($module)
Comment #4
donquixote commentedChoosing a more open-ended issue title
Comment #5
donquixote commentedComment #6
donquixote commentedComment #7
donquixote commentedComment #8
donquixote commentedIt was argued in slack that we don't need to target _only_ a procedural implementation with e.g. #[RemoveHook].
Instead, one could simply target all implementations of the module, because most modules will either be fully on OOP or fully on procedural hooks.
One reason I can think of to support removing only a procedural implementation is if a module wants to work with two major versions of another module.
In that case, it may want to remove the procedural implementation from version 1 of the other module, but also remove a specific OOP implementation (but not all) from version 2 of that other module.
---
It was also argued that we should not add more attribute classes like RemoveProceduralHook, but rather add parameters to existing ones.
The counter-argument here would be that changing the signature of an existing class can be a painful thing to do with regard to BC, especially if we support named arguments passing, and if we want to add or remove more parameters in the future.
Introducing new classes tailored to specific cases is a lot easier.
Also, this means that we can keep parameters required, and we don't need to explain which combination of parameters is allowed or not.
Comment #9
nicxvan commentedComment #12
donquixote commentedI created a preview MR.
No tests, and more attributes than we should keep in the end.
The goal is to look at the different options we have, and decide what we like best.
Comment #13
nicxvan commentedWe can likely close this when separate hooks from event dispatcher lands.
Comment #14
donquixote commentedThe ProceduralCall class still feels alien to me.
Comment #15
nicxvan commentedI know, I think this is a good compromise, it is now literally a stub just for remove and reorder.
I really think we need a stronger reason besides we don't like it.
This MR adds 4 attributes to remove one stub class and there is no clear idea how to release those change with bc.
How will contrib remove our reorder a hook and support 11.0, 11.1, 11.2 and whichever version this ends up in?
I really think this won't change, but I'll leave this open for a bit so we can discuss further.
Comment #16
nicxvan commentedI closed the other as a duplicate of this.
I still think we should clear this for the reasons in 15 but wasn't too leave it open for discussion.
Comment #18
donquixote commentedI guess this kind of change would have needed to be done earlier, when we introduced the ordering attributes.
Comment #19
nicxvan commentedYeah I agree.