Deprecate hook_requirements

Pretty sure we're now down to 2. 🤓

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.

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?

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?

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?

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.

Started a merge requests to show what I mean. It's pretty verbose, but it handles deprecation and supporting BC versions with the assumption that a module with a runtime_requirements hook only implements hook_requirements() for BC.

Expecting \Drupal\Tests\system\Kernel\Module\RequirementsTest to fail with a deprecation, so we'll have to figure out testing around that. Might want to convert that module to new hooks because it's used in a few tests and have two new modules. One that only implements hook_requirements() and hook_requirements_alter() and triggers deprecations and that has both and doesn't.

And we'll need to do something similar for update and install requirements.

Would this need a CR or is it covered under another?

> Would this need a CR or is it covered under another?

IMHO we don't, because we already have all the deprecations that explain the hook_requirements() deprecations, we just didn't formally deprecate it.

> We're concerned about performance, maybe we do a naive check somewhere for the existence of hook_requirements without one of the three replacements and set a message there.

No, performance is not a concern here. It is complex code, but this isn't going to be a measurable performance regression. It's specific to requirements hooks. system_requirements() alone can take multiple seconds with all the entity schema and other checks. If anything, it will be faster, because we won't run the same requirements checks twice for modules that want to be backwards compatible.

The only concern is code complexity, I agree that this is quite a lot, but also managable.

The complexity here is because we can't rely on LegacyHook due to 11.1 and we don't want to invoke the legacy and new hook for the same module, not the actual deprecation. If we'd deprecate the old hook then modules wouldn't have a way to avoid that deprecation message.

I've been thinking about this and I think we should just add another dedicated legacy hook attribute. That solves the runtime issue, becaause those hooks just don't exist anymore.

That just works for update/runtime because those are regular hooks. And install requirements are far easier because they area done one-by-one and have no alter, so we can just put the old hook in an else.

Needs review to get feedback on the concept, this will fail tests obviously.

I also had the idea that we could possibly generalize this into an attribute that you can provide a version for, because we'll deprecate more hooks where you'll want to keep the old one. Your build-time deprecation issue will run into that a problem because we won't be able to have conditional runtime logic like I'm doing with install here.

Something like #[HookSkipAfterCoreVersion('11.1')]. It allows you to skip and acknowledge that you just provide that hook for compatibility with earlier versions and want it skipped now.

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Updated tests, made the failing test a deprecation test that we can then just remove as we have dedicatated tests for each new hook. I also removed the deprecation message from drupal_check_module(), HookCollectorPass will trigger that, whether it's install, update or requirements. The only change there is only calling it as a fallback. I think edge cases like having an install requirements class and not returning something is not really an issue, in that case the old implementation probably also won't return something.

Unsure about the change record situation. We have 3 separate, not-connected change records about the new hooks. We can a fourth that hook_requirements() is now really really deprecated and reference all with each other. But I think it would be easier for people if there were just one.

I still think we should have done this as part of introducing the new hooks (at least with the last of the replacements and conversions landing because the situation in 11.2 is confusing. We could backport this to 11.2 without the deprecation message.

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

The reason I change the order is that we don't want to call both the old and new requirements hook of the same module. for runtime and update, that's now taken care of automatically because it's no longer registered, but for install, it falls back to the legacy function exists stuff, as the module isn't installed yet.

In most cases, calling both might just be a slight overhead in case it does something slow (it shouldn't, but who knows), but there might also some extra weird edge cases when merging the results together.

Created a new change record to group them together. Didn't find time yet to update the existing ones and cross-reference them as I had to leave.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Rebased.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Rebased again

This looks fine, but there is one part of the comment that is c&p from the hook_module_implements_alter() version and enough words need changing that gitlab suggestions didn't feel like the way to do it.

Oops. Updated the docs. It's a bit weird/challenging what exactly happens on 11.2/3 now, I don't know if this has a chance to be backported to 11.3 but I guess not and that ship definitely sailed for 11.2. It's almost certainly not a major issue if both hooks are run, maybe it's slightly slower, but still a bit weird/confusing. But not much we can do about that now.

Looks like the push didn't go through.

Discussed in slack with @longwave about 11.4 vs. 11.3 - since not having the legacy attribute makes it awkward to implement the new hooks, decided to backport this to 11.3 - I directly changed the versions (and reworded one sentence) prior to commit.

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

Published change record.