Problem/Motivation

The hook system has some awkward details that make refactoring and improving it harder.
Especially, these force some complex tricks in #3366083: [META] Hooks via attributes on service methods (hux style) #3368812: Hux-style hooks, proof of concept
Some of this can be cleaned up without major BC breaks.

Single module ModuleHandler->invoke()

One should think that this is equivalent to just calling the one implementations from ->invokeAll(), that matches the given module.
With the huxified system, it would mean _any_ implementation that is associated with that module.

However, this is not fully the case:
If the function $module . '_' . $hook exists at the time when ModuleHandler->invoke() is called, then that function is always called, even if it is not part of the (cached) list for ->invokeAll(). The latter can be the case if:

  • The implementation was removed through hook_module_implements_alter(), OR
  • The file that contains the function (e.g. *.install) was not included at the time of discovery, OR
  • The module that owns this implementation is not currently enabled - this is the case for hook_requirements().

The hook_module_implements_alter() has limited effects on single ->invoke():

  • Reordering of implementations has no effect, because there is only one function.
  • Removal of implementations has no effect, because the function is still called.
  • Anything that causes a file with the $module . '_' . $hook function to be included, if it would otherwise not be, does have an effect. This generally means that hook_module_implements_alter() has to add a module with include group, e.g. $implementations['mymodule'] = 'mygroup'. This could even be another module that implements the hook on behalf of the module being called.

In fact, most of the hooks that are called with ->invoke() are never called with ->invokeAll().
This means we could consider to handle them completely separately.

Removal of missing functions from the cached list

If a list of implementations is loaded from cache, a function_exists() check is performed on every cached implementation.
If the function does not exist, it is removed from the list, and the updated list is later written to the cache.

The only use case I can think of is during development, if the developer temporarily checks out another git branch, or temporarily removes a function.

The practice of writing the updated list to the cache is questionable.
If you switch back to the main git branch, the function that was accidentally added is now gone from the cache.

Removing this mechanism would make things much easier in #3366083: [META] Hooks via attributes on service methods (hux style).

Removal or adding of modules

Currently it is possible to add or remove modules with ModuleHandler->addModule(), ->addProfile() and ->setModuleList().
This is a bit problematic with #3366083: [META] Hooks via attributes on service methods (hux style), because the services in the container where the ModuleHandler instance comes from is still based on the old module list. So we can add hook implementations for the newly added module, but the respective services won't be available at that time.

Note that this problem is only about adding ore removing modules.
Reordering modules via module_set_weight() is actually fine, because the order of hook implementations _can_ be dynamically changed.

Proposed resolution

Consider to simplify or remove some of these fragile mechanical details.

Possible changes:

  • For ->invoke(), we could completely disregard hook_module_implements_alter().
  • No longer remove missing functions from the cached list, unless a cache rebuild was explicitly requested.
    Instead, remove these functions in every request/process when the list is verified. This is not any better or worse for behavior, but it is easier to refactor.
  • Don't allow removing or adding modules in ModuleHandler. Allow reordering for module_set_weight(), but otherwise, used the fixed list from the container parameter.
    This sounds like a BC break, although I don't know of any known contrib modules that need this outside of tests.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Comments

donquixote created an issue. See original summary.

donquixote’s picture

donquixote commented
Related issues: +#2233261: Deprecate hook_hook_info groups, mark hook_hook_info() for deletion, +#2968232: Deprecate module_set_weight()
donquixote’s picture

donquixote commented
Issue summary: View changes
donquixote’s picture

donquixote commented

Challenges from "Hux in core" plans

Single ->invoke()

With Hux in core, it is possible for one module to add more than one implementation per hook.
Normally these are most useful for hooks invoked with ->invokeAll().

But it can also be useful for some hooks invoked with for a single module with ->invoke($module, $hook).
One example would be hook_schema(): It can be useful to have multiple services that add to the schema for one module.

With the weights and before/after keys, it can be possible to have some methods before and some after the main procedural function.

Method with #[Hook('myhook', module: 'mymodule', weight: -5)]
Method with #[Hook('myhook', module: 'mymodule', before: 'mymodule')]
function mymodule_myhook
Method with #[Hook('myhook', module: 'mymodule', after: 'mymodule')]
Method with #[Hook('myhook', module: 'mymodule', after: 'othermodule')]
Method with #[Hook('myhook', module: 'mymodule', weight: 5)]

Now the annoying part:
At the time the hook is discovered, the file that contains mymodule_myhook might not be included yet.
At the time ->invoke() is called, if we find that mymodule_myhook() exists, we need to insert that function in the correct position between the other callbacks.
With the current implementation in the MR, the cached lists don't contain the necessary metadata to determine that "would be" position. So instead, the hook needs to be discovered again.
This can be done, but is super annoying, and makes the system more complex and fragile.

donquixote’s picture

donquixote commented

This might all sound a bit overkill.
But I think there is and will be an expectation that ->invoke() behaves consistently with ->invokeAll(), only that the callbacks are filtered down to the given module.

donquixote’s picture

donquixote commented

Hooks I found that are used with ->invoke().

Core (outside of tests):

  • library_info_build
  • update_last_removed
  • install
  • uninstall
  • schema
  • mail
  • translate_access
  • node_update_index
  • field_views_data
  • requirements
  • help
  • quickedit_render_field

Contrib (from a big project, with no claim for completeness):

  • webform_libraries_info
  • data_policy_destination_alter
  • pathauto_is_alias_reserved
  • webform_element_input_masks

I think there is an expectation that any hook can also be invoked with ->invoke() for a single module.

catch’s picture

catch
he/him
Primary language English
commented

:addProfile() ::addModule() ::setModuleList() are as far as I know only used in the installer, we could try to factor their uses out of the installer, then mark them @deprecated with no replacement.

For ->invoke(), we could completely disregard hook_module_implements_alter().

This is reasonable. I also think it's something we can eventually remove (hook_help() is going, hook_schema() is on its way out in core if not contrib etc.), although it will take years, so we need something in the interim.

No longer remove missing functions from the cached list, unless a cache rebuild was explicitly requested.
Instead, remove these functions in every request/process when the list is verified.

This seems fine.

donquixote’s picture

donquixote commented

Thanks @catch!
Now how much of that can we do in a 10.x release?

donquixote’s picture

donquixote commented

No longer remove missing functions from the cached list, unless a cache rebuild was explicitly requested.
Instead, remove these functions in every request/process when the list is verified.

I wonder if we should log anything in that case.

catch’s picture

catch
he/him
Primary language English
commented

@donquixote theoretically all of it - of course it depends exactly what changes end up being necessary.

However deprecating methods and slight changes to what goes into alter hooks are both OK in minor releases.

nicxvan’s picture

nicxvan commented

I think some of this is resolved with #3442009: OOP hooks using attributes and event dispatcher

nicxvan’s picture

nicxvan commented
Status: Active » Closed (duplicate)

I think the only remaining issue is the ::add functionality which has a targeted followup from the previous mentioned issue:

#3481778: Deprecate functions using ModuleHandler::add()