Problem/Motivation

Critical due to ordering regression with extra types and mixed implementations.

Currently there are three different ways to order hooks roughly in lifecycle order:

  1. During build time HookCollectorPass calls hook_module_implements_alter is called for procedural hooks for backwards compatibility reasons. This is not enough. It needs to be called for all hooks because once Drupal core has all hooks ported (goal is D11.1) then code still using procedural code (likely most of contrib and custom) won't be able to order against core hooks.
  2. In #3442009: OOP hooks using attributes and event dispatcher a new mechanism was added to order hook implementations: service providers can change the priority
    of event listeners and a HookOrder helper class is provided to make the common operations (first/last/before/after) easy. The issue is it does not work with alter extra hooks.
  3. Run time ModuleHandler::reOrderModulesForAlter calls hook_module_implements_alter with the module names of OOP hook implementations. This only happens for extra hooks like form_FORM_ID_alter and form_FORM_BASE_ID_alter. This is why HookOrder is confusing: hook_module_implements_alter always works or will after this patch but HookOrder currently doesn't work for extra hooks and it's out of scope for this issue to fix that.

Steps to reproduce

Proposed resolution

  1. Call hook_module_implements_alter with every hook not just procedural ones.
  2. #3485658: Add an advisory AltersImplementations attribute Add an advisory ImplementsAlter attribute which can be used on < hook_module_implements_alter implementations to indicate which hook this alter is using.
  3. #3485659: Remove HookOrder Remove HookOrder temporarily

Remaining tasks

From the above list #1 is done. It should be noted that in addition to the tests here, this is tested here #3483599: Convert all procedural hook implementations to Hook classes with the ckeditor reordering that happens.

These can be followups
#2 AFAIK needs to be done and then these two tested meanwhile
#3.1 Removal is RTBC.
#3.2 Longer term resolution is here #3485896: Hook ordering across OOP, procedural and with extra types i.e replace hook_module_implements_alter

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3484754

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

ghost of drupal past created an issue. See original summary.

ghost of drupal past’s picture

Issue summary: View changes
ghost of drupal past’s picture

Issue summary: View changes
ghost of drupal past’s picture

Issue summary: View changes
nicxvan’s picture

Issue summary: View changes
nicxvan’s picture

Issue summary: View changes

ghost of drupal past’s picture

Issue summary: View changes
ghost of drupal past’s picture

Issue summary: View changes
ghost of drupal past’s picture

Title: Ordering of alter "extra hooks" is a gigantic mess » Ordering of hooks is a gigantic mess
nicxvan’s picture

Issue summary: View changes
nicxvan’s picture

Issue summary: View changes
nicxvan’s picture

Status: Active » Needs review
nicxvan’s picture

Issue summary: View changes
nicxvan’s picture

Issue summary: View changes
nicxvan’s picture

Title: Ordering of hooks is a gigantic mess » Ordering of hooks needs some attention
nicxvan’s picture

Issue summary: View changes
nicxvan’s picture

Issue summary: View changes
nicxvan’s picture

godotislate’s picture

Status: Needs review » Needs work

1 small nit and a question.

nicxvan’s picture

Status: Needs work » Needs review

Applied and replied, let me know what you think!

godotislate’s picture

Test failure is probably unrelated and needs a re-run. Fine to go to RTBC after test passes.

nicxvan’s picture

Status: Needs review » Reviewed & tested by the community

Tests passed rtbc per 22

nicxvan’s picture

I rebased and fixed the deprecation test. I'll keep an eye on the tests.
With #3482618: ModuleHandler->alterDeprecated does not properly handle deprecated OOP hooks the alter on these hooks is now respected so the order in the deprecation is inverted from the other issue.

Leaving RTBC since that's the only change and it was expected once the mentioned issue was merged. If it needs another review let me know.

nicxvan’s picture

Priority: Major » Critical
Issue summary: View changes
Issue tags: +beta target
nicxvan’s picture

Issue summary: View changes

  • catch committed a8aa5421 on 11.1.x
    Issue #3484754 by nicxvan, godotislate: Ordering of hooks needs some...

  • catch committed 052c0c26 on 11.x
    Issue #3484754 by nicxvan, godotislate: Ordering of hooks needs some...
catch’s picture

Version: 11.x-dev » 11.1.x-dev
Status: Reviewed & tested by the community » Fixed

I think this is good to get us to a point of minimal change for 11.1.x and then we can figure out hook ordering for all-OOP hooks will look like a bit further along.

Committed/pushed to 11.x and cherry-picked to 11.1.x, thanks!

fabianx’s picture

RTBC + 1 from me - post merge

This implementation is much cleaner.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.