Deprecate hook_hook_info()

Created on 4 April 2014, over 10 years ago
Updated 8 March 2023, almost 2 years ago

Objective

  1. #1892574: Remove hook_hook_info_alter() β†’ was removed a long time ago already.

  2. hook_hook_info() only exists for lazy-loading legacy hook implementation code.

  3. hook_hook_info() has architectural problems causing code to not be loaded (see related issues).

  4. ModuleHandler apparently maintains two cache items; one for 'module_implements', and another one just for 'hook_info'.

  5. Object-oriented code can be properly auto-loaded in D8.

Remaining implementations

  1. system_hook_info() (hook_token*())
  2. views_hook_info() (hook_views_*())

Proposed solution

  1. Remove hook_hook_info() + persistent caching from ModuleHandler.

  2. Convert remaining affected hook implementations...

    A) Either move them into .module files.

    B) Or find a simple object-oriented replacement pattern.

πŸ“Œ Task
Status

Active

Version

10.1 ✨

Component
ExtensionΒ  β†’

Last updated 9 days ago

No maintainer
Created by

πŸ‡©πŸ‡ͺGermany sun Karlsruhe

Live updates comments and jobs are added and updated live.
  • API clean-up

    Refactors an existing API or subsystem for consistency, performance, modularization, flexibility, third-party integration, etc. May imply an API change. Frequently used during the Code Slush phase of the release cycle.

  • VDC

    Related to the Views in Drupal Core initiative.

Sign in to follow issues

Merge Requests

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • πŸ‡«πŸ‡·France andypost

    Removed field_hook_info() from IS as it was moved to views_hook_info() in #2399931: Generic entity api field handler should live in views module not in field module β†’

    So only tokens and views using this hook but it needs to deprecate \Drupal\Core\Extension\ModuleHandlerInterface::getHookInfo() as well

    Filed follow-up to remove broken ref πŸ“Œ Remove stray reference to field_hook_info() Fixed

  • πŸ‡¬πŸ‡§United Kingdom catch

    If we replace hooks completely, fine. But we are not there yet I think.
    Until then, something like hook_hook_info() is useful for code organization, similar to autoloading for classes (not autoloading itself, but the PSR-4 file organization that is made necessary by autoloading).

    Just to answer #19 - a lot of modules and some of core too already puts the logic of their hook implementations into classes, so that the hook implementation in .module is just three lines or so. This pattern is available for every hook, not just the ones that have hook_hook_info() implemented for them.

  • πŸ‡¨πŸ‡­Switzerland berdir Switzerland

    Definitely no reason to keep this now that πŸ“Œ OOP hooks using event dispatcher Needs review landed.

    But doing so is tricky and needs two layers. We can't just remove hook_hook_info() implementations, because that would immediately break hooks that live in those files. First we need to deprecate hook implementations that live in those files. I don't think we need to do that one-one and introduce a deprecated flag on the hook info, we can do it for everything. But to avoid moving the token hook implementations twice, we probably want to convert those in core to services first.

  • πŸ‡¬πŸ‡§United Kingdom catch

    I wonder if we want to roll this into πŸ“Œ [Plan] Determine how to deprecate procedural hooks. Active .

    First deprecate procedural hooks that don't have the @LegacyHook attribute.

    This could still leave @LegacyHook hook implementation in hook_hook_info() files.

    But then when we're ready to drop Drupal 10 support, we can deprecate @LegacyHook too. And at the same time, we can deprecate hook_hook_info().

  • πŸ‡¨πŸ‡¦Canada Charlie ChX Negyesi 🍁Canada

    Actually πŸ“Œ OOP hooks using event dispatcher Needs review removed this functionality. That turned out to be a mistake.

    I can already see the future thanks to test issue where nicxvan is testing converting everything everywhere all at once (or, in short, the Oscar patch). The views hooks can be converted without a problem and almost all *.views.inc becomes empty and is deleted. However, these includes not only contain hooks but helpers as well.: datetime_type_field_views_data_helper, views_entity_field_label, views_field_default_views_data

    So for now, πŸ› Hook info must be procedural and module handler needs to handle hook_info properly Active restores this functionality. These three helpers will need to be deprecated manually before hook_hook_info can be laid to rest.

  • First commit to issue fork.
  • πŸ‡ΊπŸ‡ΈUnited States nicxvan
  • πŸ‡ΊπŸ‡ΈUnited States nicxvan

    Also we'll have to deprecate this but keep the system and views one around since contrib uses those so much.

    The views hook classes support both types, but contrib will have to move those LegacyHook calls into the .module files at the least.

  • πŸ‡ΊπŸ‡ΈUnited States nicxvan
  • πŸ‡ΊπŸ‡ΈUnited States nicxvan
  • πŸ‡¨πŸ‡­Switzerland berdir Switzerland

    Per #31, we can't really deprecate this, not like we usually do.

    > Also we'll have to deprecate this but keep the system and views one around since contrib uses those so much.

    This doesn't just apply to system and views. It applies to all implementations. We can't tell modules to stop implementing this hook, because other modules that implement their hooks might still be in those files.

    I think we should add a mention to hook_hook_info() docblock that this should no longer be implemented but existing implementations must be kept for BC.

    And then we can just drop it in D12 (or whenever we remove the old hooks), no deprecation needed, it won't do anything anymore. It doesn't need to be converted, so no deprecation needed.

    So IMHO, this task is actually not blocked on anything and it's just a documentation issue.

  • πŸ‡ΊπŸ‡ΈUnited States nicxvan
  • πŸ‡ΊπŸ‡ΈUnited States nicxvan
  • πŸ‡ΊπŸ‡ΈUnited States nicxvan
  • πŸ‡ΊπŸ‡ΈUnited States nicxvan

    @berdir had a discussion in slack here about this: https://drupal.slack.com/archives/C079NQPQUEN/p1732497552707689

    I copied my thoughts to the IS.

    His approach is slightly different than mine, but I think they mesh well and will cause the least disruption.

    The main issue with hook_hook_info is this:

    If we deprecate it then modules will remove them.
    Any modules that used the include files will no longer be autoloaded and broken, but they got no notice it was deprecated.

    @berdir proposes that we update the documentation that it will be removed in 12.
    When it's removed we'll have deprecated procedural hooks and modules will have already moved theirs hooks except hook hook info.
    Modules that support 10, 11 and 12 can keep the hook hook info function since it provides the inc files (which presumably only have legacy hook implementations) This will break D12 sites that still have helpers in the .inc files.

    The approach I propose in case the summary is updated.
    We do what @berdir proposed, but also in 11.2 we deprecate the include files themselves.
    This will force any modules that utilize for example mymodule.tokens.inc to move all of the functions out of the .inc, whether it's a hook or a helper.
    Then when hook hook info is removed no modules will break because we've already deprecated the effect the hook hook info has.
    The include files can be deprecated when we add them in module handler. This should also hit all custom modules as well.

  • πŸ‡ΊπŸ‡ΈUnited States nicxvan

    I pushed up a POC, tons of tests should fail cause we have not removed the three helpers that core still has and this will trigger deprecations.

  • Merge request !10334Deprecate includes β†’ (Open) created by nicxvan
  • Pipeline finished with Failed
    26 days ago
    Total: 642s
    #349968
  • πŸ‡ΊπŸ‡ΈUnited States nicxvan
  • πŸ‡ΊπŸ‡ΈUnited States nicxvan
  • πŸ‡ΊπŸ‡ΈUnited States nicxvan

    I pushed up a first attempt at documenting the expected lifecycle of hook_hook_info.

    I also moved views.views.inc and datetime.views.inc so I could see which tests really fail.

    Commit f21048ab5d3d4c88733a1a1aeab3c9f9efddb955 will need to be reverted when the two issues postponing this one are in.

  • Pipeline finished with Failed
    25 days ago
    Total: 236s
    #351432
  • Pipeline finished with Failed
    25 days ago
    Total: 630s
    #351435
  • Pipeline finished with Failed
    25 days ago
    Total: 591s
    #351444
  • πŸ‡ΊπŸ‡ΈUnited States nicxvan

    Turns out I missed content moderation views inc I'll add that to the deprecation issue.

  • Pipeline finished with Failed
    25 days ago
    Total: 739s
    #351454
  • πŸ‡ΊπŸ‡ΈUnited States nicxvan

    The only failure is the one we need to add an expect deprecation for because it tests the functionality we're deprecating.

  • Pipeline finished with Failed
    25 days ago
    Total: 892s
    #352017
  • Pipeline finished with Success
    24 days ago
    Total: 651s
    #352460
  • πŸ‡ΊπŸ‡ΈUnited States nicxvan
  • πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10
Production build 0.71.5 2024