Only load all modules when a hook gets invoked

Given how early the module handler initializes, that's not as shocking as I first thought. :-)

I suppose the real test will be moving the loadAll() call to the first invoke call. That's when things really start to asplode.

I have some questions and thoughts:
* I'm still concerned about modules bringing a procedural API with them, that could be used before first module invokation?
* If the first module hook invokation remains hook_init() this has no sense because at the first hook_init() call during "full bootstrap" everything will be loaded anyway?
* Making the lazy loading lazier will also enfore the if() check to be done more frequently, at each getImplementationInfo() call, does this really worth the shot to have this check *always* running even when loaded? I know it's a simple if() but it's a simple if() that will be called *a lot*
* Wouldn't it be simpler to leverage hook groups and moving all hooks implementations to additional files in order to empty .module files whenever possible in order to minimize the "module load all" operation performance impact instead? This would also mean to do smarter groups so that we wouldn't never ever load hook_menu(), hook_flush_cache() etc... by, for example, grouping all "cache related" hooks altogether

All your answers sounds right, indeed, thanks.

I started looking into this to see if it was just another case of "stupid installer". Unfortunately it's not. The fix for the installer is simple but once I got past that I kept running into more places where module code is assumed to be loaded in pre-hook-invocation scenarios. The most significant place is in old-style menu callbacks and the fix for this is to add a 'module' column to the menu_router table so that we can load the .module file before trying to call the page callback.

But then there are various other places where module files are assumed to be loaded, e.g. the EntityFormController calling field_attach_load() and apparently various other widgets and things that get called before the first time any module hook is invoked. A lot of these can be dealt with by passing the module_handler to the service in question.

And then there's this in the theme() function, which is often called before the first time a hook is invoked:

  // If called before all modules are loaded, we do not necessarily have a full
  // theme registry to work with, and therefore cannot process the theme
  // request properly. See also _theme_load_registry().
  if (!drupal_container()->get('module_handler')->isLoaded() && !defined('MAINTENANCE_MODE')) {
    throw new Exception(t('theme() may not be called until all modules are loaded.'));
  }

I'm sure these problems can be overcome and am happy to work on this issue - just want to get some feedback on whether it's worth persevering given the extent of the changes required. See the interdiff for how far I went with this. No point in getting testbot to test the patch yet though as there is still much borkage :-/

For page callbacks, we don't need an extra column. When we're building up the menu_router information we have the $module name available, so just default the "page file" file to be the .module file if not otherwise specified. It's included via a require_once(), so it should work fine. (And I hope we still get to remove that system entirely, but it's not gone yet.)

That said, the number of other function-based APIs we have floating around is still ugly. :-( Perhaps we should use this issue as a canary to ferret them out for prioritization for OO conversion, but not actually expect it get this committed any time soon? (Eg, "hey look, we need to make field_attach_load() and friends into an OO system next.")\

Seems like the benefits we could get from this qualify a "major" status.

patch no longer applies

After #340723: Make modules and installation profiles only require .info.yml files, this should be much simpler now.

However, removing the "main" loadAll() from bootstrap_code does not really buy us something, because it is immediately followed by:

  // Load all enabled modules
  \Drupal::moduleHandler()->loadAll();

  // Make sure all stream wrappers are registered.
  file_get_stream_wrappers();

...whereas file_get_stream_wrappers() invokes hook_stream_wrappers() in all modules, which causes all modules to get loaded. :-)

Thus, while we could proceed here in parallel, it's not going to make an effective difference until #2028109: Convert hook_stream_wrappers() to tagged services.

That said, it's possible that the stream wrapper hook is not the only one in bootstrap... perhaps we should perform an analysis of which hooks are getting invoked during bootstrap?

Yes, noticed that too about the stream wrappers and stopped looking at this due to that.

About the hook, it is the only one, cold cache:

stream_wrappers
module_implements_alter
stream_wrappers_alter
----HANDLE REQUEST----
language_negotiation_info_alter
entity_type_build
entity_type_alter
entity_load
view_load
views_pre_view
views_pre_build
views_query_alter
views_query_substitutions
views_query_substitutions
theme
theme_registry_alter
theme_suggestions_feed_icon
theme_suggestions_alter
theme_suggestions_feed_icon_alter
template_preprocess_default_variables_alter
views_post_build
views_pre_execute
views_query_substitutions
query_alter
query_views_alter
query_views_frontpage_alter
query_node_access_alter
schema
schema_alter
query_alter
query_node_load_multiple_alter
field_info_alter
entity_base_field_info
entity_base_field_info_alter
entity_load
node_type_load
entity_bundle_field_info
entity_load
field_config_load
entity_load
field_instance_config_load
entity_bundle_field_info_alter
entity_bundle_info
entity_bundle_info_alter
data_type_info_alter
entity_load
node_load
....

Warm cache:

stream_wrappers
stream_wrappers_alter
----HANDLE REQUEST----
entity_load
view_load
views_pre_view
views_pre_build
views_query_alter
views_query_substitutions
views_query_substitutions
theme_suggestions_feed_icon
theme_suggestions_alter
theme_suggestions_feed_icon_alter
template_preprocess_default_variables_alter
views_post_build
views_pre_execute
views_query_substitutions
query_alter
query_views_alter
query_views_frontpage_alter
query_node_access_alter
module_implements_alter
query_alter
query_node_load_multiple_alter
entity_load
field_config_load
entity_load
field_config_load
entity_load
field_config_load
entity_load
field_config_load
entity_load
node_load
...

There's a lot more after those of course...

1. How big a performance win could this be?
2. D8 or D9?

We need to get the stream wrappers issue in before this would help, I guess it depends on whether we can actually avoid loading the .module files completely, which might be possible on certain routes that don't load/render stuff.

Patch is ~2 years old, so it will need more than a reroll.

#2353357: hook_stream_wrappers_alter() should be removed as it is broken since modules are not loaded on demand is experimenting with this, as we need to get rid of that alter before this can make sense.

Posting a re-rolled patch from there, will re-focus that on the actual task there. Most remaining fails seem to be around functions not being found when submitting forms..

I think #2372745-2: KernelTestBase ignores extensions in site-specific directories is related here.

Reroll.

Is there any benchmarks using a real production environment (meaning an environment with OPcode cache configured sufficiently aggressively so that binary code is always into memory and files attributes are not checked when loaded) to prove this is really an significant performance improvement? A first baby step could be to simply remove all file_exists() in Drupal core and test this first? An smart OPcode cache loader won't attempt file system checks when told not to and include|require call are almost noop in that case since they keep the bytecode into memory, so in theory, adding code to conditionnally load would make a more significant overhead in the end. But this is pure theorical thought.

I don't know how much of a difference this is, just pushing the patch a bit, so we will eventually have something that works and that we can use to test and compare.

pounard, even if all php code will cached with OPcode cache binary code, each query to Drupal will parse all code (yes, not php-source but binary code, not from disk - from memory, without reading from disk) and spend CPU time for this.
For example, Drupal need to display only one node content (without views, blocks, comments, other fields and other modules), but site have 300 modules installed. And now for display only one node - server must execute all code from each module (load function list, initialize variables, defines, etc), and it spend CPU time for this. For example, core/modules/comment/comment.module file have 31 kb size, and for display node without comments I don't need any string from this file.

So solution with lazy load modules in Drupal 8 must improve the speed: it will load and execute code only for modules, that needed and used for current query, not from all installed on site modules.

As I see in some other PHP frameworks, they create list of links files vs functions and objects in array (function name + file name), and if function or object not already initialized - include needed file in realtime. Maybe we can implement same behavior in Drupal core?
For example, Drupal Views module not load all plugins every time, it loads only plugins, that needed for display current view.

I'm not sure this would be that significant, sure it worth a few benchmarks. But in the end, I do hope you would never ever build a site with 300 modules! If you look at Symfony2 which always load at least the Bundle class for each bundle, and ZF2 which always loads the Module class for each module, there not much difference in the end, especially when considering that D8 has not many things in the module file anymore.

@pounard I have a site with exactly 375 enabled modules.

• 56 commerce and commerce contrib
• 53 Features/Custom.
• 25 Performance (mostly authcache and advagg submodules)
• 22 core
• 21 fields related
• 12 Views
• 11 Search/Search API

The entire list (minus custom/features):
https://gist.github.com/4468dc6a12ca007610ff

And I don't even have Media. (Heard D8 media is splitting off to be a bit more modular like commerce). And I don't have Domain or OG modules which I very easily could add their ilk to the mix if needed down the road.

Would very much like to performance test this if it gets to a working state.

I've heard of a site that has upwards of 500 so unfortunately this seems to be a thing.

In the mean time I've got some pruning to do;)

Wrongly built site I guess, I mean if you want performances, start by writing 10 lines of code instead of using a 3000 thousand line module! As far as I am concerned, optimizing for sites with more than 100 modules is like trying to make a pig fly, it cannot possibly work.

Lazy loading modules would make the module.inc file even more complex, and would cause really complex problems to solve, such as answering the question on when should be the module loaded if it provides an API that other modules can call anytime ? D8 solves partially all this already by making the module file optional, but for D7 it's definitely a no-go in my opinion.

I think people should try to spend more time optimizing their sites before making complex code even more complex when there's absolutely no reason to do so. Having modules being lazy loaded would make potential wrong code and errors being silent because some modules would almost never be loaded, for example I saw a module with a typo error in a function call outside a hook making core call file_scan_directory() on live sites, if the module was lazy loaded, my arbitrary profiling traces would never have revealed it.

That rational around not loading some modules sounds good.

Though "wrongly built site"? If I want to maintain the functionality I'd not use a CMS i'd use a framework(and do).

Only loading a module when a hook is invoked and lazy loading a module is basically the same thing, the condition for the lazy loading being the hook being invoked. I really think this will add out-of-proportions complexity where there is already too much. Starting by refactoring the module.inc to remove most of its caches (half of it does not serve any purpose except masking another cache) and drop its complexity would be the first thing to do, then maybe a nice and simple module lazy loading solution on a sane basis could be doable. I actually did open a few issues about this a long time ago, most where just ignored. I found this very peculiar that most people wish to optimize the worst case scenarios (ie. having hundreds modules) whereas some other all-cases optimizations are just ignored.

Hum, that's a quite simple rule, and I guess that for D8 it makes sense since that modules aren't supposed to use hooks much, but I'm afraid that if you still are using hook_help() and a few others, you'd end up loading modules almost as much as if you don't put that in place, is there some numbers or stats about hooks being called in D8 ?

I'm a bit surprised at the sentiment expressed in Pounard's comments.

I don't have the skill to optimise site to the degree you seem to consider normal practise.

Modules are what make people like me use and enjoy Drupal. That's the great thing about Drupal: it's WYSIWYG, and there's a module for everything. Asking me to code optimisations is like giving me LEGO and then saying "well, you shouldn't really use it if you don't know how to extrude plastic yourself". To me Drupal is about democratising advanced functionality precisely to people without advanced skills.

Even on small sites I easily use over 150 modules. Lazy-loading them would seem to make a lot of sense. It can't be that hard to create "if page has geo-coder field, then load geocoder module and its dependencies" for example. But like I said: I'm not a pro-developer, I'm just a UX guy.

We can use __autoload PHP function http://php.net/manual/ru/function.autoload.php for auto-loading files when class is queried.

I kinda doubt we can implement this without some form of BC layer, aka. make this an opt IN behaviour for sites, which need this bit of performance. In general though these days, module files are quite empty for modules anyway, so the effect is probably much less, than expected, especially given that we kinda "require" opcode caching.

I'm working with a customer who is experiencing this issue on an intermittent basis, provided that the Shield and Key modules are enabled. Both modules have been implicated in related issues on drupal.org (see, for example, https://www.drupal.org/project/key/issues/2936885), and in both cases the underlying issue seems to be loading site config at too early a phase in Drupal's bootstrap procedure. Also in both cases, it seems that a solution to _this_ issue would prevent the problem.

My understanding of the problem, at a high level, goes something like this:

1. One of these modules loads config in a middleware layer, very early in Drupal's bootstrap process, before all modules have been loaded.
2. Presumably this involves firing off hook_entity_config_alter() implementations —but since not all modules have been loaded, not all hook implementations are fired.
3. The result is cached. Later on when other parts of Drupal attempt to load config, they simply refer to the cached object. This can lead to fatal errors if certain modules expect the entity type config to have been altered in certain ways —for instance, how the `menu_ui` module adds its `list_builder` handler.

I don't think the patch here still applies, but I wanted to note that this is still an issue —not sure if it'd be better to advocate for a fix here, or in the queues of any module that does this sort of thing. Recommendations?

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 #3207813: ModuleHandler skips all hook implementations when invoked before the module files have been loaded, 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 #2345611: Load user entity in Cookie AuthenticationProvider instead of using manual queries

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 drush cr.

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.

Created a follow up https://www.drupal.org/project/drupal/issues/3391394 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: https://www.drupal.org/project/drupal/issues/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.

@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.

OOP hooks arrived

#74 >I'd be fine with just going ahead with deprecating procedural hooks.

Is the idea to try to introduce the deprecation in D11 or is that too soon?

Answering my self

(crossposted with your self-answer, but still posting it now that I wrote it...)

That's not entirely clear yet, I think there's no specific plan, also depends on what you understand by deprecating.

procedural hooks are deprecated, for module and themes and support for them will be removed in D13. There are no phpunit or phpstan deprecations/rules yet for that however.

@nicxvan and I have slightly different opinions on the approach, I prefer to start deprecating early and more explicitly (we currently only trigger deprecations. But I've also seen what happens in larger contrib right now with the plugin annotations we added, any test gets you a massive wall of deprecation messages which can be tedious when you're trying to for example fix tests. Hooks would likely be worse and depending on where you we do it, could happen frequently enough that it might have a measurable performance on real sites.

It's possible that we'll skip deprecating that entirely and just directly deprecate .module files as I said in #73. For that, we first need to get rid of .module files in core, we're actively working on that here: #3566536: [meta] eliminate core .module files.

Personally, I'd vote to close this as won't fix. Per #65, this *will* result in fatal errors in contrib, I don't see how we could prepare or deprecate for that and our efforts are better put into getting rid of .module files

Yeah I think we'll get this roundabout by deprecating .module files.