Problem/Motivation

In #3571067: [12.x] Remove hook_hook_info and support in HookCollectorPass, we removed explicit and implicit (which was a more or less accidental new feature in the initial OOP hook implementation to try and do hook_hook_info without hook_hook_info) include file loading.

We are however still discovering hooks in those include files. The reason for that is that we support a feature where modules can put hooks in include files and then load them explicitly and always in their .module file.

The problem is that if modules don't do that but just have their old auto-included hook files still laying around, then it currently results in very confusing exception:

InvalidArgumentException: Class "entity_usage_views_data" does not exist.

Steps to reproduce

Proposed resolution

This was discussed in slack in https://drupal.slack.com/archives/C079NQPQUEN/p1783632281242269, with the conclusion that we kind of have two options:

1. We stop support for legacy hooks in include files completely. That is this issue. This will break things like https://git.drupalcode.org/project/better_exposed_filters/-/blob/7.0.x/b... in a way that was not explicitly announced before. I found that with this search: https://search.tresbien.tech/search?q=require_once%20f%3A%5C.module%24%2.... Note that only some of these matches are affected, if they contain non-legacy hooks.

2. We improve the error handling around missing hooks. There are a few options, from rethrowing it as a better exception to silently ignoring them, which would kind of restore the behavior of non-existing hooks in D10. #3549397: [regression] Uninstalling and installing modules during config:import can lead to fatal errors is a related issue, which specifically deals with one weird edge case but just in general ignores non-existing hooks. We might want to do this anyway even if we do option one as well as I think the DX is really not great right now if you remove a legacy hook (haven't seen yet what happens exactly with an OOP hook)

In the slack thread, @catch gave a preliminary +1 to doing this (option 1). I think the impact is fairly limited for contrib. The D12 bot is converting all hooks to OOP anyway and most projects are going through with that, so in practice, most contrib projects will already be fully converted to OOP and this will be a non-issue for them then. Custom modules I think are fairly unlikely to have includes like this, this is I think a practice that's kind of a leftover from before D8.

A somewhat related issue is #3574003: Properly include install files for install hook_requirements where I also proposed to stop scanning .install, which we only do for hook_requirements().

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3610009

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

berdir created an issue. See original summary.

nicxvan’s picture

nicxvan commented

If we go with 1 do we throw an exception if we find functions without the legacy hook attribute in a .inc?

If it is a helper function there is 0 harm in adding the legacy hook attribute.

That works allow contrib to still maintain some global functions in. .inc files if they really want to.

berdir opened merge request !16336

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

> If we go with 1 do we throw an exception if we find functions without the legacy hook attribute in a .inc?

No. We completely stop looking at .inc files. And if we don't look at them, we also can't throw exceptions or anything else based on what might or might not be there.

It's impossible to know if a function in an .inc file is a hook or not when we can for them, so not point in trying.

Started a MR, but this should almost certainly fail, for example on the new ignore dir tests.

nicxvan’s picture

nicxvan commented

Maybe just rename it to a .module file it will still be collected and it will be ignored. Then the test will work.

Then we include it manually in the real .module so we can preserve the test.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Active » Needs review

That's the neat part, we don't have to keep the test. The check for ignored directories is gone, as is arbitrary recursion into folders. .inc files are the only ones that can be in unknown folders, so that part can completely go. that was always going to be the case with D13, this means it's already possible with D12.

We could in theory even refactor it to remove the filter function completely if we start discovery in src/Hook and add .module/.install manually to the list, but I think we can do that in D13.