Properly include install files for install hook_requirements

Thoughts on my suggestion?

No, I mean my reopened mr https://git.drupalcode.org/project/drupal/-/merge_requests/14799

what MR is meant to be reviewed?

Think if you do option 1 drush should be fixed first

From the issue summary on option 4:

> Nicxvan does not think this is viable since it breaks update and runtime requirements calls too.

Not with the the ModuleHandler change that I'm proposing in my MR. Yes, it requires a workaround there, but it's isolated and relatively temporary.

As my MR shows, the code is already being loaded.

In turn, what I don't like about option one is the inconsistency in the registered hook information. IMHO, that system should either know a hook and if so, support loading it, or it shouldn't know about it.

In slack, we discussed that based on my approach, we could actually go one step further and in general stop parsing .install files in D12. They can be very large and include many update functions and that would be a decent performance win. The problem: We wouldn't be able to trigger a deprecation on hook_requirements() anymore, at least not during hook discovery.

Wanted to follow up on this one to see about next steps.

Added subheadings for the options to make it clearer.

Option 2, ' Downside every invoke and invoke all call has to check if it's requirements' seems really ugly to me. That's going to be a LOT of checks.

> Option 4: Nicxvan does not think this is viable since it breaks update and runtime requirements calls too.

A lot of modules use the same code for both even though the hooks have been split. (Including core!!!)

Option 3 I don't know enough about and there doesn't seem to be a MR for it?

> Option 1: Manually load all install in drupal_check_module
> Downside it requires an issue in drush.

Drush has already had to make a new major for Drupa 12, hasn't it?

Hooks are technically an implementation API - it's inward-only. Hooks aren't really mean to be invoked by other code, they're just meant to be implemented.

What we should do is add an API that gets the collected requirements, and Drush can call that. Would it help Drush if we backported that API to 11?

> A lot of modules use the same code for both even though the hooks have been split. (Including core!!!)

I don't really get that nor the original feedback. I want to repeat again that with the invokeAllWith() check that added in the MR, nothing is broken. Any operation works as long as they first load the include files, which was always required before OOP hooks were introduced in D11, which added logic to try to automatically include include files that hooks lived in, which was never a feature before that, only the predefined include files supported by hook_hook_info(). And later on support for hook_hook_info() was also added, so we had two different include systems on top of each other. The referenced issue removed both, which exposes the inner inconsistent behavior that we currently have:

We scan for and register requirements hook in install files, and then implicitly expect that _all_ install files are loaded first and unconditionally initialize and expect those callbacks to be loaded. That's the reason you get the class exists error, because it calls into \Drupal\Core\Utility\CallableResolver::getCallableFromDefinition(), which does a return-early on the is_callable(), but if that function isn't loaded at that it falls through all the other cases and implicitly assumes that if it's not a currently-available function, then it must be a class, but then it's confused because it's also not a class.

Keep in mind that this is about the install operation, which is called on a not-installed module. We never call the install operation on the installed modules, we never call it with invokeAll(). This is always a single-invoke() on a specific module that's about to be installed. ModuleHandler just over-eagerly prepares all requirements hooks anyway on a single-invoke. What happens then is invoke() doesn't find the hook (obviously, the module is not installed) and falls back to legacyInvoke(). All that weirdness is why we split the install operation out of the hook system entirely into special classes, and split the remainder of the requirements hook into two different regular hooks. But we need to deal with the old requirements hooks because we deprecated it too late to be able to remove it in D12 entirely.

I'm not pushing for option 4 because it fixes drush out of the box (it does because it falls back to how it worked in D10. a single include works with a single invoke. This is also how drupal_check_module() works in core now). I'm pushing for option 4 because it makes the behavior consistent: .install functions are not handled by the new hook system, and this allows us to not scan them at all. This isn't doing that yet, because we need a new way to trigger deprecation messages in that case. But I think it would help performance if we can skip parsing those files.

This isn't about drush and whether or not drush should use API's or call hooks directly. As mentioned above, the same assumption that drush 13 has is also in drupal_check_module() and this is broken too. Drush 14 actually uses drupal_check_module(), the reason it didn't before is that it's not an API function, it's a UI helper, which adds stuff to the messenger. Drush 14 now calls that function and then has to fish the message out of the messenger service.