Hook ordering across OOP, procedural and with extra types

Created on 6 November 2024, 5 months ago

Problem/Motivation

We needed to preserve hook_module_implements_alter in it's current form to handle all current ordering scenarios.

📌 Remove HookOrder Active HookOrder was temporarily removed because it does not quite work with extras.
🐛 Ordering of alter "extra hooks" is a gigantic mess Active Resolves the BC break (this issue is the followup to address point 3)
📌 [PP-1] Determine how to implement Form Alters with attributes. Active is related since it is the most common form (lol) of an alter with extra types.

Steps to reproduce

Order hook_form_FORM_ID_alter on a form_alter

Proposed resolution

Add a base parameter to #Hook #[Hook('form_FORM_ID_alter', base: 'form_alter')]
Set this automatically in FormAlter depending on the parent hook
Use this parameter to build a map that stores drupal_hook.form_alter listeners and filter out runtime the extra types
Add HookOrder back in if possible

Remaining tasks

Agree on approach

User interface changes

N/A

Introduced terminology

N/A

API changes

N/A

Data model changes

N/A

Release notes snippet

N/A

📌 Task
Status

Active

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

Merge Requests

Comments & Activities

  • Issue created by @nicxvan
  • 🇺🇸United States nicxvan
  • Merge request !10497Resolve #3485896 "Hook ordering across" → (Open) created by nicxvan
  • Pipeline finished with Failed
    4 months ago
    Total: 192s
    #363745
  • Pipeline finished with Failed
    4 months ago
    Total: 131s
    #363751
  • 🇺🇸United States nicxvan
  • 🇺🇸United States nicxvan
  • Pipeline finished with Failed
    4 months ago
    Total: 234s
    #363887
  • Pipeline finished with Failed
    4 months ago
    Total: 122s
    #363894
  • Pipeline finished with Failed
    4 months ago
    Total: 142s
    #364429
  • Pipeline finished with Failed
    4 months ago
    Total: 1242s
    #364431
  • Pipeline finished with Failed
    4 months ago
    Total: 121s
    #364734
  • Pipeline finished with Failed
    4 months ago
    Total: 127s
    #364798
  • Pipeline finished with Failed
    4 months ago
    #364808
  • Pipeline finished with Failed
    4 months ago
    Total: 2217s
    #364975
  • Pipeline finished with Failed
    4 months ago
    Total: 149s
    #365885
  • 🇺🇸United States nicxvan
  • I think that introducing (or re-introducing?) 5 different attributes here to control hook order makes the scope of the issue quite large, at least the testing aspect of it.

    But before that, I think it'd be helpful to define what the expectation is of how ordering should (or will?) work. I understand the goal is to replace hook_module_implements_alter in a way that works with both procedural and OOP hooks, but I think a full example with the variations of hook implementations and how exactly they can be ordered in a business sense is something missing from the IS.

  • 🇺🇸United States nicxvan

    This issue really isn't ready for review yet, we still need the runtime BC layer, but you can see how it works in ckeditor here:

      #[Hook('form_filter_format_form_alter')]
      #[HookOrderGroup(['form_filter_format_add_form_alter', 'form_filter_format_edit_form_alter'])]
      #[HookAfter(['editor', 'media'])]
    

    We kind of have to do this all at once, we're only replacing the one implementation in this issue though.

  • 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
  • Pipeline finished with Failed
    4 months ago
    Total: 447s
    #367152
  • Pipeline finished with Canceled
    4 months ago
    Total: 126s
    #367173
  • Pipeline finished with Failed
    4 months ago
    Total: 238s
    #367193
  • Pipeline finished with Failed
    4 months ago
    Total: 430s
    #367990
  • Pipeline finished with Failed
    4 months ago
    Total: 170s
    #368116
  • Pipeline finished with Success
    4 months ago
    Total: 881s
    #368198
  • Pipeline finished with Canceled
    4 months ago
    Total: 294s
    #368225
  • 🇺🇸United States nicxvan
  • Pipeline finished with Success
    4 months ago
    #368231
  • Pipeline finished with Failed
    4 months ago
    Total: 1015s
    #368539
  • 🇨🇦Canada Charlie ChX Negyesi 🍁Canada

    (please continue not crediting me)

  • Pipeline finished with Success
    4 months ago
    Total: 1327s
    #368564
  • 🇺🇸United States nicxvan
  • Merge request !10558Resolve #3485896 "Attempt simplification" → (Open) created by nicxvan
  • 🇺🇸United States nicxvan

    nicxvan changed the visibility of the branch 3485896-attempt-simplification to hidden.

  • Pipeline finished with Failed
    4 months ago
    Total: 180s
    #368727
  • Pipeline finished with Canceled
    4 months ago
    Total: 65s
    #368737
  • 🇺🇸United States nicxvan

    Updating the IS an CR.

  • 🇺🇸United States nicxvan

    nicxvan changed the visibility of the branch 3485896-attempt-simplification to active.

  • 🇺🇸United States nicxvan

    nicxvan changed the visibility of the branch 3485896-hook-ordering-across to hidden.

  • 🇺🇸United States nicxvan
  • 🇺🇸United States nicxvan
  • 🇺🇸United States nicxvan
  • Pipeline finished with Failed
    4 months ago
    Total: 681s
    #369981
  • 🇨🇭Switzerland berdir Switzerland

    I think we should consider to convert all implementations in core of this to verify the concept. There aren't that many and then we can immediately deprecate implementations hook_module_implements_alter().

    That said, there are at least two common use cases for hook_module_implements_alter(). One is covered here, it's ensuring that certain hooks are called before/after others. The second is completely replacing hook implementations. This might be more common in custom code for some advanced use cases and maybe this is already possible through a service provider or so, but we need to document that if we want to deprecate the hook.

    Looking through common core and contrib implementations as well as those in my project.

    core:
    * content translation is a trivial first/last (I assume if two modules want to be first then it would keep the order if it would have by default between those two).
    * layout_builder is also a trivial last
    * layout_builder_test is a before layout_builder.
    * navigation has two, page_top is a trivial last, but the second part is an implementation of the second use case, it replaces layout_builder. so that use case does in fact exist in core.
    * page_cache_form_test is also a variation of that, it removes system
    * common_test_module() I think is just a general test for it, if we have tests for the new system, we could keep it just for BC testing?
    * module_test too is more test edge cases that I think we should just use for BC testing.
    * workspaces is a fun one: It moves itself first *but* also moves content_moderation "firster". How would that look with the new system?

    contrib:
    * address: last
    * field_group: last
    * google_tag: last + remove google_analytics
    * metatag: last
    * monitoring: remove a requirements hook for test purposes
    * paragraphs: last
    * webform: last (same hook as paragraphs)
    * workbench_email: last (really, after content_moderation)

    And one custom implementation that removes a hook.

    Looking through this, I'm wondering about the group stuff. It makes sense that we tackled this by looking at the most complex implementation and made sure that we support that. But, I think such a complex thing that's based on two different hook that we have to put in a group is very, very rare. This is literally the only implementation that I found that doesn't just put itself first or last. We could either say that such a special case needs to handle this itself by altering the service container, or much simpler: We just solve it like everyone else does it and put ckeditor5 last.

    I didn't review the implementation at all, but I suspect it would remove a considerable amount of complexity if we don't need to support that group thing? Thoughts?

  • 🇷🇴Romania amateescu

    workspaces is a fun one: It moves itself first *but* also moves content_moderation "firster". How would that look with the new system?

    Workspaces needs the ability to implement the entity presave hook twice, one that runs first and one that runs last :) The current code that moves CM's implementation before is just a workaround.

  • 🇺🇸United States nicxvan

    @berdir thanks for your review a lot of good points! I'll reply more fully once I've thought through some things.

    @amateescu great to know! That would be super easy in this implementation then!

    Two hook attributes on the same method, one first, one last.

  • 🇨🇦Canada Charlie ChX Negyesi 🍁Canada

    > Two hook attributes on the same method, one first, one last.

    Or , depending , one hook implementation which runs first and one that runs last -- we now have the ability to implement the same hook twice. Yay.

    Removing the group stuff as it would make alter quite a bit simpler. But I do not advocate for doing so, I do not advocate for anything.

  • 🇷🇴Romania amateescu

    Or , depending , one hook implementation which runs first and one that runs last -- we now have the ability to implement the same hook twice. Yay.

    Yup, that's what we need for Workspaces, to split the current implementation in two separate methods.

  • 🇺🇸United States nicxvan

    Yup, that's what we need for Workspaces, to split the current implementation in two separate methods.

    Even easier, I'll reach out if I have questions.

  • 🇺🇸United States nicxvan

    I think we should consider to convert all implementations in core of this to verify the concept.

    I agree, I was concerned about scope, I reached out to @catch on slack and since there are only a few hooks we can convert them here except common_test and module_test

    There aren't that many and then we can immediately deprecate implementations hook_module_implements_alter().

    Minor correction here, we will not deprecate hook_module_implements_alter in the traditional sense, we will deprecate hook_module_implements_alter without the #[LegacyHook] attribute. In order to support older versions of Drupal contrib / custom will need to keep their implementation and tag it.

    The second is completely replacing hook implementations

    Good catch! I think we can handle that here as well though with a more simple remove or replace parameter. (What is replace other than remove the other one and add mine? So with this you can remove theirs and on the same attribute provide the replacement #[Hook])

    Opinions on the name of the parameter are welcome.

    I think functionally it has to be remove, but semantically replace might be easier to understand as a DX

    In the case that you only wanted to remove you would just put an empty implementation and replace the hook you wanted to remove.

    There is one caveat to this approach that could be addressed in the future followup. Remove for a given module / hook combination would be global, meaning if a module implements a hook multiple times all would be removed if this parameter is passed.

    Looking through this, I'm wondering about the group stuff. It makes sense that we tackled this by looking at the most complex implementation and made sure that we support that. But, I think such a complex thing that's based on two different hook that we have to put in a group is very, very rare. This is literally the only implementation that I found that doesn't just put itself first or last. We could either say that such a special case needs to handle this itself by altering the service container, or much simpler: We just solve it like everyone else does it and put ckeditor5 last.

    Yes, this does complicate things and was where a lot of the effort went. It is an edge case, but I would say it's worth keeping because it does solve the extra types use case. Most of the comments on the MR from catch were about explaining this so it definitely needs more comments.
    Putting it last won't work because it's across extra types, so alter doesn't know how to move it. In fact I think this will solve some unique ordering issues for alter hooks.

  • 🇺🇸United States nicxvan

    Assigning to myself per the assignment policy.
    I am working on addressing comments on the MR and converting the rest of core.

    I will also take a look at adding the remove / replace parameter name TBD

  • 🇨🇭Switzerland berdir Switzerland

    > Putting it last won't work because it's across extra types, so alter doesn't know how to move it. In fact I think this will solve some unique ordering issues for alter hooks.

    I didn't look at the implementation yet. I see that it gets tricky with multiple types either way. I assumed we could just give a very low priority/high weight, but we don't really have that, just an array. And we always process the first hook first, so ckeditor5 would be first since it implements the base hook that's specified first.

    One option would be to register the ckeditor5 hook twice, once as add and once as edit form id, instead of using the base form id. Then each could depend on the respective other and they'd be same hook?

  • 🇺🇸United States nicxvan

    I tried that, it didn't work cause it then gets called multiple times since it's new hooks.

  • 🇺🇸United States nicxvan

    Ok I rebased to fix the navigation performance test.
    I also converted all core implementations except common_test and module_test which we've not converted anything for.
    I also did not convert module_handler_test_all1 since it is testing hmia.
    I did not convert workspaces as I am waiting on: 📌 Cleanup hook implementations in the Workspaces module Active

  • Pipeline finished with Failed
    3 months ago
    Total: 200s
    #382806
  • Pipeline finished with Success
    3 months ago
    Total: 1055s
    #382809
  • 🇺🇸United States nicxvan
  • 🇺🇸United States nicxvan

    I'm wondering if replacements is more confusing since it is really just removing those implementations. It could be called removals.

    Once a consensus is reached I can update it. In the mean time I finished converting navigation and wrote tests.

    I also added a new CR just for this parameter.
    https://www.drupal.org/node/3496786

  • 🇺🇸United States nicxvan

    Once that workspaces issue is in I'll convert it, I will look at deprecating HMIA too here since we've converted everything but the test ones.

  • 🇺🇸United States nicxvan

    nicxvan changed the visibility of the branch 3485896-deprecatehmia to hidden.

  • Merge request !10756Resolve #3485896 "Deprecatehmia" → (Open) created by nicxvan
  • 🇺🇸United States nicxvan

    Workspaces is in, let me convert that one too!

  • 🇺🇸United States nicxvan

    nicxvan changed the visibility of the branch 3485896-deprecatehmia to hidden.

  • Pipeline finished with Failed
    3 months ago
    #382833
  • 🇺🇸United States nicxvan

    Ok workspaces has been converted, I also created yet another CR for deprecating hook_module_implements_alter, I also created a new branch that I hid to add the deprecation message where I think it needs to go, I'm running that test now, if we think we should deprecate as part of this issue it's easy to move it over to the canonical branch.

  • Pipeline finished with Success
    3 months ago
    Total: 950s
    #382838
  • 🇺🇸United States nicxvan

    I think I'm going to rename replacements remove.

    I think longer term it will be confusing to have it called something it's not doing.

  • 🇺🇸United States nicxvan
  • 🇺🇸United States nicxvan
  • Pipeline finished with Failed
    3 months ago
    Total: 283s
    #382894
  • Pipeline finished with Failed
    3 months ago
    Total: 6916s
    #382897
  • Merge request !1Resolve #3485896 "Deprecatehmia" → (Closed) created by nicxvan
  • Pipeline finished with Failed
    3 months ago
    Total: 104s
    #383252
  • 🇺🇸United States nicxvan

    If you would like to see what is needed on top of this change to deprecate hook_module_implements_alter:

    https://git.drupalcode.org/issue/drupal-3485896/-/merge_requests/1/diffs

  • 🇺🇸United States nicxvan
  • Pipeline finished with Canceled
    3 months ago
    Total: 561s
    #383259
  • 🇷🇴Romania amateescu

    Pushed a commit to fix Workspaces' presave implementation, we don't want to run the whole thing twice :)

  • 🇺🇸United States nicxvan

    That broke Drupal\Tests\content_moderation\Kernel\WorkspacesContentModerationStateTest

  • 🇷🇴Romania amateescu

    The current code from HMIA doesn't run the implementation twice, it moves it first then moves CM's implementation before it. I'll look at the test failure tomorrow :)

  • 🇺🇸United States nicxvan

    Ah I misunderstood what you were saying here:

    Workspaces needs the ability to implement the entity presave hook twice, one that runs first and one that runs last :) The current code that moves CM's implementation before is just a workaround.

  • 🇷🇴Romania amateescu

    I looked into it and the test failure is genuine, \Drupal\workspaces\Hook\EntityOperations::entityPresaveLast() runs before \Drupal\content_moderation\Hook\ContentModerationHooks::entityPresave().

    What I found is that ModuleHandler::invokeAllWith() executes the callbacks in a foreach keyed by module name, and ModuleHandler::invokeMap['entity_presave'] looks like the screenshot attached.

  • 🇺🇸United States nicxvan

    And what does it look like with my version? Cause that passed.

  • 🇺🇸United States nicxvan

    That should do it! I'll keep an eye on tests.

  • Pipeline finished with Failed
    3 months ago
    Total: 454s
    #385132
  • 🇺🇸United States nicxvan
  • 🇺🇸United States nicxvan

    Ok this is ready for review again

  • 🇺🇸United States nicxvan

    Cleaned up the remove hook bit and the override order bit.

    I'll update the IS, CRs and docs later tonight or over the weekend. It's still ready for review.

  • Pipeline finished with Failed
    3 months ago
    Total: 195s
    #385376
  • Pipeline finished with Success
    3 months ago
    Total: 1916s
    #385381
  • 🇺🇸United States nicxvan
  • 🇺🇸United States nicxvan
  • 🇺🇸United States nicxvan
  • Pipeline finished with Failed
    3 months ago
    Total: 215s
    #385517
  • 🇺🇸United States nicxvan
  • 🇺🇸United States nicxvan

    I've updated CRs and the IS.

  • 🇺🇸United States nicxvan

    Renamed to ReOrderHook

  • Pipeline finished with Failed
    3 months ago
    Total: 166s
    #385812
  • 🇺🇸United States nicxvan
  • 🇺🇸United States nicxvan
  • Pipeline finished with Failed
    3 months ago
    Total: 262s
    #387422
  • Pipeline finished with Failed
    3 months ago
    Total: 347s
    #387423
  • Merge request !10824Deprecatehmiav6 → (Open) created by nicxvan
  • Status changed to Needs review 3 months ago
  • Merge request !2Deprecatehmiav6 → (Closed) created by nicxvan
  • 🇺🇸United States nicxvan
  • 🇺🇸United States nicxvan
  • 🇺🇸United States nicxvan

    I think we should just deprecate HMIA here, it's only +94 -67 https://git.drupalcode.org/issue/drupal-3485896/-/merge_requests/2/diffs

  • Pipeline finished with Failed
    3 months ago
    Total: 153s
    #388028
  • 🇺🇸United States nicxvan

    Tagging for the deprecation question since this is unique.

  • 🇺🇸United States nicxvan
  • 🇺🇸United States nicxvan
  • Pipeline finished with Failed
    3 months ago
    Total: 151s
    #392211
  • 🇺🇸United States nicxvan
  • 🇺🇸United States nicxvan
  • 🇺🇸United States nicxvan
  • 🇺🇸United States nicxvan
  • 🇺🇸United States nicxvan
  • 🇺🇸United States nicxvan
  • 🇺🇸United States nicxvan
  • The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

    This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

    Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

  • 🇺🇸United States nicxvan

    I will rebase this if necessary but this Needs Review either way.

  • Merge request !3Deprecatehmiav6 → (Closed) created by nicxvan
  • Pipeline finished with Failed
    2 months ago
    Total: 157s
    #404975
  • Pipeline finished with Failed
    2 months ago
    Total: 180s
    #404980
  • 🇺🇸United States nicxvan
  • 🇺🇸United States nicxvan
  • Pipeline finished with Failed
    about 2 months ago
    Total: 186s
    #413793
  • Pipeline finished with Failed
    about 2 months ago
    Total: 244s
    #413829
  • 🇩🇪Germany donquixote

    Some feedback.

    I don't like attributes that are not related to the symbol they are attached to.
    #[ReOrderHook] and #[RemoveHook] can be placed anywhere with the same effect, so perhaps there should be a different way to do this?

  • 🇺🇸United States nicxvan

    I don't like attributes that are not related to the symbol they are attached to.
    #[ReOrderHook] and #[RemoveHook] can be placed anywhere with the same effect, so perhaps there should be a different way to do this?

    While I agree with your sentiment in the general sense, in this specific case these two attributes are likely going to be super rare.
    Further, when they are implemented, they will almost always have a specific hook that they are being enacted FOR, so there is a specific hook they are related to. It will be extraordinarily rare for one of them to be just alone, in that case you can throw them on a hook class with an __invoke method that does nothing.

    Evidence that these are rare:
    RemoveHook Has one implementation in all of core.
    ReOrderHook Also has one implementation in all of core.

    The next question of course is if it is so rare why have it, is there another way to do what they are doing?
    The answer to that is they both provide a specific piece of functionality that hook_module_implements_alter provided that is crucial we maintain for the modules that maintain it. We spent a significant amount of time exploring options and this is the cleanest we came up with.

    If you have an alternate suggestion I'm all ears.

  • Pipeline finished with Failed
    about 2 months ago
    Total: 264s
    #420214
  • 🇺🇸United States nicxvan

    I created this CR @larowlan's request: https://www.drupal.org/node/3505563
    Turns out it is already covered by BC policy: https://www.drupal.org/about/core/policies/core-change-policies/bc-polic...

    I have requested it get deleted.

  • 🇨🇭Switzerland berdir Switzerland

    I've given up trying to argue that we shouldn't support the complex grouping stuff, but I still think we can at least get rid of the use case we have in core: 📌 Simplify ckeditor5_module_implements_alter() in favor of checking for ckeditor5 in media module Active . It might not save much in lines of code (given that we have to keep support for such use cases) but it makes it IMHO self-contained and easier to understand.

  • 🇺🇸United States nicxvan

    Not to repeat the whole comment, but I think that change should be postponed on this since for now that is a concrete way to show how groups work. I agree long term we can remove it, but since grouping is part of the most complex bit having a concrete example makes this issue easier to grok.

  • 🇺🇸United States nicxvan

    I rebased the main branch, I'll rebase the deprecation later.

  • Pipeline finished with Success
    about 1 month ago
    Total: 1231s
    #426119
  • Merge request !11224Deprecatehmiav7 → (Open) created by nicxvan
  • Merge request !4Deprecatehmiav7 → (Closed) created by nicxvan
  • 🇺🇸United States nicxvan

    nicxvan changed the visibility of the branch deprecatehmiav6 to hidden.

  • Merge request !11233Deprecate hmia fresh → (Open) created by nicxvan
  • Merge request !5Deprecate HMIA → (Closed) created by nicxvan
  • 🇺🇸United States nicxvan

    I recreated the deprecation branch, rebasing has become painful so here is a patch with the changes only.
    I'm going to hide it, but I'm attaching it here for posterity.

  • 🇺🇸United States nicxvan

    nicxvan changed the visibility of the branch deprecatehmiav7 to hidden.

  • Pipeline finished with Failed
    about 1 month ago
    Total: 167s
    #426762
  • 🇺🇸United States nicxvan

    I had missed a couple of test updates, here is an updated patch.

  • 🇳🇿New Zealand quietone

    I deleted the CR titled, "Delete this CR".

  • 🇺🇸United States nicxvan

    Thanks!

  • 🇺🇸United States nicxvan

    @larowlan signed off on the deprecation method on slack so I'm removing framework manager tag. https://drupal.slack.com/archives/C079NQPQUEN/p1740180561937599?thread_t...

    From a framework manager POV I'm happy with the approach from a technical perspective. From the POV of should it be one issue or two, it's probably a release manager question. My 2c would be to do it in one issue and sidestep the risk of missing shipping the BC layer.

    Leaving the release manager tag for that decision.

    I'll update the IS later.

  • 🇺🇸United States nicxvan

    @larowlan signed off on the deprecation method on slack so I'm removing framework manager tag. https://drupal.slack.com/archives/C079NQPQUEN/p1740180561937599?thread_t...

    From a framework manager POV I'm happy with the approach from a technical perspective. From the POV of should it be one issue or two, it's probably a release manager question. My 2c would be to do it in one issue and sidestep the risk of missing shipping the BC layer.

    Leaving the release manager tag for that decision.

    I'll update the IS later.

  • Pipeline finished with Failed
    about 1 month ago
    Total: 785s
    #432357
  • 🇺🇸United States nicxvan

    @donquixote thank you for the thorough review.

    I applied several suggestions and renamed a couple of variables you pointed out were confusing.
    You're not the first person commenting on the term of Group, it'd be a lot of work, but if someone comes up with a better name for hooks that ordered together, I'm all ears.

    There are some questions I want to review and think about.

    There were also several suggestions and reorganizations that I think I see what you were going for but I disagree if they are clearer.

    Specifically renaming the nested foreach in process and gatherOrderInformation, I don't think we should change those.

  • Pipeline finished with Success
    about 1 month ago
    Total: 666s
    #432764
  • 🇺🇸United States nicxvan

    Ok I think we need to rename group and the local variable orderGroup.

    I asked in slack for suggestions and as of this comment @catch, @longwave and @donquixote participated.

    Suggestions were:

    • set
    • extra_types
    • implementations
    • grouped_implementations
    • implementation_groups
    • family

    Since they are the extra types we are ordering against in alter, I think the best option is the following:
    on the ComplexOrder object: extra_types As a local variable on HookCollectorPass: orderExtraTypes.

  • 🇺🇸United States nicxvan

    Ok I renamed group to extraTypes, $orderExtraTypes and $orderedExtraTypes where applicable, I think that might be a lot clearer.

  • Pipeline finished with Failed
    about 1 month ago
    Total: 158s
    #434235
  • 🇬🇧United Kingdom catch

    Unless a good reason emerges which I can't currently think of, I would also lean towards doing the deprecation/bc layer in this issue.

  • 🇺🇸United States nicxvan

    Great thanks! I'll pull that in and then update the IS

  • 🇺🇸United States nicxvan

    nicxvan changed the visibility of the branch deprecateHMIAFresh to hidden.

  • 🇺🇸United States nicxvan

    Ok that has been pushed up

  • Pipeline finished with Success
    about 1 month ago
    Total: 345s
    #435115
  • 🇩🇪Germany donquixote

    Some higher level thoughts.

    Divergence from symfony event system

    We are using the symfony event system, but then we are reinventing the way that these events are declared, how we specify ordering, and how we invoke them.

    Explicit vs calculated numeric priorities

    Atm, if a module uses explicit event subscriber priorities, I suspect it does not really work so well in combination with the dynamic/calculated priorities produced by the hook ordering system proposed here.

    If we want to stick closer to the event system, then the default way to specify ordering would just be to specify explicit priorities. There are reasons why we might want to avoid this, because modules will come up with random numbers like -999, -5 or -1000000. But this also applies to events that we already have in Drupal, and where we are ok with just priorities.

    We could also consider to order event subscribers without setting a calculated priority.
    If we have 10 subscribers with prio = 0, we can just order that array and set it, while the priority remains 0 for each.
    We could either do this later in the process, when the tagged services have already been evaluated and applied as a service parameter, or we rely on the order of tagged service definitions in the container. Not sure if this works.

    Relative ordering

    One thing the symfony event system does not cover is relative ordering.
    I remember older discussions where it was said the relative ordering is not really needed.
    We only had one such case in core (ckeditor5) and here it was not even really required. But perhaps contrib has such cases.

    In the past, with no more than one implementation per module (for regular hooks), relative ordering just meant ordering of module names relative to each other.
    Even with advanced multi-hook alter hooks, where you could already have multiple implementations per module, we would group them by module for ordering, to have something in the format we can pass to hook_module_implements_alter().

    Now we want to order implementations relative to each other, which is more than what we would need if we only want to preserve the old/existing ordering functionality.

    In my older MR, we would order implementations relative to module names, which completely eliminates the possibility of circular conflict.

    Module name per implementation

    Also we need to know the module name for each implementation, which symfony does not really help us with.
    We have to store the impl -> module map in a separate place, outside the actual event subscribers list.

    Order lost when grouping by module

    Currently in ModuleHandler::getHookListeners(), we are grouping implementations by module:

                $this->invokeMap[$hook][$module][] = $callable;
    

    This means that previous ordering by implementation is destroyed, if we have multiple implementations for one module.

    A solution here could be to let ::getHookListeners() return an iterator, which would allow to repeat the same key.

    yield $module_A => $module_A_early_implementations;
    yield $module_B => $module_B_implementations;
    yield $module_A => $module_A_late_implementations;
    

    Of course the `$this->invokeMap` would need to have a different structure in that case. Perhaps it could contain iterator closures, or IteratorAggregate objects.

    Ordering conflicts

    With the system proposed here, there is always the potential for ordering conflicts.

    E.g. A wants to run before B, but B also wants to run before A.
    This type of conflict already existed (in theory) with hook_module_implements_alter(), and it almost never was a problem in practice, so I don't see it as a blocker.

    And then we can have different implementations that want to run first, in that case the original module order decides.
    Again, we already know this edge case problem from hook_module_implements_alter(). But explicit priorities or weights would avoid this fully.

    Alternative

    I want to recap the ordering logic of my older PR, from what I remember.
    This was done without the event system, but in fact it would be very possible to integrate it with the event system, similar to what we do now, but without calculated priorities.

    Basically, we have different levels of ordering, similar to semantic versions:

    • Explicit numeric priority or weight. This would beat everything else.
    • Altered order of modules. This would start with the default module order by module weight, and can be altered with hook_module_implements_alter(). This could either be applied for each weight/prio group, or only for priority = 0.
    • Relative ordering relative to a module name. Instead of asking to run before or after a specific implementation, ask to run before or after a specific module's regular implementations, if the priority/weight is the same.

    So it would look something like this:

    // $impl_by_module order has been altered by hook_module_implements_alter().
    foreach ($imp_by_prio as $prio => $impl_by_module) {
      foreach ($impl_by_module as $module => $module_implementations) {
        foreach ([...$before[$prio][$module], $module_implementations, ...$after[$prio][$module]] as $impl) {
          yield $impl->module => $impl->callback;
        }
      }
    }
    

    The `$module_implementations` would include procedural _and_ OOP implementations, and a derivative of it is passed to hook_module_implements_alter().
    This allowed to convert a procedural implementation with no impact on the ordering.

    (The MR looked more complex due to micro optimization.)

    This would also completely solve "combined" alter hooks, because we can just combine implementations with same prio and module.

  • 🇺🇸United States nicxvan

    Thank you so much for your review and this comment, you've obviously put a lot of thought into this topic over the years and this issue specifically.

    At a high level, I don't see anything here that should block this issue, once this is in there is some opportunity for clean up / expansion, but this is already far broader than a typical issue due to the nature of HMIA. I truly think for this issue the thought should be

    Most of your comments could be addressed in follow ups that are far more narrowly scoped.
    I will create those follow ups shortly with some details to come later.

    We are using the symfony event system, but then we are reinventing the way that these events are declared, how we specify ordering, and how we invoke them.

    Yes, hooks are different so the divergence is expected

    Explicit vs calculated numeric priorities

    These are not subscribers, so again them being different is expected. The initial OOP hook issue did use explicit priorities and that blew up which is why it was pulled out, this issue was specifically borne out of a BC way to allow ordering relatively.

    Relative ordering

    This section only applies when it goes through the legacy ordering, and HMIA can't handle multiple implementations so them getting flattened when run through legacy hmia runtime is acceptable, once all modules convert to this system there will not be an issue.

    Module name per implementation

    Yes, we do that.

    Order lost when grouping by module

    While true, let's handle it in a follow up, multiple implementations are new, going to be rare, and ordering them as a separate entity will be even rarer. That follow up scope will be much more narrow and easier to handle. Let's not hold this up for an edge case of an edge case of a new feature.

    Ordering conflicts

    I agree with your assessment, this is not a blocker, this can also be solved with module weights, or by using ReOrderHook which was not previously available.

    Alternative

    I think this is pure follow up material, again I and Ghost have ideas for further clean up too, but as I've mentioned, this issue scope is already huge, and we know it works, let's not get bogged down in perfection.

  • 🇺🇸United States nicxvan

    Created two follow ups

  • 🇩🇪Germany donquixote

    Thank you so much for your review and this comment, you've obviously put a lot of thought into this topic over the years and this issue specifically.

    I regret the inconsistency in my level of participation and commitment.

    At a high level, I don't see anything here that should block this issue, once this is in there is some opportunity for clean up / expansion, but this is already far broader than a typical issue due to the nature of HMIA. I truly think for this issue the thought should be

    In the MR for this issue we are introducing the different Order classes, including ComplexOrder with the ->extraTypes, which would be obsolete if we use a different model / architecture for the ordering.
    (I need to do a more detailed analysis of these)

    We should avoid adding features and extension points that we later remove.
    We don't need to do everything in one issue, but we should know in which direction we are moving.

    The initial OOP hook issue did use explicit priorities and that blew up which is why it was pulled out, this issue was specifically borne out of a BC way to allow ordering relatively.

    Can you remind me what is the "initial OOP hook issue" and how it failed? Maybe I missed it.
    (I assume we are not talking about my older issue here, which mostly suffered from being overly ambitious. I don't recall that anything "blew up".)

    This said, I can imagine how this could have failed, depending how it is done.

    Yes, hooks are different so the divergence is expected

    My point is the more we diverge and the less we reuse from symfony events, the more we have to ask what we gain from using the symfony system, or if it would have been better to do something custom.
    Maybe it is too late now.

    multiple implementations are new, going to be rare, and ordering them as a separate entity will be even rarer.

    The MR provides a way to order specific implementations relative to each other.
    If we assume all implementations of one module to have the same ordering position, then all we would need is to order relative to a module, not relative to an implementation.
    To me this MR is incomplete if we don't properly support multiple implementations per module. It could even reveal shortcomings in the approach.

  • Pipeline finished with Canceled
    about 1 month ago
    Total: 66s
    #437370
  • 🇺🇸United States nicxvan

    There were two very long discussions in slack relevant to this.
    The first regarding naming of extra types and other variables: https://drupal.slack.com/archives/C079NQPQUEN/p1740408731650039
    All variables have been renamed.
    There is still a point of contention about extraTypes on the order object. I have suggested relatedAlters. Since this is the second rename I will not do this unless there is more consensus. I will post more thoughts on this later.

    The second discussion was around higher level feedback most recently about ordering around extraTypes and ordering multiple implementations: https://drupal.slack.com/archives/C079NQPQUEN/p1740590792125979

    Before I dive into that further I want to highlight the purpose of this issue and why it is so large.
    The purpose of this was to provide the ability to order hooks and deprecate hook_module_implements_alter. In order to deprecate HMIA we need to replace all of it's functionality:

    • set a hook first
    • set a hook last
    • order a hook before another module
    • order a hook after another module
    • remove another module's hook
    • reorder another module's hook
    • reorder a hook relative to alter hooks with extra types

    That is a huge ask, especially when there is the constraint of handling this in a BC compatible way so current modules that have not converted yet can still order hooks relatively. This issue has been able to achieve that.

    I want to also highlight that an inordinate amount of attention has been given to ordering relative to extraTypes (formerly orderGroups). We have to support this to deprecate HMIA because core does do this, and inevitably some module somewhere does to. However, I cannot stress enough how rare this should be. However, as complex as this is, the situation is significantly better because now it is part of a discreet object and parameter so if we need to change it in the future, we can deprecate. As part of HMIA it's all or nothing deprecation so for this stage we must provide that option. I will likely update the IS and CR later highlighting how rare this should be, and if you find yourself using extraTypes, you should take a step back and consider deeply if you really need the approach you are taking. It did take the most time since it's the most complex bit, and it has faced the most scrutiny, but we know it works with ckeditor5 and the tests, and it really should be the least used bit of this. This is also why I will hold off on renaming again unless there is wider consensus, it's a lot of thought and attention something that will almost never be used, and it is named after how it is used in ModuleHandler::alter.

    The other point of contention in this thread and slack is that if a module has multiple implementations they want to order that order is lost. However, this should be a follow up, this is a current gap in implementation, and HMIA cannot order against multiple implementations anyway. The scope of this issue is so large we should not take on pre-existing bugs that are for a feature that is brand new and will likely not be used. Again, it is an edge case of an edge case and we have a follow up already for it.

    Now to address some specific points in the previous comment:

    We should avoid adding features and extension points that we later remove.
    We don't need to do everything in one issue, but we should know in which direction we are moving.

    In general I agree, however I don't think that applies here for two reasons. 1. I strongly doubt we will remove this unless altering with extra types goes away and that's not happening anytime soon. and 2. this situation is much, much better because if we do change it we can deprecate just that one piece of ordering.

    Can you remind me what is the "initial OOP hook issue" and how it failed? Maybe I missed it.

    The one that is in the IS is where we discuss how it blew up 🐛 Ordering of alter "extra hooks" is a gigantic mess Active
    A couple of final comments on the approach you mentioned using priority in the slack message and why it blows up. If you use priority then you have to exclude that hook implementation from hook ordering in HMIA which means there is a line in the sand you cannot legacy order hooks using priority so you force other modules to update if they need to order relative to you. With our relative ordering approach it is far less disruptive.

    My point is the more we diverge and the less we reuse from symfony events, the more we have to ask what we gain from using the symfony system, or if it would have been better to do something custom.

    I didn't want to add too much here since there is already information overload, but I suspect we'll likely go in this direction 🐛 HookCollectorPass registers event listeners but they do not follow the required calling convention Active But we can't start that until this gets in.

    To me this MR is incomplete if we don't properly support multiple implementations per module. It could even reveal shortcomings in the approach.

    This is out of scope for an issue already this big. This already cannot happen, it remains to be a problem, but once this is in it will be a smaller issue to fix.

    I really think this issue is close to being ready, it has been looked at by several people in depth. We converted all core hmia implementations so we know they are covered, and we have significant test coverage. I am starting to worry about attrition for this issue, let's not perfect be the enemy of an issue that replaces one of the most complex and problematic hooks.

  • Merge request !6Draft: WIP: Changes by donquixote, part1 → (Open) created by donquixote
  • Pipeline finished with Failed
    about 1 month ago
    Total: 158s
    #438095
  • 🇩🇪Germany donquixote

    donquixote changed the visibility of the branch 3485896-donquixote-suggested-changes to hidden.

  • Pipeline finished with Failed
    about 1 month ago
    Total: 1057s
    #438154
  • 🇺🇸United States nicxvan

    @donquixote did a deep dive and has a lot of suggestions. Some are substantial that I need to review and give them due consideration. First review, I think one or two might be a gap that needs to be addressed before this is merged. I think most of them are just an alternate way of doing things, not sure if they are better, worse, or equivalent without more thought.

    Many are still related to typing and naming, before going through that I'd like a consensus on a couple of questions:

    1. A lot of the typing is on parameters that have not changed, I think that would be out of scope for this issue, should we avoid that?
    2. Same for constructors being moved to multiline, I prefer this myself and I know coding standards allow this, but I'm not sure it is in scope here.
    3. Suggesting renaming ComplexOrder with RelativeOrder or RelativeOrderBase, I'd lean towards RelativeOrderBase.
    4. Rename value for ordering to direction or position, I'd lean toward position.

    If I get consensus on 1-4 I'm happy to apply them.

  • Pipeline finished with Failed
    about 1 month ago
    Total: 622s
    #439047
  • 🇩🇪Germany donquixote

    donquixote changed the visibility of the branch 3485896-donquixote-suggested-changes-part1 to hidden.

  • 🇩🇪Germany donquixote

    donquixote changed the visibility of the branch 3485896-donquixote-suggested-changes-part2 to hidden.

  • 🇺🇸United States nicxvan
  • Pipeline finished with Canceled
    30 days ago
    Total: 128s
    #439344
  • Pipeline finished with Success
    30 days ago
    Total: 704s
    #439347
  • 🇩🇪Germany donquixote

    Another higher-level recap.

    Basic concepts

    (these are just observations to verify my understanding.)

    The new attributes and attribute properties allow to specify ordering information for hook implementations, or to entirely remove specific implementations.
    An implementation can be first, last, or before/after specific other implementations.
    For relative ordering and for removal, other implementations are addressed by hook name, class name and method name.
    (Consequence: When the class or method for a hook implementation get renamed, the ordering attributes that target the old name no longer work.)
    Alternatively, a relative ordering can address module names, which will target all implementations by that module.

    Ordering rules from attributes are applied _after_ the implementations have been reordered with hook_module_implements_alter().

    Ordering rules from attributes are applied one by one, in the order in which they were discovered, but with the order on regular Hook attributes being applied first, then the order from ReOrderHook attributes.
    An ordering rule applied later will overwrite / win over a conflicting order by a previous rule.

    A relative ordering rule can have "extra hooks" specified, which means that this ordering rule should be applied for ->alter() calls with multiple hooks. (more on that below)

    Conflict resolution

    The resolution of conflicts is quite simple:
    Any "later" ordering rule will overwrite any "earlier" ordering rule.

    If we want to control this further in the future, we should provide ways to prioritize the ordering rules.
    (This could bring us to a similar situation as explicit priorities, but we can discuss that in the future)

    Overall this basic concept is fine!
    It is different from what I proposed in my older MR, but it is a very valid way to order hook implementations.

    Extra hooks logic

    Tbh I find it hard to follow the "extra hooks" logic.
    I mean I think I understand the intention, but then I have trouble to follow what exactly is happening in the code.

    I also suspect that developers who want to apply order attributes may not fully understand the consequences of adding or not adding hooks in the order extra types.

    At the same time I had the suspicion that it might not behave as we expect in many scenarios.
    But it is hard to point out possible flaws when I might just not correctly understand it.
    This is why my earlier comments were primarily focused on docs, naming and possible refactoring.

    Naming of methods and variables

    (these are generic comments, for specific comments see the review)

    My comments about naming may appear nitpicky and pedantic.

    In general there is no "perfect" naming, but there are some mistakes we can avoid.

    Overall, I like it if the same thing has the same name throughout a subsystem, and more so within the same method, and different things to have different names.
    E.g. when I see something like `foreach ($priorities as $weight)` or `foreach ($trees as $forest)`, I get confused.

    Of course it is much easier to point out flaws than to propose better names.

    Sometimes, the difficulty to find a good name can point to a design issue, where the variable or method should not really exist in that way.

    Test coverage

    Actually I have not looked at all into the tests.
    But I suspect we need more, given that some possible problems I found in the code did not cause test failures.

    If we could isolate the calculation some more, it might become possible to cover a lot of stuff with unit tests, which are much faster than

    Follow-up issues?

    To me, follow-up issues should be for additional functionality, or to fix things that we left unchanged.
    I don't like to plan follow-ups to change things from a current MR.

    We still have some time until the next release, so we _could_ change or rewrite parts of this ordering in follow-ups, but I would prefer if we can avoid it.

    Alternative: Order at call time (cached)

    I can imagine one alternative that (I think) would allow to drop the "extra hooks" or "extra types" in the order object added to an attribute.

    We could simply apply the ordering rules from attributes at runtime:

    At container build time:
    - Calculate the order for regular single hook.
    - Store ordering rules per hook.

    At runtime for a multi-hook alter call:
    - Load and merge the implementations for each hook.
    - Group by module, apply module order, call HMIA with the primary hook (as was done in older versions of Drupal).
    - Undo the grouping by module, so we have a list of individual implementations, by identifier.
    - Apply the ordering rules for each of the hooks from the ->alter() call.
    - Cache the result.

    A tricky edge case here would be if the same class + method implements multiple hooks that are invoked together.
    So, if something calls `->alter(['A', 'B'], ..)`, and we have `#[Hook('A_alter'), Hook('B_alter')]` on one method, possibly with different ordering information, then the targeting with class + method would become ambiguous.
    To avoid this ambiguity there could be an additional parameter in the Order object, to determine whether we order relative to A_alter or B_alter.
    But, I think it would be much better to just disallow that, or to order relative to both.

    This alternative will lead to a situation where we order "combined" hooks at call time, but regular hooks at container build time.
    We _could_ instead just do all the ordering at call time, which would probably reduce code duplication.

    Note that in the current MR we already do ordering at call time for a multi-hook, if we do not find a pre-ordered combined hook.

  • 🇺🇸United States nicxvan

    Basic concepts

    This section is generally correct.

    (Consequence: When the class or method for a hook implementation get renamed, the ordering attributes that target the old name no longer work.)

    Yes, but when doing relative ordering you have to know about the implementation you are ordering relative to.

    Alternatively, a relative ordering can address module names, which will target all implementations by that module.

    For the hook providing the ordering directive.

    Conflict resolution

    Yes, that is how it works here.

    Extra hooks logic Tbh I find it hard to follow the "extra hooks" logic.

    Yes, it is hard to follow.

    I also suspect that developers who want to apply order attributes may not fully understand the consequences of adding or not adding hooks in the order extra types.

    I can almost guarantee that almost no one will ever need this, it's needed in core so we had to provide it, this solves core's usage, proven by tests. That should be the bar for acceptance for this bit of the feature.

    At the same time I had the suspicion that it might not behave as we expect in many scenarios.
    But it is hard to point out possible flaws when I might just not correctly understand it.

    I can also all but guarantee that anybody trying to use this will probably not need it and it will be wrong.
    We can't build this feature for what might be or how people might use it, it's far, far too much of an edge case. If someone opens a bug against it I will happily chase it down and try to find a solution, but we can't make guesses in this area, if it were not for HMIA supporting something like it, we would not have built it in the first place. As it stands we do need it unfortunately.
    The bar for this really should be does it work in core, can we address contrib if they need it? The truth is if there is a contrib module doing some ordering on extra types this doesn't work for they can keep their procedural implementation and we can work on solving their use case before Drupal 12.

    Naming of methods and variables

    Not going to address anything here beyond what I've said before, if other's agree we should rename anything I am happy to change to a consensus. I'm not going to rename anything further before that. See IS for current options.

    Test coverage

    There is extensive test coverage, further, we have converted all of core which has some of the most complex HMIA implementations. I'm confident we have enough tests. This is a large enough issue that there are gaps, but we cannot find them without contrib using it which requires merging this.

    Follow-up issues?

    To be fair my core contribution experience is more limited than yours, but from what I've seen everything I've called out as a follow up falls within standard usage. I mean look at the size of this issue, it's already at least 5 times larger than average and far far more complex. There are some edge cases we know about, the sooner we get this in the sooner we can track those down. Needing to rebase a 2000 line MR to try to solve theoretical edge cases is not the way to progress.

    Alternative: Order at call time (cached)

    Runtime ordering is where almost all of this complexity comes from, so I would be strongly against adding more of that in the future.

  • 🇩🇪Germany donquixote

    I can also all but guarantee that anybody trying to use this will probably not need it and it will be wrong.
    We can't build this feature for what might be or how people might use it, it's far, far too much of an edge case. If someone opens a bug against it I will happily chase it down and try to find a solution, but we can't make guesses in this area, if it were not for HMIA supporting something like it, we would not have built it in the first place. As it stands we do need it unfortunately.
    The bar for this really should be does it work in core, can we address contrib if they need it? The truth is if there is a contrib module doing some ordering on extra types this doesn't work for they can keep their procedural implementation and we can work on solving their use case before Drupal 12.

    We are designing a public API here, and anything we create will be very hard to remove later.
    We can fix things before a release, but in general we should only commit what we are ready to release.

    From what I remember, ckeditor5 would work just fine with "Order::Last" instead of "Order::After".
    The main reason why we would try to support this case in core instead of replacing it with "Order::After" is that there are likely more such cases in contrib, and we keep this one as a canary.
    (Or maybe we are concerned about BC impact of moving this hook priority? I don't think so.)

    What we don't want is add a bunch of complex code and complex extension points, and then if we are worried it might not work we say nobody will use it anyway. If we only care about one specific case in core, we better to some kind of one-off hack.

    This makes me wonder: Order::First and Order::Last do not allow to specify additional hook names.
    How will first and last work if alter is called with multiple hooks?

    To be fair my core contribution experience is more limited than yours

    To be clear I don't have any authority and have been a bit out of the loop in the last years.
    I mostly express what makes sense to me.
    I welcome any opinions from others.

  • 🇺🇸United States nicxvan

    We are designing a public API here, and anything we create will be very hard to remove later.
    We can fix things before a release, but in general we should only commit what we are ready to release.

    I am 100% confident in how we designed this, but as mention in the MR even though it is an API it is marked internal so we can tweak it. The extreme complexity is limited to only one parameter on a parameter, that gives us the ability to deprecate if we have to change it wholesale, we do not have that ability to do targeted updates now which is why it is so massive.

    From what I remember, ckeditor5 would work just fine with "Order::Last" instead of "Order::After".
    The main reason why we would try to support this case in core instead of replacing it with "Order::After" is that there are likely more such cases in contrib, and we keep this one as a canary.

    I literally tried that multiple ways, it does not work and is the whole reason we created extra_types.

    This makes me wonder: Order::First and Order::Last do not allow to specify additional hook names.
    How will first and last work if alter is called with multiple hooks?

    The same way it does now if you only move your implementation first without doing any array searching and slicing like ckeditor5 does.

  • 🇩🇪Germany donquixote

    $snake_case vs $camelCase

    The 11.x version of HookCollectorPass and other classes has a mix of snake and camel case in local variables and parameters, with snake case being the majority.
    The MR introduces new variables that are mostly lower camel case, although in HookPriority I see new snake case variables.

    I have a personal preference which has shifted over time, currently leaning towards snake case.
    But this should not be relevant here.
    The coding standards regarding this have been relaxed. I even saw discussions to soften the rule of not mixing within a file. But this does not mean it should be arbitrary in Drupal core.

    But I also agree with @nicxvan to avoid renaming until there is broader consensus. Don't rename things just for me.

  • 🇺🇸United States nicxvan
  • Pipeline finished with Failed
    28 days ago
    Total: 133s
    #441399
  • Pipeline finished with Success
    28 days ago
    Total: 2639s
    #441428
  • Pipeline finished with Failed
    27 days ago
    #442355
  • Pipeline finished with Success
    27 days ago
    #442356
  • 🇩🇪Germany donquixote

    donquixote changed the visibility of the branch 3485896-donquixote-suggested-changes-part2 to active.

  • 🇩🇪Germany donquixote

    https://git.drupalcode.org/project/drupal/-/merge_requests/11344/diffs
    The current version of this (commit af51ab5bb1ef81e35658ffa2dd1514d7757669c2) shows how we can calculate everything first and _then_ write to the container, in HookCollectorPass.
    (It also contains some unrelated changes.)

    It passes existing tests.
    However, it is very possible that we just don't have enough coverage.
    In that case, we should improve the tests.

    To improve test coverage for different combinations, we can create a hook with a bunch of implementations with different ordering info, where each does "return __METHOD__;", or sets `$arg[] = __METHOD__` for an alter hook.
    Then, instead of asserting something in $GLOBALS, we can assert the return value which gives us a list of all the methods that were called, and their order.

    There are still more possible simplifications we can do.

  • 🇩🇪Germany donquixote

    Once HMIA is gone this edge case is gone.

    We can cross that bridge when we come to it, if we ever do.
    Realistically we won't need to. This only takes affect if they are form alters.

    To me these are not good answers that justify to "resolve" a review comment.
    We are designing a new system, and we want to support BC.
    All of this has to work reliably and predictably, otherwise it should not be added.
    Form alters are not an edge case. HMIA is not an edge case, it has to be 100% supported until we remove it in 12.x.
    Also, the time spent on an issue does not determine the mergeability.

    The next steps I see for me:
    - Add tests to cover more combinations.
    - Further cleanup and refactoring of the code in this issue.

  • 🇺🇸United States nicxvan

    Can you rebase against the main branch so the fork mr is accurate so I can see your changes.

    I reopened those two threads.

    However, in the one I responded:

    Once HMIA is gone this edge case is gone

    Maybe edge case is the wrong word, it's more that once this ordering is the only way there won't be a way to order against unknown hooks so we don't have to fall back to legacy ordering. I closed it because of the comment about legacy ordering.
    I would still consider that answered.

    The other one I'm not sure is an issue either, the last one should win, just like with hmia now.

    All of this has to work reliably and predictably, otherwise it should not be added.

    It does, and far more that hmia it is easier to understand how to implement your item ordering. It doesn't have to be perfect we can't achieve that in a world where we need to be bc with hmia.

    Form alters are not an edge case.

    I never said they were, but ordering across multiple layers of form alter very much is an edge case.

    If I could cut that out of this issue I would because 90% of all comments outside of typing and renaming are about extra types, and one hook uses extra types.

    If you can find a contrib module doing this type of ordering I will gladly convert it to test this further.
    Until we find other uses for it, this is all speculative. Modules can use legacy ordering and file an issue if we didn't meet their edge case, it's not the end of the world, we solved a fews issues since oop hooks came out that we missed.

    Feel free to add more tests in your branch we have many tests and we have tons of implicit tests with the hmia conversions we did.

    Feel free to experiment with refactoring too, but most of your refactoring I've seen are internal implementation details that would be far easier to get in and scope once this is in.

    Other than your comment about module handler add i have not seen an architectural question you've raised we had not already considered which is one of the reasons I'm so confident. I'm not worried about module handler add because it has far deeper architectural issues and it's on its way out and this works just as well as it does in 11.1

  • 🇺🇸United States nicxvan

    I took a look at your branch and just reviewed HCP directly.
    If I read it right you can remove Hook priority entirely right? I'd have to take some more time to look at it, but it looks interesting.

    I still think it's far more efficient to store legacyImplementationMap and remove the one or two that need to be removed rather than loop over all thousands of hooks just to build it after, but I wouldn't block the issue on that.

    Just to make sure I am reviewing everything, I don't think you made any substantial changes beyond what you changed in HCP and the really minor optimization in ModuleHandler:alter right?

  • 🇩🇪Germany donquixote

    Hi
    Just a quick clarification.

    If I read it right you can remove Hook priority entirely right? I'd have to take some more time to look at it, but it looks interesting.

    Yes, we can remove HookPriority class, I just did not get to it yet.

    I don't think you made any substantial changes beyond what you changed in HCP

    The main change here was to change the algorithm within HookCollectorPass.
    I wanted less interaction with the container, and only deal with priorities once everything is finished.
    Once this is complete, we can run the main part of the sorting logic in a clean unit test without too much mocking.

    I did try to keep the behavior of the existing MR.
    This was successful as far as existing tests, _but_ as mentioned, it could be that additional tests will reveal functional changes.

    In this version, we cannot remove the $order->extraHooks / ->extraTypes.

    In a future version (which I think we can still do in this issue), we can try sorting at call time, which would allow us to get rid of $order->extraTypes.
    (I say in this issue, because I much prefer not add something than to add and then remove.)

  • Pipeline finished with Failed
    26 days ago
    Total: 154s
    #442732
  • Pipeline finished with Failed
    26 days ago
    Total: 181s
    #442733
  • Pipeline finished with Failed
    26 days ago
    Total: 584s
    #442731
  • Pipeline finished with Failed
    26 days ago
    Total: 370s
    #442860
  • 🇩🇪Germany donquixote

    I refactored this based on a discussion with @GhostOfDrupalPast to use the container reflections that Symfony caches, this should be much faster.

    What do we actually gain from this?
    Worst case is that a ReflectionClass is created again, after it was already created in symfony.
    I suspect the main cost of "new ReflectionClass" is just to load and parse the class file. Once the class is known to php, it will be very fast.
    See also https://gist.github.com/mindplay-dk/3359812

    symfony ContainerBuilder::getReflectionClass() actually does some crazy magic that we don't need, so it might actually be slower than just creating the reflection class again.
    It definitely increases complexity and mysteriousness.

    I would like to remove it to reduce the places that get passed a container.

  • Pipeline finished with Success
    26 days ago
    Total: 403s
    #442907
  • 🇺🇸United States nicxvan

    I don't have a super strong opinion on this bit, it was more that we already have the reflection class so why not use it.

    I know that @godotislate is experimenting with updating this elsewhere too.

  • It was suggested as a possible performance enhancement on 📌 Investigate possibilities to parse attributes without reflection Active #16. I have not done any metrics on it.

  • 🇩🇪Germany donquixote

    Let's keep this as follow-up, with actual measurements.
    As said, I suspect the main cost is loading the file. After that, I suspect we can call "new ReflectionClass" and "$reflector->getAttributes()" repeatedly without much extra cost.

    Atm I think we should drop everything that smells like additional complexity.
    In this case, dropping that symfony reflection cache allows to do most of the calculations without a container.

  • 🇺🇸United States nicxvan

    No objection from me to remove that on your branch, let me know when it's ready for a deeper look.

  • 🇩🇪Germany donquixote

    The third MR extracts a new class HookCollector from HookCollectorPass which now does most of the logic and is mostly independent from the container.
    https://git.drupalcode.org/project/drupal/-/merge_requests/11415/diffs

    If you want a cleaner diff, look at v2 instead:
    https://git.drupalcode.org/project/drupal/-/merge_requests/11344/diffs

    I would still like to do more changes:
    - I think we should use $snake_case for all new local variables and non-promoted parameters. But for now I want to reduce disruption. We can do this once we agree.
    - I would like to rename more variables, but again want to reduce disruption.
    - More tests.
    - Order and alter hook implementations at call time, especially for multi hook alter calls.

  • 🇩🇪Germany donquixote

    Some thoughts about relative ordering

    In hook_module_implements_alter(), relative ordering always means to order one module relative to another module.
    With the new code, we can order specific implementations relative to other specific implementations, OR to all implementations of a module.

    As mentioned earlier, ordering relative to a specific implementation easily breaks when that implementation gets renamed.
    Ordering relative to all implementations of a module might have unintended effects if one of these as a "first" or "last" order applied.

    In my older proposal, there was the possibility to order relative to a module's default odering position.
    So, even if the implementations of that module get moved around with first, last, before or after ordering, we still only order relative to the original position.

    So, let's say:
    - We have modules A, B, C, D, in that order.
    - We have hook implementations A1, B1, B2, B3, C1, D1.
    - We have ordering info "B1 first" and "D1 before B*".

    With the current proposal we get D1, B1, A1, B2, B3, C1.
    If "before B*" is changed to "before the default position of B", we get B1, A1, D1, B2, B3, C1.
    We can even make this work if none of the B* implementations remain in the default position.

    Another way to approach this is to define an order of ordering operations.
    E.g. we could apply all relative ordering (before/after) before all absolute ordering (first/last). But this only gets us so far.

  • 🇺🇸United States nicxvan

    But for now I want to reduce disruption.

    Too late for that lol.
    Honestly I don't really care snake case vs camel.
    Things seem to be trending toward camel so I generally lean towards camel. In an issue of this size where we are talking about a whole new approach this feels like noise and can be addressed when we settle on which approach.

    More tests

    Sounds good.

    Order and alter hook implementations at call time, especially for multi hook alter calls.

    I still feel like this would be a mistake. If you want predictability we want as much ordering during build as possible. This will also be more performant.

    As mentioned earlier, ordering relative to a specific implementation easily breaks when that implementation gets renamed.
    Ordering relative to all implementations of a module might have unintended effects if one of these as a "first" or "last" order applied.

    Yes, this is a limitation, but in the interest of scope we should try to minimize changes.

    In my older proposal, there was the possibility to order relative to a module's default odering position.

    Feels out of scope and something that could be added later.
    Out of curiosity why would you want this?

    Another way to approach this is to define an order of ordering operations.

    Same scope concerns and as you pointed out it's just punts down the road.

    First impression on the further changes.
    I'm on the fence about splitting hook collector out, but honestly pass is getting huge. I wonder if we have more files, one that does collection, one with parsing and ordering and we leave the registration in hcp?

  • Merge request !11429Draft: Additional tests → (Open) created by donquixote
  • 🇩🇪Germany donquixote

    I created new branches with tests.
    For now I created an MR only for one of them, for tests that include the 3485896-attempt-simplification and do not include my own changes.

    In general I think this is direction that these tests should take:
    We call a hook or an alter hook, we get a list of called implementations as from __FUNCTION__ or __METHOD__, and we compare that to the expected order.

    The test has some additional magic where we want to check that `->alter(['a', 'b', 'unknown'], ...)` produces the same call list as `->alter(['a', 'b'])`.
    Currently this is more like a proof of concept, we definitely don't want to merge as-is.

    The tests confirm my suspicion:
    An order operation with "extra" types has side effects beyond that one implementation being reordered.
    We could probably craft something even more clear to confirm this.

    The tests would become a lot more interesting if we fix the grouping by module problem in ModuleHandler.
    I know this is currently planned as follow-up, but we might actually want to do this first.

  • 🇩🇪Germany donquixote

    Should we take this out of needs review for a bit?

    Done.

    I was on the fence about splitting hook collector out, but honestly pass is getting huge. I wonder if we have more files, one that does collection, one with parsing and ordering and we leave the registration in hcp?

    I am undecided about how exactly to split the classes.
    For now I just wanted to see that we _can_ split this up, so that ModuleHandler no longer has to deal with a compiler pass directly.
    A separate HookCollector feels like a more generic class to use in different places.
    Currently we still have lots of stuff in that class. So indeed we might want to move some of it back into HookCollectorPass, anything that deals with the container.

    But, any decision we take now about the final shape of HookCollector might become obsolete with further changes.

  • 🇩🇪Germany donquixote

    New branch to prove the side effect:
    https://git.drupalcode.org/issue/drupal-3485896/-/compare/11.x...3485896...
    This commit:
    https://git.drupalcode.org/issue/drupal-3485896/-/commit/404bc461f0d3fa5...

    So we add a #[ReOrderHook] that targets a non-existing implementation in a non-existing module.
    This should have no effect on the order of the other implementations.
    But the extraTypes messes things up.
    We see the first fail, but I suspect there is more, including possibly implementations disappearing.

  • 🇺🇸United States nicxvan

    So we add a #[ReOrderHook] that targets a non-existing implementation in a non-existing module.
    This should have no effect on the order of the other implementations.

    We can check that the implementation exists before adding to the order group.
    We had considered that so i want to look deeper.

    I suspect there is more, including possibly implementations disappearing

    What mechanism are you proposing that does that?

  • 🇩🇪Germany donquixote

    We can check that the implementation exists before adding to the order group.

    This is missing the point.
    I just pushed another version of the side effect test, which is using existing implementations.

    What mechanism are you proposing that does that?

    The "extra types" mechanism is just fundamentally flawed.
    For an implementation to ask "I want to be ordered together with X" inevitably has an effect on other implementations and other groupings that can exist at call time. This can be mitigated to some extent, but the basic flaw is always present.

    The correct thing is to order at call time.
    Otherwise we would have to predict every possible combination of hooks that can be passed to alter().

  • 🇩🇪Germany donquixote

    To explain it differently:
    The sorting done in one specific group of hooks can only be used when ->alter() is called with those exact hook names.
    But this is not what the MR is doing.

    Instead, when you call ->alter() with hooks/types ['a', 'b', 'c'], you might get the pre-ordered list from ['b', 'c', 'd'] or from ['b', 'x'], which gives you not only the wrong order, but can also give you the wrong implementations. Even if we correct the list of implementations at call time, by adding and removing implementations, the order would still be wrong.

    There is no way to fix this within the current proposal, because naturally there won't always be a pre-ordered list for the current combination.

  • 🇺🇸United States nicxvan

    There is no way to fix this within the current proposal, because naturally there won't always be a pre-ordered list for the current combination

    I do not think that's true, but let me work it out, thanks for the tests!

  • Pipeline finished with Failed
    23 days ago
    Total: 177s
    #445224
  • Pipeline finished with Failed
    23 days ago
    Total: 168s
    #445242
  • 🇩🇪Germany donquixote

    I created a separate MR with call time ordering.
    Still a bit rough, but this is what I have for now.

  • Merge request !11438Resolve #3485896 "Nicxvan order at call time" → (Open) created by nicxvan
  • 🇺🇸United States nicxvan

    Created one off yours to fix the first stage just so we can see the full test suite.

    I'll take a look at your tests on my branch in a bit.

  • Pipeline finished with Failed
    23 days ago
    Total: 145s
    #445275
  • Pipeline finished with Failed
    23 days ago
    Total: 140s
    #445287
  • Pipeline finished with Failed
    22 days ago
    Total: 158s
    #446137
  • Pipeline finished with Failed
    22 days ago
    Total: 156s
    #446154
  • Pipeline finished with Failed
    22 days ago
    Total: 155s
    #446156
  • Pipeline finished with Failed
    22 days ago
    #446158
  • Pipeline finished with Failed
    22 days ago
    Total: 136s
    #446162
  • Pipeline finished with Failed
    22 days ago
    Total: 554s
    #446185
  • 🇩🇪Germany donquixote

    I found that objects in a container service parameter definition are no good, it prevents dumping of the container. So I changed it to serialize them explicitly.

    Now we have HookOrderTest failing in the pipeline but not in my local.

    The reason seems to be that RecursiveDirectoryIterator / DirectoryIterator does not produce a predictable order.
    https://stackoverflow.com/questions/29102983/order-in-filesystemiterator
    And the way we look for hooks, we mix procedural and oop in the same scan operation.
    This means it is not clear whether OOP hooks are first or procedural hooks are first, if from the same module.

  • Pipeline finished with Failed
    21 days ago
    Total: 466s
    #446493
  • 🇺🇸United States nicxvan

    nicxvan changed the visibility of the branch 3485896-nicxvan-order-at-call-time to hidden.

  • 🇺🇸United States nicxvan

    nicxvan changed the visibility of the branch 3485896-donquixote-suggested-changes-part3 to hidden.

  • 🇺🇸United States nicxvan

    nicxvan changed the visibility of the branch 3485896-donquixote-tests-nicxvan-FAIL-side-effect to hidden.

  • 🇺🇸United States nicxvan

    nicxvan changed the visibility of the branch 3485896-donquixote-suggested-changes-part2 to hidden.

  • 🇺🇸United States nicxvan

    nicxvan changed the visibility of the branch 3485896-donquixote-tests-donquixote-2 to hidden.

  • 🇺🇸United States nicxvan

    nicxvan changed the visibility of the branch 3485896-donquixote-tests-baseline to hidden.

  • 🇺🇸United States nicxvan

    nicxvan changed the visibility of the branch 3485896-donquixote-tests-nicxvan to hidden.

  • 🇺🇸United States nicxvan

    RecursiveDirectoryIterator / DirectoryIterator does not produce a predictable order.
    https://stackoverflow.com/questions/29102983/order-in-filesystemiterator
    And the way we look for hooks, we mix procedural and oop in the same scan operation.
    This means it is not clear whether OOP hooks are first or procedural hooks are first, if from the same module.

    Why would this matter?
    We key them by class hook and method.
    Collection order should not have an effect.
    Your tests look like they are failing due to deprecations.

    I hid most of the branches.

  • 🇩🇪Germany donquixote

    #[LegacyModuleImplementsAlter] problem

    We found one problem with #[LegacyModuleImplementsAlter], or the idea behind it.

    The idea was that a contrib module can support Drupal 11.1.x and Drupal 11.2.x+ by:

    • Creating a MYMODULE_module_implements_alter() with #[LegacyModuleImplementsAlter] so that it is only called in Drupal < 11.2.0.
    • Adding OOP hook implementations with e.g. #[Hook('my_hook', order: Order::First)], where the "order" part would only be picked up in Drupal 11.2.0 and up.

    Problem:
    In 11.1.x, the hook attributes will be discovered, but then the `order: Order::First` part will blow up because:
    - The enum for Order::First does not exist.
    - The named parameter `order: ` does not exist.

  • 🇩🇪Germany donquixote

    Collection order should not have an effect.

    It matters because if a module has multiple implementations, their order can be different on different system.
    We key by class and method, but we don't sort.
    Also, if we would sort by class, we would get a weird position for procedural based on the alphabetic order of ProceduralCall class.

    Instead, we should decide whether procedural implementations should be always before or always after oop implementations from the same module (if no other order specified).

  • 🇩🇪Germany donquixote

    We found one problem with #[LegacyModuleImplementsAlter], or the idea behind it.

    The solution could be a polyfill in a separate package outside of Drupal core.

  • 🇺🇸United States nicxvan

    There is no issue with:
    #[LegacyModuleImplementsAlter]
    #[RemoveHook]
    #[ReOrderHook]

    They are just ignored.

    The issue may be with the new enum and ComplexOrder object in the new order parameter for hook, I think it does get loaded so if contrib converts to the new ordering then is loaded on Drupal 11.1 then those do not exist so there is a fatal error. (The fact that the parameter is not parser does not matter because those missiles will have kept hmia around for ordering.)

    I need to confirm this and look at a solution.
    I will also look at the extra type tests you wrote.

  • 🇩🇪Germany donquixote

    There is no issue with:
    #[LegacyModuleImplementsAlter]
    #[RemoveHook]
    #[ReOrderHook]

    Correct, technically these work as designed.
    The problem is not with the #[LegacyModuleImplementsAlter] attribute but the greater purpose behind it.
    Of course technically the problem is with the new order property in #[Hook].

    We want modules to be able to use the new order property of the #[Hook] attribute, while at the same time keeping HMIA around to support older Drupal versions.
    This will cause problems as described above in 11.1.x.

    What these modules could do is declare compatibility with 10.x and 11.2.x but not with 11.1.x.

  • 🇩🇪Germany donquixote

    We could actually create a separate issue where we add a part of HookOrderTest for HMIA as baseline, to be merged _before_ this one.
    This will make it more visible if or how the ordering changes.

  • 🇩🇪Germany donquixote

    Some people in chat were concerned about serialization of order operations, I think mostly for security.
    I am not strongly attached to serialize/unserialize, but I think the security should be ok if we use a hard-coded list of very simple classes for 'allowed_classes'.

    From https://www.php.net/manual/en/function.unserialize.php:

    Warning
    Do not pass untrusted user input to unserialize() regardless of the options value of allowed_classes. Unserialization can result in code being loaded and executed due to object instantiation and autoloading, and a malicious user may be able to exploit this. Use a safe, standard data interchange format such as JSON (via json_decode() and json_encode()) if you need to pass serialized data to the user.

    If you need to unserialize externally-stored serialized data, consider using hash_hmac() for data validation. Make sure data is not modified by anyone but you.

    In this case the data is not "untrusted".
    But, even if it was, what exactly can go wrong if the classes we allow are as simple as here?
    I would like to know.
    (and don't tell me "if the class has a wakeup method...". the specific classes we are looking at don't have it.)

  • 🇺🇸United States nicxvan

    We raised three specific concerns about that piece:
    Speed, yes this is only for alters, but form alters can be called many times on a page.
    Size/memory, in the beginning there are not many, but this api is much easier than HMIA so adoption will likely go up.
    Security, you're hand waving this, but there are escalation attacks related to these functions and classes. why not avoid this concern entirely?

    Looking at how you're using it is is so you can just call the ->apply method right?

  • 🇺🇸United States nicxvan
  • Pipeline finished with Canceled
    19 days ago
    Total: 257s
    #448758
  • Merge request !8Draft: bad changes → (Open) created by donquixote
  • Merge request !9Draft: TBD changes → (Open) created by donquixote
  • Pipeline finished with Failed
    19 days ago
    Total: 118s
    #448819
  • 🇺🇸United States nicxvan

    I think we will be consolidating on the new approach shortly. There is still some refinement to happen, but before I hide the attempt simplification branch and update the CRs and IS to remove mentions of extra_types I wanted to give some notice.

  • 🇺🇸United States nicxvan

    nicxvan changed the visibility of the branch 3485896-donquixote-order-at-call-time-nicxvan-changes-BAD to hidden.

  • Pipeline finished with Failed
    19 days ago
    Total: 1892s
    #448859
  • Pipeline finished with Failed
    17 days ago
    Total: 134s
    #449824
  • 🇩🇪Germany donquixote

    donquixote changed the visibility of the branch test-file-scan-order to hidden.

  • 🇩🇪Germany donquixote

    donquixote changed the visibility of the branch 3485896-donquixote-order-at-call-time-nicxvan-changes-TBD to hidden.

  • 🇩🇪Germany donquixote

    donquixote changed the visibility of the branch 3485896-test-file-scan-order to hidden.

  • Pipeline finished with Failed
    17 days ago
    Total: 552s
    #449835
  • 🇩🇪Germany donquixote

    donquixote changed the visibility of the branch 3485896-test-file-scan-order to hidden.

  • Pipeline finished with Failed
    17 days ago
    Total: 163s
    #449893
  • 🇺🇸United States nicxvan

    nicxvan changed the visibility of the branch 3485896-attempt-simplification to hidden.

  • 🇺🇸United States nicxvan
  • 🇺🇸United States nicxvan

    Ok I've officially updated the IS and CRs in order to get behind the new approach, we need to polish the approach.

  • Pipeline finished with Failed
    10 days ago
    Total: 251s
    #455468
  • Merge request !11579Draft: Order the order operations by specifity → (Open) created by donquixote
  • 🇩🇪Germany donquixote

    donquixote changed the visibility of the branch 3485896-order-operations-by-specifity to hidden.

  • Pipeline finished with Failed
    10 days ago
    Total: 288s
    #455486
  • Pipeline finished with Failed
    9 days ago
    Total: 200s
    #456619
  • Pipeline finished with Failed
    9 days ago
    Total: 275s
    #456620
  • Pipeline finished with Failed
    9 days ago
    Total: 244s
    #456623
  • Pipeline finished with Failed
    9 days ago
    Total: 205s
    #456697
  • Pipeline finished with Failed
    9 days ago
    Total: 215s
    #456710
  • Pipeline finished with Failed
    9 days ago
    Total: 210s
    #456733
  • Pipeline finished with Failed
    8 days ago
    Total: 262s
    #457181
  • Pipeline finished with Failed
    8 days ago
    Total: 188s
    #457615
  • Pipeline finished with Failed
    8 days ago
    Total: 162s
    #457617
  • Pipeline finished with Success
    5 days ago
    Total: 1261s
    #459979
  • Pipeline finished with Success
    5 days ago
    Total: 741s
    #459987
  • Pipeline finished with Canceled
    5 days ago
    Total: 402s
    #459993
  • 🇺🇸United States nicxvan

    I think this might be ready for review again!

    The actual proposal did not change. The underlying implementation changed.

    If you reviewed this before the changes are in HookCollectorPass, ModuleHandler and the other classes in core/lib/Drupal/Core/Hook
    There are many more tests too.

  • 🇺🇸United States nicxvan
  • Merge request !11676Draft: Introduce ImplementationList class → (Open) created by donquixote
  • Pipeline finished with Failed
    4 days ago
    Total: 118s
    #460402
  • 🇩🇪Germany donquixote

    donquixote changed the visibility of the branch 3485896-ImplementationList-object to hidden.

  • Pipeline finished with Failed
    4 days ago
    Total: 110s
    #460416
  • 🇩🇪Germany donquixote

    I am proposing this change to avoid relying on two synced arrays in ModuleHandler:
    https://git.drupalcode.org/project/drupal/-/merge_requests/11676/diffs?c...

Production build 0.71.5 2024