- Issue created by @nicxvan
- πΊπΈUnited States nicxvan
I have been thinking about this since I created this and I'm really not sure we should do this.
- π©πͺGermany donquixote
For now we need to target procedural hooks by setting ProceduralCall as the class name, which is awkward.
With π HookCollectorPass registers event listeners but they do not follow the required calling convention Active , which includes π Drop/replace the ProceduralCall class for hooks Active , 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)
- π©πͺGermany donquixote
It 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. - π©πͺGermany donquixote
donquixote β changed the visibility of the branch 3516146-rethink-procedural-hook-ordering to hidden.
- @donquixote opened merge request.
- π©πͺGermany donquixote
I 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.