HookCollectorPass fails to register hooks for the correct module in some contexts

That should not be happening, we sort the module names by length so the longer modules should be picked up first, I can take a look though.

Oh I see it's being assigned to the submodule.

> Oh I see it's being assigned to the submodule.

Yeah. We have no way of knowing which module it belongs to, length really doesn't mean much.

I'm pretty sure we discussed this once around possible optimizations and so on. #3493043: Prevent hooks that do not go through module handler from being added to the container. addresses a similar issue with update functions.

Right now, we default to possible edge cases of a module maybe implementing a hook for another. That's a weird and IMHO extremely rarely edge case. We support it now with new hooks too, but at lest now it's an explicit decision (and i hope very, very rarely used).

I think we should default to assuming that hooks are implement for the current module, that's really the 99% use case and only if not, then we see if it maybe matches another. In other words, start with a str_starts_with() and only if that fails go for the preg_match().

It's not possible to figure out whether my_module_library_info_alter is module my_module_library hook info_alter or module my_module and library_info_alter. You can change things but it'll just break some other case. The information is simply not there. This has always haunted the module system #548470: Use something other than a single underscore for hook namespacing

There is one way out: we could borrow doxygen parsing from the rector rule but only fire it if there's a prefix collision for performance reasons -- there's no need otherwise. This would require collecting modules which have a prefix collision and if the module preg hits one of those then look for Implements hook_foo() in doxygen. The rector rule now has code to correctly deal with all sorts of Implements hook_FOO_bar(). That's the only way out. If the function name doesn't have doxygen on it then you can't tell what's up and what's down.

preg_match_all('<#' . implode('[^#]+|#', $modules) . '[^#]+#>', '#' . implode('#', $modules) . '#', $matches); finds modules which have a prefix in another, these are the ones to run the doxygen parser against. https://3v4l.org/vt4Qt

Converting it should also disambiguate it too.

No, strictly speaking it's not possible to figure it out 100% reliably. But again, implementing a hook for another module is a a rare edge case. token used to do it but I changed that. It seems like a pretty safe assumption that if a hook exists in a module and it matches the module prefix, then it's a hook that the developer intended to be for that module, not another.

The other option would be to register both/all possibilities, that would essentially keep the current behavior?

That might work, I can take a look later.

> The other option would be to register both/all possibilities, that would essentially keep the current behavior?

Brilliant idea, thanks that should work and should be easy to implement too, upgrade the preg_match to preg_match_all \PREG_SET_ORDER and loop it.

I wonder if we really want to do this, while that does ensure the right hook is registered it also ensures the wrong one is too.

You already have an issue in the old system.

In procedural land if:
paragraphs_layout library_info_alter
paragraphs_layout_library info_alter

You'd get errors and even without that the wrong hook would get called.

We have a way to properly deduplicate this, which is to convert it and add #[LegacyHook]

What I would think we want to do is detect if there is an ambiguous module name like this and set a message to convert.

Maybe we do 10 and set a deprecation if we detect a duplicate?

Though typing that out I realize it won't work, this would also blow up the registry cause we would pick up every function in paragraphs_layouts as matching paragraphs too.

So, possible solutions are

1. Default to functions belonging to the module they are in and only if there is no such prefix match then do a module_preg to find "implements on behalf of others". This only changes behavior if there are two modules where one is a prefix of another. Currently my_module_library will be matched as hook module before my_module even inside my_module.module / .inc. After this change, it won't be. It's a change. I have no clue whether the BC policy allows for making this change.
2. Parse doxygen if the prefix problem is present. We are adding more code to a BC layer
3. Just store both splits. This means the size of the implementations container parameter grows. ncixvan says this is in particular a big hit for webform, there would hundreds of duplicates.
4. Demand conversion for these ambiguous cases. I have no clue whether the BC policy allows for this one. We do have the rector rule and ncixvan already did core and he is converting webform now with it. That's proof enough it is working. Note however group_content_menu and more which would pressure group to convert. This might be a hard sell.

I think we need direction here.

Without thinking loads about it, what if we did #1 and also issued a deprecation so we can do #4 in Drupal 12?

Currently my_module_library will be matched as hook module before my_module even inside my_module.module / .inc. After this change, it won't be. It's a change. I have no clue whether the BC policy allows for making this change.

That's fine with a change record.

Edit: I crossposted with catch's comment. The IS, however, contains more information than my comment.

If we do 1 then a module could never implement on behalf of a submodule if the submodule uses a prefix. Since paragraph will always match before paragraph_layout etc.

Edit: in procedural code.

If we do 1 then a module could never implement on behalf of a submodule if the submodule uses a prefix. Since paragraph will always match before paragraph_layout etc.

Are there any known examples of this? I can't think of a use case for it.

Yeah, I see the theoretical problem, but also struggle with a use case for it. Submodules like that typically extend the main module, which shouldn't really depend or know about the various submodules/subprojects that extend it.

FWIW, I think the whole concept of implementing hooks for another module is a really bad practice and is way more often an undesired, unexpected side effect than it's done on purpose. I understand we want to support this as it's currently possible and _might_ be used, but with all the possibilities that now exist, I think there should almost always be better alternatives. We support flexible hook order and removal of other hooks, but there should be very few cases to do something in the name of another module like that.

token used to do that extensively to define tokens "in the name of" some core modules, such as field tokens, I removed that as mentioned above. with legacy hooks, if that other module ever implements that hook too it's a fatal error.

And yes, while actual conflicts between modules like that are rare as it needs a combination of valid hook names and the submodule name, we don't know which hooks exist and would have to treat almost every single function of a submodule as hooks for two or even more modules. That's also why we can't deprecate this, because we couldn't deprecate actual conflicts only. Unless we still store all possible conflicts and only trigger on real conflicts at runtime. The cost of that just seems too high, agree with that.

1 is iMHO the most sensible option (surprise, it's my idea), we assume that people did the expected/common thing and implement hooks for their module. If there really are real-world edge cases out there that get hit by this, they can convert to OOP.

I took a stab at implementing a fix for this. A couple notes about my approach:

1. I decided to split the regular expression code from HookCollectorPass::collectAllHookImplementations() into a separate private method so that it can be reused.
2. Instead of simply checking whether the function name prefix matches the module being scanned, I decided that it was appropriate to do a separate, higher priority regular expression match instead. This ensures that the ownership shortcut also avoids update hooks (as seemingly desired in the regular expression).
3. The cost of generating this additional expression for each installed module should be negligible; it basically consists of a sort and preg_quote() on a single array element, and string concatenation. The ownership expression should also short-circuit the match against the full module list in 99% of cases since most modules only define hooks for themselves.
4. I believe only having a unit test should be sufficient -- the one I wrote passes, and fails as expected when running test-only changes.

Patch file for 11.2.4 subtree split enjoyers :)

Thank you for working on this, we know one consequence of this is that parent modules cannot implement these hooks on behalf of the sub modules anymore. It's the inverse of this issue, but the rarer by far most likely. We also probably want to deprecate on behalf of moving forward anyway.

The only question is do we want to do a CR for this since this can break implementations?

Confirmed Test only fails.

Asked in slack, then decided this likely does need a CR, it could use some work since it's very much an edge case and hard to describe.

Leaving it in needs review for that.

The cr is much clearer now thanks to the edits @berdir made.

I made some minor tweaks, I think this is ready but berdir mentioned he wanted to take another pass so I'll leave it open for now.

Looks good to me, but needs a rebase, as #3493043: Prevent hooks that do not go through module handler from being added to the container. just landed.

Got a duplicate here

Removing tag based on 14 which also affirms that this needs a cr. Forgot that when I asked in slack, but this has one now!

Updating the title to be a bit clearer and calling out that the duplicate has an excellent summary.

I don't want to overwrite this issue since I think it's pretty close.

I rebased and also added the missing return type hint.

Ready for review again!

Ran test-only test:

PHPUnit 11.5.39 by Sebastian Bergmann and contributors.
Runtime:       PHP 8.4.12
Configuration: /builds/issue/drupal-3502302/core/phpunit.xml.dist
..F                                                                 3 / 3 (100%)
Time: 00:00.041, Memory: 8.00 MB
There was 1 failure:
1) Drupal\Tests\Core\Hook\HookCollectorPassTest::testPrefixOwnership
Failed asserting that two arrays are identical.
--- Expected
+++ Actual
@@ @@
 Array &0 [
-    'theme_suggestions_alter' => Array &1 [
+    'suggestions_alter' => Array &1 [
         'Drupal\Core\Extension\ProceduralCall' => Array &2 [
-            'test_module_theme_suggestions_alter' => 'test_module',
+            'test_module_theme_suggestions_alter' => 'test_module_theme',
         ],
     ],
 ]
/builds/issue/drupal-3502302/core/tests/Drupal/Tests/Core/Hook/HookCollectorPassTest.php:128
FAILURES!
Tests: 3, Assertions: 4, Failures: 1.
Exiting with EXIT_CODE=1

So looks good!

Can move to RTBTC. I have reviewed the code and comments have been addressed?

Marking RTBC per #34.

I took another pass.

My contribution was documentation and adding a return type that @berdir pointed out was missing while rebasing.

I read through the cr again to refresh my memory and it looks great!

Rtbc +1

Committed/pushed to 11.x, thanks!