[Plan] Determine how to deprecate procedural hooks.

Created on 17 October 2024, 6 months ago

Problem/Motivation

πŸ“Œ OOP hooks using event dispatcher Needs review Allows for OOP hook implementations.

We should deprecate procedural implementations that do not use LegacyHook.

Steps to reproduce

Proposed resolution

It may be as simple as setting a deprecation in ProceduralCall: https://git.drupalcode.org/project/drupal/-/merge_requests/7604/diffs#25...

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

πŸ“Œ 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

Comments & Activities

  • Issue created by @nicxvan
  • πŸ‡¬πŸ‡§United Kingdom catch

    We'll need to convert all core hook implementations before we do this, and would be good if the rector rule was available to contrib too given 99.9% of modules will need it.

    There are a couple of things I'm not sure about here:

    At some point we will want to stop discovering and invoking procedural hooks, this could be either Drupal 12.0 or Drupal 13.0 - it's quite an expensive bc layer.

    At that point, any remaining hooks will be 'dead code' and not run. This is a bit different from some deprecations where there is a clear error message when the thing is removed, that code will just stop running instead. We would not want to keep the discovery around just to throw an exception.

    Given the eventual removal of bc will be 'silent' rather than a hard break, that makes me wonder if we should deprecate in 11(.2?) for removal in 13.0, then there is plenty of time for noisy deprecations. The disadvantage of this is that we'd have to keep the expensive bc layer around, but maybe we can put it behind a feature flag so that sites can turn it off, if they know they're 100% converted over

    It is really great that we came up with LegacyHook for 10.x support since this means modules can convert to using the new stuff immediately and still support 10.3 or even 10.2, then one day rm example.module when they drop Drupal 10 support.

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

    I'd vote for deprecation in the next minor version after all of core is converted.

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

    πŸ“Œ Deprecate Procedural hook implementations Active is a super naive implementation of this. Chx pointed out this is to expensive to really do it this way most likely.

    I plan on cleaning up my conversion script for contrib over the next week or so.

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

    Also, only slightly related, but I think I'm going to create a cleanup meta now that core is converted to clean up some inc and module files that only really have constants or a single helper function.

  • πŸ‡¬πŸ‡§United Kingdom longwave UK

    Are we almost at the stage where we can deprecate .module files entirely? It would be very cheap to throw a deprecation warning if the .module file still exists, and convert that to an error in the next major.

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

    Not quite, but close.

    I think we need to figure out follow up 2 here: https://www.drupal.org/project/drupal/issues/3472165 πŸ“Œ [PP-1] Explore converting Install hooks to OOP Active

    Then we need to figure out how in themes / on behalf of themes.

    Hook hook info
    Hook module implements alter

    And where to put cost and define statements.

    Then we can.

    I think we're close to deprecating .inc

    We just have to figure out the three helpers for views and Hook Hook info for that I think https://www.drupal.org/project/drupal/issues/2233261 πŸ“Œ Deprecate hook_hook_info() Active

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

    I think we need to decide whether we want to remove support for procedural hooks in Drupal 12 or 13. For me, even if it's automated, it's probably going to be too much to remove support in Drupal 12 - there are a lot of sites with a lot of old custom modules.

    Also even though I know the reasons why, it feels a bit hard to say you can't have a .module file but you have to have a .install file - giving ourselves a bit more time might mean we can figure out alternatives for hook_install() et al by then.

    We've got two main problems keeping the bc layer in:

    1. The performance cost of legacy hook discovery. If πŸ“Œ Add a feature flag for the procedural hooks bc layer Active works we could make that opt-out in Drupal 11, opt-in in Drupal 12, gone in Drupal 13.

    2. Hook ordering and etc. if we can come up with an all-OOP hook ordering system, I think it would be good and fine to deprecate hook_module_implements_alter() for removal in Drupal 12 - if you have procedural hooks in Drupal 12, you don't get to order them and it's a bug report/task against the module to convert if it breaks something.

    If the above is roughly OK, then deprecating .module files in their entirety, for removal in Drupal 13, starts to sound pretty good then - we give people a lot of warning and then do a clean sweep, and they've got until the end of Drupal 12 support to get ready.

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

    Hook documentation needs to be converted to OO as well before we do this.

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

    api module support is progressing. We have generic attribute support just needs a test -- this has been so for weeks now, I have been helping nicxvan with the Oscar patch instead of api work -- and we have a proof of concept for #Hook support in specifics.

    Once πŸ“Œ [PP-1] Determine how to implement Form Alters with attributes. Active is done we can write a very simple Rector rule which splits hook docs onto relevant attribute classes.

    As far as I am concerned πŸ“Œ Investigate ModuleHandler::add Active is the most urgent here, it needs to be gone as soon as possible, that method is a complete lie now as it only works with procedural hooks, it can't work with anything else and so it subtly blocks progress.

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

    Also, if we want to prioritize .install etc, there is a path we can take where we just have a special serviceProvider like class for the install time ones, we don't get the true benefits of OOP, but it would still give some organization options and we could deprecate .install files at that point.

    I think these are the ones in .install still:
    * - hook_install()
    * - hook_post_update_NAME()
    * - hook_schema()
    * - hook_uninstall()
    * - hook_update_last_removed()
    * - hook_update_N()

    These are sometimes in .install, but they already support OOP
    * - hook_module_preinstall()
    * - hook_module_preuninstall()
    * - hook_modules_installed()
    * - hook_modules_uninstalled()

    Once that is broken down it would make it easier to scope out how to truly convert those hooks.

    The really big hurdle I think just from an amount of code in .module files is handling:

    • hook_preprocess_HOOK
    • hook_process_HOOK
    • hook_theme_suggestions_HOOK

    As well as handling hooks called from themes, if we solve those two things that covers like 80% (made up percentage) of the remaining code in .module files I think, most everything else in my experience are helper methods that are in .module for convenience and can be moved, or things for batch / queues, but those already have a way to move.

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

    Oh yeah we also have to get rid of hook info and replace hmia too.

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

    Can't forget hook_requirements which has πŸ“Œ Replace hook_requirements with tagged services Active

    It hook_install_tasks and alter.

    I think realistically if we want to really deprecate procedural hooks we will need to complete follow up 3 and 4 here πŸ“Œ [PP-1] Explore converting Install hooks to OOP Active

    That likely will involve specific classes for most install time hooks so we can load them manually.

    If we decide to ignore hooks that do not go through modulehandler (e.g. ones invoked directly by moduleinstaller install.core.inc and during update)

    I think that leaves:
    hook_hook_info πŸ“Œ Deprecate hook_hook_info() Active
    hook_module_implements_alter πŸ“Œ Hook ordering across OOP, procedural and with extra types Active
    hook_requirements πŸ“Œ Replace hook_requirements with tagged services Active

    Info and requirements have a solid plan with some follow ups ready for review.
    Hmia needs a plan.

  • πŸ‡ΊπŸ‡ΈUnited States nicxvan
  • πŸ‡¬πŸ‡§United Kingdom catch

    I think it's fine to deprecate procedural hooks without sorting out ones that can't be converted yet, it will just need clear documentation on the change record etc.

    Ideally it would be good if we had solutions for install hooks and etc. before we completely disallow procedural hooks in Drupal 13 because I think that will be harder to explain. But all those things can be worked on in parallel.

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

    Once we solve those three we can likely just deprecate: https://git.drupalcode.org/project/drupal/-/blob/11.x/core/lib/Drupal/Co... directly and that should filter everything.

  • πŸ‡ΊπŸ‡ΈUnited States nicxvan
Production build 0.71.5 2024