[pp-many] Deprecate hook_requirements

Created on 1 December 2024, 7 months ago

Problem/Motivation

Deprecate hook_requirements

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

πŸ“Œ Task
Status

Postponed

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 States nicxvan
  • πŸ‡ΊπŸ‡ΈUnited States dww

    Pretty sure we're now down to 2. πŸ€“

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

    We should also update hook_requirements() itself (the module.api.php documentation function) to refer to the replacements and make it clear that this is deprecated.

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

    Not sure about the version. It's technically not deprecated yet, but all replacements *are* in 11.2, so I think it can be argued to backdate the version it's deprecated in to 11.2 and possibly also remove this in D12?

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

    The issue is detecting that the replacements are there.

    The deprecation would continue being thrown when supporting 11.1

    I discussed this briefly with @catch on slack and he suggested deprecating in 12 for removal in 13.

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

    I see, but that seems like a blocker for actually converting to the new hooks, because we don't want to to run both and the result in a mess when merging them?

    But I'm not sure this is really an issue?

    We have #LegacyHook and a special version of that for hook_module_implements_alter().

    We might miss a few edge cases that already converted hook_requirements() to OOP, but I think we never officially supported that, even if it would work for runtime/update phase. And I can't find anything with #[Hook('requirements' on http://codcontrib.hank.vps-private.net/search?text=%23%5BHook%28%27requi....

    So all it would take is another special case in \Drupal\Core\Hook\HookCollectorPass::addProceduralImplementation() and trigger a deprecation?

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

    I see, but that seems like a blocker for actually converting to the new hooks, because we don't want to to run both and then end up with a mess when merging them?

    In contrib? No that's why Catch suggested for 12

    We might miss a few edge cases that already converted hook_requirements() to OOP

    I think there might be a disconnect, hook_requirements explicitly cannot be oop.
    That is why it was replaced with update and runtime requirements and the new class.

    If we deprecate the original hook in 11 then contrib has to think about how to support 10.

    If we wait they can just drop less than 11.2 when they convert.

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

    What I mean is that I think we should make sure that we don't call both hook_requirements() and hook_runtime_requirements() of the same module, because they both do the same thing.

    We do have #LegacyHook, with that, D10 would be a non-issue. you just keep the old hook and call through to the new hook depending on $phase. But just like hook_module_implements_alter(), the problem is 11.1, which skips LegacyHook but doesn't understand the new requirements hooks.

    The problem with waiting is that the new hooks and their change records are out. There are already early adopters and there will be more over the next months. field_encrypt for example now implements hook_requirements() *and* the new hook_runtime_requirements(). Right now, we call both and merge them together. We use a flat array_merge(), so it's not a huge deal, the legacy hook just wins and we ignore the same keys from the runtime requirements.

    So we could leave it at that, but I'm not 100% sure that this won't cause any unexpected issues.

    What we should have done is add it's own legacy attribute like LegacyModuleImplementsAlter, so that we can identify that and skip it in 11.2+. Alternatively, we could deal with when we call the hook, for example change \Drupal\system\SystemManager::listRequirements to call the runtime hook first with invokeAllWith(), remember the modules, and then skip them in a second invokeAllWith() for the legacy requirements hook.

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

    The problem is that not all instances of hook_requirements go through module handler invoke so the attribute doesn't work unless we introduce reflection there.

    Maybe that is what you are suggesting and maybe that is worth doing.

Production build 0.71.5 2024