[pp-1] Create new attribute to make reordering procedural hooks easier.

Created on 29 March 2025, 2 months ago

Problem/Motivation

Once reordering is in, reordering procedural hooks is a bit awkward.
Let's make an attribute to make that easier.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

πŸ“Œ Task
Status

Postponed

Version

11.0 πŸ”₯

Component

base system

Created by

πŸ‡ΊπŸ‡ΈUnited States nicxvan

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

  • 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

    Choosing a more open-ended issue title

  • πŸ‡©πŸ‡ͺ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.

  • πŸ‡ΊπŸ‡ΈUnited States nicxvan
  • πŸ‡©πŸ‡ͺ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.

Production build 0.71.5 2024