In an attempt to meet bc needs for hook discovery when a hook was discovered in a .inc file we store that file and include it runtime.
This was not enough and the real solution makes this unnecessary. The real solution was to store any inc files matching hook_hook_info implementations to auto include in order to load helper functions as well. Now that we properly store hook_hook_info includes all other .inc files with hooks should be manually included already in a .module file so we do not need to store these files and include them manually any longer.
Technically this means that we now autoload .inc files. This is an unexpected, undocumented and unwanted feature. We should create a CR that it will be removed in Drupal 12 in case anyone is relying on this.
MR is only to confirm it's not necessary in core, but should not be merged.
N/A
Publish CR that autoloading inc files should not be relied on.
Require that update_requirements and runtime_requirements are OOP only.
Add a loop to include install files during system runtime check??? (if we don't want to do this, then we can only do the next step in drupal 13)
Create follow up to actually remove $include functionality.
N/A
N/A
TBD
N/A
N/A
Show commandsStart 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:
changes, plain diff MR !13612
Comments
Comment #2
nicxvan commentedComment #3
berdirI'd vote to just drop those includes in D13 as we remove legacy hooks. Deprecating it will just confuse people because we don't really know if the files have explicitly been loaded or not. I think barely anyone explicitly relies on it being loaded. Webform has tons and tons of those legacy hooks in .inc files, but they're all loaded explicitly in the .module file.
What we could do is avoid pushing include files into both includes and group includes and I think that even fits into #3506930: Separate hooks from events because we introduce the new global includes list there and even have an explicit test asserting it's in both places. With that, the list will be a lot smaller.
I did a manual diff in my project to see what's left, it's lots of entries for hook_requirements() in .install files, various legacy drush.inc files, some dynamically loaded .inc files such as field_group_form_entity_form_display_edit_form_alter(). I also did find some actual non-converted hooks in webform.editor.inc for example (webform_filter_format_access), the uppercase implements docblock block that I mentioned before. But as mentioned, they are explicitly loaded.
I think the biggest risk is attempting to parse and run broken code, as modules might have old inc files with dead code that for example was never ported properly to D8+. But we're already doing that, apparently didn't run into any major issues so far and deprecating automatic including won't stop that.
Comment #4
berdirI pushed a change to that issue to deduplicate the two lists.
Comment #5
nicxvan commentedI think we can drop this in 12, maybe with just a cr.
I cannot imagine people are relying on this.
I suspect this was meant to solve hook hook info includes, but we missed helper functions so this was insufficient.
We now execute hook hook info build time to store those files and deprecate them.
I'm nearly positive this is vestigial.
Comment #7
nicxvan commentedComment #8
nicxvan commentedComment #9
nicxvan commentedWith: #3566140: Properly include install files for runtime requirements the only failure is module_implements_alter_test_altered_test_hook which is expected and will be removed in 12.
Just to record the slack discussion: catch confirmed this is unintentional and can be removed as part of the hook_hook_info removal as long as it happens after the HMIA removal.
Catch confirmed we can post a CR afterwards if someone was relying on this feature that was unintentionally introduced.
I think I'll mark this closed since it has served the purpose I was testing for.