Only load all modules when a hook gets invoked

I tried to use patch from comment #28 and made tests pass. Marking issue as NR so as to get direction on next steps to work on. Thanks.

Impressive that you got this green. it looks like it required a lot of not so nice workarounds though :)

One question. In practice on a regular page with a warm cache (but disabled page cache/logged in user), what exactly does this mean in practice? What's the first hook that is invoked and how early is that?

A long time ago in #16, that was stream wrappers, which is now a service, then it was an entity load. Would be good to document.

One thing to keep in mind is that the performance benefit of this is fairly fragile. This would also solve 🐛 ModuleHandler skips all hook implementations when invoked before the module files have been loaded Needs review , which kind of does the same in the current implementation, but I suggested there that we should disallow that. This would obviously conflict with this issue. That means modules like shield can undo all possible performance benefits this would bring us.

I bet first hook will be loading current user after 📌 Load user entity in Cookie AuthenticationProvider instead of using manual queries Needs work

For the open threads. Also think the IS could use some love from 10 years ago

Re #61, I tried to get list by below method:

1. Did a standard profile installation through UI.

2. Added below code to system.module file.

function system_module_implements_alter(&$implementations, $hook) {
  \Drupal::messenger()->addStatus(t($hook));
}

3. Cleared cache from `admin/config/development/performance`.

4. Reloaded home page.

5. Triggered hooks from above code are:
* entity_load
* view_load
* element_plugin_alter
* date_format_load
* element_info_alter
* views_pre_view
* views_data
* action_load
* entity_base_field_info
* entity_base_field_info_alter
* entity_field_storage_info
* field_storage_config_load
* entity_field_storage_info_alter
* search_page_load
* field_views_data
* field_config_load
* entity_bundle_field_info
* entity_bundle_field_info_alter
* field_views_data_alter
* comment_type_load
* base_field_override_load
* views_data_alter
* views_plugins_sort_alter
* views_plugins_filter_alter
* views_plugins_area_alter
* views_plugins_pager_alter
* views_pre_build
* views_plugins_query_alter
* views_plugins_style_alter
* views_plugins_row_alter
* views_query_alter
* views_query_substitutions
* views_post_build
* views_pre_execute
* views_plugins_cache_alter
* query_alter
* query_views_alter
* query_views_frontpage_alter
* node_grants
* query_node_access_alter
* views_post_execute
* views_plugins_exposed_form_alter
* views_pre_render
* views_post_render
* entity_create_access
* node_type_create_access
* theme
* theme_registry_alter
* theme_suggestions_views_view
* theme_suggestions_alter
* theme_suggestions_views_view_alter
* template_preprocess_default_variables_alter
* theme_suggestions_pager
* theme_suggestions_pager_alter
* theme_suggestions_container
* theme_suggestions_container_alter
* display_variant_plugin_alter
* system_display_variant_plugin_alter
* block_load
* entity_access
* block_access
* query_entity_query_alter
* query_entity_query_block_content_alter
* menu_load
* system_info_alter
* block_alter
* block_build_alter
* block_build_system_branding_block_alter
* block_build_search_form_block_alter
* block_build_system_menu_block_alter
* block_build_local_actions_block_alter
* block_build_system_messages_block_alter
* block_build_local_tasks_block_alter
* block_build_system_breadcrumb_block_alter
* block_build_node_syndicate_block_alter
* block_build_page_title_block_alter
* block_view_alter
* block_view_page_title_block_alter
* block_build_help_block_alter
* block_build_system_main_block_alter
* block_view_system_main_block_alter
* block_build_system_powered_by_block_alter
* page_attachments
* file_url_alter
* page_attachments_alter
* page_top
* page_bottom
* theme_suggestions_html
* theme_suggestions_html_alter
* breakpoints_alter
* toolbar
* shortcut_default_set
* shortcut_set_load
* theme_suggestions_menu
* theme_suggestions_menu_alter
* link_alter
* user_format_name_alter
* toolbar_alter
* theme_suggestions_toolbar
* theme_suggestions_toolbar_alter
* theme_suggestions_page
* theme_suggestions_page_alter
* block_view_system_branding_block_alter
* theme_suggestions_block
* theme_suggestions_block_alter
* theme_suggestions_region
* theme_suggestions_region_alter
* block_view_search_form_block_alter
* form_alter
* form_search_block_form_alter
* theme_suggestions_input
* theme_suggestions_input_alter
* theme_suggestions_form_element
* theme_suggestions_form_element_alter
* theme_suggestions_form_element_label
* theme_suggestions_form_element_label_alter
* theme_suggestions_form
* theme_suggestions_form_alter
* block_view_system_menu_block_alter
* block_view_local_actions_block_alter
* menu_local_actions_alter
* block_view_system_messages_block_alter
* block_view_local_tasks_block_alter
* entity_form_mode_load
* entity_form_mode_info_alter
* entity_view_mode_load
* entity_view_mode_info_alter
* local_tasks_alter
* menu_local_tasks_alter
* block_view_system_breadcrumb_block_alter
* system_breadcrumb_alter
* theme_suggestions_page_title
* theme_suggestions_page_title_alter
* query_entity_query_shortcut_alter
* entity_preload
* query_shortcut_load_multiple_alter
* entity_storage_load
* shortcut_storage_load
* shortcut_load
* block_view_help_block_alter
* help
* block_view_node_syndicate_block_alter
* theme_suggestions_feed_icon
* theme_suggestions_feed_icon_alter
* block_view_system_powered_by_block_alter
* theme_suggestions_off_canvas_page_wrapper
* theme_suggestions_off_canvas_page_wrapper_alter
* theme_suggestions_big_pipe_interface_preview
* theme_suggestions_big_pipe_interface_preview_alter
* library_info_build
* library_info_alter
* css_alter
* js_alter
* js_settings_build
* js_settings_alter
* theme_suggestions_links
* theme_suggestions_links_alter
* ajax_render_alter

Marking this issue as again NR so as to check if I am working in the right direction.

I think warm caches is a more useful scenario, at least the generic stuff like plugins and so on. What's more interesting at this point is the *first* hook and specifically the call to loadAll(), to confirm it's really later than it is now.

Did some quick tests myself and can confirm that's the case with the latest version now. For the frontpage it's going through the views controller and then loading the view, for an entity route, it's entity upcasting (the user entity actually, not the node, due to problematic context stuff, but that's another issue).

I also confirmed that for example the load call in \Drupal\views\Routing\ViewPageController::handle() really is required, and without it, it's a fatal error. And that's the main issue here now, BC. Getting this into a minor release would break all kinds of custom and contrib controllers immediately, with no way of providing BC. This might only be possible to get into the actual 11.x release like this, and then we need to announce that this will happen, because we also can't do deprecation warnings, as there's no way to detect this.

What we could do is focus on making this patch smaller, by removing the need for as many of those manual hardcoded load calls as possible. For example views_add_contextual_links() should be moved to a class method. There are a bunch of related issues to it, some it would help with but none that does exactly that: https://www.drupal.org/project/issues/drupal?text=views_add_contextual_l... → .

The call in \Drupal\block\BlockListBuilder::buildBlocksForm() for example does not seem to be needed, because it then calls load(), and that triggers entity hooks. ContextualController needs it for _contextual_id_to_links(), that is really only called there and in a test, that could be moved to a static method too. The file upload stuff has various existing issues that would help I suppose. And so on.

Changes can be done in related issues for user_logout and views_add_contextual_links and this issue can get benefit from that.

Changes between previous passed commit bcf67669 and above commit 6c25e9f6 are mainly related to making changes in batch_test.module and form_test.module, so as to make tests pass. Similar changes can also be done for deprecation_test.module.
These all changes might not to be done in this MR and separate issues can be opened similar to #66

But before working on that it will be good to get framework manager feedback.

Also it might be possible that code still breaks at many places due to missing tests for some feature. Eg progress() method in FileWidgetAjaxController.

Left a comment on the MR.

I agree with @Berdir's comment in #65 - the changes to move .module code to services etc. should all be split into separate issues.

Removing ::loadAll() from DrupalKernel feels like an 11.x-only issue to me. We could still do the ::loadAll() changes in ModuleHandler in a minor release.

The question then is how to communicate the change to contrib modules so that we can actually remove the up-front loading in 11.x

I think we need to do something around this due to 🐛 ModuleHandler skips all hook implementations when invoked before the module files have been loaded Needs review but also long term there's 🌱 [META] Hooks via attributes on service methods (hux style) Active which is essentially going to deprecate .module files altogether - not for a couple of major releases though probably.

Created a follow up https://www.drupal.org/project/drupal/issues/3391394 📌 Remove ::loadAll() from DrupalKernel Active issue. I will do the changes once confirmed by @catch.

Seems solution is still being worked out.

Also for IS update.

I am hiding all patches as work is in the MR 4458. Looking good!!

I was directed to use the approach here instead of the patch in #3207813 . I don't see a patch in here and the merge request has quite a few changes. Is this ready for use on 10.3.x? It seems there's still some discussion on approach and the code looks like a work in progress.

@joelg you can apply the patch from the other issue as a stopgap, but for anything to get committed it would probably be the approach here.

@catch: Per #68, the long-term thing actually happened, I wonder if we should at this point skip this and work toward deprecating .module files entirely?

The BC aspect of that is much clearer to handle than the your-module-files-might-or-might-not-be-loaded-so-better-not-call-those-functions-in-your-controller this issues causes. We can just trigger deprecation notices for every .module file we load then. Lots of work of course to get there.

@berdir we'll still have the issue in 10.x until EOL if we do that, but also this is potentially disruptive to fix, so we might not even be able to backport it to 10.x anyway. I'd be fine with just going ahead with deprecating procedural hooks. In fact it may even be the case that the OOP hooks patch has at least changed how this works since hook discovery was rewritten.

OOP hooks arrived