Deprecate module_load_install() and replace it with ModuleHandler::loadInstall

I guess this is independent of the UpdateModuleHandler patch.

Oops, just saw that @chx is assigned, but I guess you're working on the parent issue?! Sorry, in case any toes were stepped on.

No problem.
module_load_include() wasn't converted itself, and I feared that converting the two would lead into all sorts of installer/update/foo-related problems, so I didn't take that up. I'll do that no, however, and see what breaks.

Ahh, in module_load_include() there's this gem.

/**
 * @todo The module_handler service has a loadInclude() method which performs
 *   this same task but only for enabled modules. Figure out a way to move this
 *   functionality entirely into the module_handler while keeping the ability to
 *   load the files of disabled modules.
 */

I guess that's a feature that's specifically needed for loadInclude(). So that makes the patch above broken, IINM.

EDIT: Fix weirdo syntax highlighting

Yeah, to do this cleanly we would need to convert drupal_system_listing() into a class we can inject into ModuleHandler. I'll see if I can come up with something.

OK, this is embarassing...: https://api.drupal.org/api/drupal/core!lib!Drupal!Core!SystemListing.php...

OK, here's as far as I got.

module_load_install() is currently being (ab)used for loading the install profile's .install file. I had no more steam for fixing that.

Also, this does not yet work, Drupal does not yet install. Somehow update.fetch.inc from update module is not loaded.

At least Drush installs with this. Let's see.

A note for reviewers: I went the extra mile of splitting the 'loader' part into a dedicated interface, because not doing that would have meant tainting the ModuleHandler with a SystemListing object. I wanted to avoid that, because I think it's a very powerful feature that ModuleHandler relies solely on the module list.

Suggestions on everything (!), but also specifically on naming improvements are very much welcome.

Title and OP are a bit contradictory:
Is this actually about module_load_install() or about module_install()?

Anyway. Let's review what we have here:

1.

I went the extra mile of splitting the 'loader' part into a dedicated interface, because not doing that would have meant tainting the ModuleHandler with a SystemListing object. I wanted to avoid that, because I think it's a very powerful feature that ModuleHandler relies solely on the module list.

Nice. Yes, I agree that ModuleHandler should only act on our (enabled) $moduleList, making sure that code of uninstalled modules is clearly separated and never called here. We don't know yet the outcome of #1199946: Disabled modules are broken beyond repair so the "disable" functionality needs to be removed, but if it doesn't get postponed to D9, one thing is clear: there won't be a thing such as completely disabled modules anymore. Either the disabled status gets removed altogether and replaced by some convenient, or at least some code continues to run.
So at this point we only need to separate out uninstalled/never installed modules (forthcoming paradigm), but may separate out disabled modules as well (current paradigm). In any case, a ModuleHandler method should never be able to call or load any uninstalled module's code.

So what you're doing is fine. However we should take notice that #2001310: Disallow firing hooks during update brought us UpdateModuleHandler (extending ModuleHandler), and of #2004784: Move module install/uninstall implementations into ModuleInstaller wanting to bring us ModuleInstaller (implementing enable(), disable(), uninstall() and probably to be added install() in a new ModuleInstallerInterface. If the latter one lands, we could just roll loadInstall() into ModuleInstaller without tainting ModuleHandler, and that might be enough.
We probably need to further track the direction of all of these issues to keep things a bit consistent. It'still so much a moving target... :(

2.
It is very nice to have a separate loadInstall() method. ModuleHandler::loadInclude should certainly disallow the 'install' type now, because we're no longer loading the install API there:

   public function loadInclude($module, $type, $name = NULL) {
-    if ($type == 'install') {
-      // Make sure the installation API is available
-      include_once DRUPAL_ROOT . '/core/includes/install.inc';
-    }

Now I think we should push this paradigm a bit further:
First of all, we don't want loadInclude() load any file. As the name says, it should load includes (PHP files with the ending .inc) and not any '.php' or '.module' or '.whatever' file. This would also allow us getting rid of the $type parameter.
At the moment we're seeing loadInclude() being inconsistently implemented:
Drupal::moduleHandler()->loadInclude('locale', 'fetch.inc')
Drupal::moduleHandler()->loadInclude('views', 'inc', 'views.theme')
Drupal::moduleHandler()->loadInclude('views', 'inc', 'includes/ajax')
While the first one is convenient, it slightly abuses the second parameter.
Much cleaner if we can leave out the $type parameter, so it always looks like this:
Drupal::moduleHandler()->loadInclude('locale', 'locale.fetch')
Drupal::moduleHandler()->loadInclude('views', 'views.theme')
Drupal::moduleHandler()->loadInclude('views', 'includes/ajax')

3.

+  // @todo The install profile is enabled using module_enable(), so eventually
+  //   this function is called with the profile name as $module. There should be
+  //   a dedicated ProfileHandler that handles this.

Yes, a ProfileHandler (ThemeHandler, too?) would be nice. Fine to be a followup.

4.
We don't want to load includes for disabled modules, so I believe loadInclude() should only implemented in ModuleHandler and shouldn't act on SystemListing.
The other way around, we actually don't want ModuleHandler to call any install hook or load the file containing it. This should be possible only on uninstalled modules. But for that we'd need to separate out the actual install hook from our install files, so we'd have a real install file that can only be loaded if uninstalled, an uninstall file with the uninstall hook that can only be loaded when installed, an update file that is only called by the new UpdateModuleHandler and a schema file that even could be in YAML format (leveraging the Symfony model, see #1263478-29: Identify pieces of Drupal that could be replaced by Symfony code.

5.
Deferring more nitpicks to get the general paradigm settled first... :)

1. Yeah, this was basically spawned by the UpdateModuleHandler patch (which got in already). Removing the 'disabled modules' concept is not necessarily relevant to this patch. When installing a previously uninstalled module, we still need some facility to load its install file. I disagree with "#2004784: Move module install/uninstall implementations into ModuleInstaller" basically for the same reason I mentioned here: It would taint the ModuleHandler with SystemListing information. I must admit I haven't followed that to closely so far.

2. While I agree that loadInclude()'s parameters are completely bogus, IMO changing them is out of scope of this issue. IIRC there are over 50 invocations of that in core. Also this is a complete bikeshed, which parameters to change, remove, etc.

3. Yay! :-)

4. In theory I agree with everything you said. The thing is that we need loadInstall() for both enabled and disabled/uninstalled modules. And loadInstall() currently calls loadInclude(). Personally, I think refactoring that would be out of scope for this issue, that might be something to do together with the follow-up in 2. I haven't looked into the concrete implications of splitting loadInstall() and loadInclude(), however, it might be more trivial than I imagine.

5. OK. :-)

1.

I disagree with "#2004784: Move module install/uninstall implementations into ModuleInstaller" basically for the same reason I mentioned here: It would taint the ModuleHandler with SystemListing information.

Why would it taint the ModuleHandler? It would just taint the ModuleInstaller, and I'd say that's okay.

2. There's #507396: Change module_load_include() parameters, and I still believe we will come to a conclusion. It's all too obvious that it needs to be changed to the better. Slightly changing 50 invocations is no problem at all, given the huge amount of changes we're having anyway in D8.

4.

Personally, I think refactoring that would be out of scope for this issue, that might be something to do together with the follow-up in 2. I haven't looked into the concrete implications of splitting loadInstall() and loadInclude(), however, it might be more trivial than I imagine.

Yes, it's very trivial. And while it might be out of scope here, it might influence our decision, where to put all of this. If our issue here were critical or major, or very complex, then we should probably get it fixed, do the rest in followups and possibly come back and refactor. But in this case I'd rather propose doing it altogether.

1. I didn't know the direction of that issue was to *not* re-(ab)-use ModuleHandler. I totally agree with the later comments in that issue.

2. Well let's discuss that there, then. Right changing 50 invocations is no problem. It makes a patch very succeptible to re-rolls, though... So, yeah, let's leave that out for now.

4. Hmm... I'll see what that would actually change in terms of real code.

@tstoeckler:
Regarding the disentangling, I'm preparing a patch in #507396: Change module_load_include() parameters later tonight.
We don't have much time left here either, so we should move this one into ModuleInstaller per #14.1.
But for that we need #2004784: Move module install/uninstall implementations into ModuleInstaller moving on.
Do you have time for a new patch now? Otherwise it would be on my to-do list for tonight. Will be a long night. Needs to be... :)

too many changes since the last patch in the core, trying to make the patch for 8.6.x

Working on #697946: Properly deprecate module_load_include() and move it into \Drupal::moduleHandler() service it's clear this loading install should be separate method

I have created patch for 9.1.x-dev.

Clarified issue summary. As stated in the original IS, this issue is a follow-up to #2001310: Disallow firing hooks during update, so I read that to determine what the intention of this issue was.

#31 no longer applies. It also does not properly deprecate module_load_install() (it just removes it) and it contains many coding standards problems.

I think we've gotten a little off-track here over the years. The main reason for this issue is to deprecate the procedural function module_load_install($module) and provide ModuleHandler::loadInstall($module) as a replacement for it. This is consistent with the larger effort in the meta, #3097045: [META] Provide modern replacements for and deprecate the legacy include files.

By off-track I mean that instead of just moving the code the above patches are trying to change the implementation of the load install, and also change the way other pieces of code (e.g. ModuleHander::loadInclude()) work.

I think that given the fact this issue has been bouncing around for 8 years now, and that we're trying to get the procedural code removed before D10 (less than 6 months from now!), a more productive and safe strategy is to just do the straight deprecation and code move. We know that will work because it's working currently. Improvements on the ModuleHandler implementation should be deferred and handled in a separate issue.

Here is my patch to do this. It is basically written from scratch, but includes bits from #697946: Properly deprecate module_load_include() and move it into \Drupal::moduleHandler() service because that issue was expanded recently to overlap this one. I have also added a draft change record for the changes in my patch.

I aborted the test in #36 because there was a mistake in the patch. Here is the corrected patch.

Because the app.root service is already being injected into the ModuleHandler, we should use that instead of DRUPAL_ROOT.

One line diff ...

It has collision with #697946: Properly deprecate module_load_include() and move it into \Drupal::moduleHandler() service

@andypost Yes, I'm aware. I mentioned this in #36. Regardless, I think that it would be easier to handle them separately. This issue is a lot less complicated that the other, and keeping them separate makes both easier to commit IMO.

Re #42:

Surely that is an issue relevant to the app.root service itself, and NOT an issue with code (including this patch) that simply uses that service.

The function module_load_install() is deprecated.
There is a deprecation message test add.
All usage of function have been replaced.
I have updated the IS.
The IS and the CR are in order.
For me it is RTBC.

Edit: for the added method Drupal\Core\Installer\Form\SelectProfileForm::__constructor(), the class is not extended in contrib. See: http://grep.xnddx.ru/search?text=SelectProfileForm&filename=

1. +++ b/core/includes/schema.inc
   @@ -165,9 +165,10 @@ function drupal_uninstall_schema($module) {
      @trigger_error('drupal_get_module_schema() is deprecated in drupal:9.2.0 and is removed from drupal:10.0.0. No direct replacement is provided. Testing classes could use \Drupal\TestTools\Extension\SchemaInspector for introspection. See https://www.drupal.org/node/2970993', E_USER_DEPRECATED);
   +  $module_handler = \Drupal::moduleHandler();
      // Load the .install file to get hook_schema.
   -  module_load_install($module);
   -  $schema = \Drupal::moduleHandler()->invoke($module, 'schema');
   +  $module_handler->loadInstall($module);
   +  $schema = $module_handler->invoke($module, 'schema');
   

   This is a deprecated function and therefore should not be changed. It is okay for deprecated functions to use deprecated functions.

2. +++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php
   @@ -247,6 +247,16 @@ public function moduleExists($module) {
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function loadInstall($module) {
   +    // Make sure the installation API is available.
   +    include_once $this->root . '/core/includes/install.inc';
   +
   +    return module_load_include('install', $module);
   +  }
   

   This makes me very unsure... we're just calling out to procedural code again. I think it might be better to move module_load_install() and module_load_include() to bootstrap.inc. That way they'd be autoloaded by composer and we could then work on removing the rest of module.inc. The usages of these method should go away as we replace them with solutions based on the autoloader.

@alexpott this seems close to landing; if we get this in then #697946: Properly deprecate module_load_include() and move it into \Drupal::moduleHandler() service can come next to deal with module_load_include()? That issue is kind of stalled as it's duplicating this one somewhat, but splitting the scope in two seems easier to deal with.

@longwave as stated in #45 I'm not really convinced that adding these to ModuleHandler is really the right way to go. Loading code is our autoloaders responsibility - this is all legacy stuff. The call out to the extension list in #697946: Properly deprecate module_load_include() and move it into \Drupal::moduleHandler() service from loadInclude() so we can include modules that are not installed is a step backwards. It means the ModuleHandler and extension list service have a circular dependency. If we want to get rid of module.inc we should move these methods to bootstrap.inc and then work on deprecating the other methods here. And then we need to work on properly OO-ifying the systems that are dependent on this code.

The two points in #45 seem contradictory to me. The first says leave the procedural call in the deprecated function, the second says the procedural call should be removed from the deprecated function. I don't understand the reasoning.

To the first point, I would prefer not to leave the procedural call in drupal_uninstall_schema(). As the deprecation says, "No direct replacement is provided" for this function, so anyone trying to deal with the deprecation in their own code is going to look in core to see how it is implemented and what they can replace it with. If the only answer is to replace the deprecated function with another deprecated function, that's not helpful at all. Core should be serving as an example here, as the source code in core is often the best or only documentation. I feel it is best to remove all uses of module_load_install() from core if we're going to deprecate it.

To the second point, in my view the big step we're taking here is changing the API and making the deprecation, as that's the task that is constrained to happen only for a major point release. The details of the implementation may be changed at any time. And in particular #697946: Properly deprecate module_load_include() and move it into \Drupal::moduleHandler() service will be replacing this use module_load_include(). I think it's important to leave it in until that issue figures out how to re-write module_load_include() so that it covers all the current use-cases in core and so that we don't interfere with what that issue is trying to do.

I don't think we should "move module_load_install() and module_load_include() to bootstrap.inc". module_load_install() especially - since it is rarely used, we really don't want it to be loaded on every page. Moving these functions just pushes the problem down the road. I think there's an argument to keep this functionality out of the autoloader because having the autoloader do discovery of functions in randomly named include files seems like a lot of overhead to me. When we have one class per file, with a defined directory structure, finding classes can be simple and efficient. But finding functions when there are many functions per include file, and the include files can be anywhere, that seems like something that might be better left in the ModuleHandler. Contrib is always going to have to deal with explicitly loading procedural code at some level.

Maybe just decorate classloader for lookup module name in namespaces?

we really don't want it to be loaded on every page. Moving these functions just pushes the problem down the road.

Move them into the module handler will also load the code on every page. Adding to the ModuleHandler interface gives us more problems down the road.

Move them into the module handler will also load the code on every page

Hmm, yes I guess it will.

Adding to the ModuleHandler interface gives us more problems down the road.

OK, then how about moving module_load_install() into the ModuleInstaller? That was one of the ideas discussed above over the past 8 years. I don't really want to rehash that discussion, but if you think that's a better place then I'll re-roll the patch to do that.

Regardless, if the goal is to remove and replace the procedural code in module.inc, it has to go somewhere. Is it realistic to talk about extending the autoloader for this purpose before D10? Or should it just be moved to a service like ModuleInstaller (which doesn't get loaded on every page, I hope ...).

Regardless, if the goal is to remove and replace the procedural code in module.inc, it has to go somewhere.

I think this goal is of questionable value. Procedural code does not equal bad code. When we first embraced object orientation we perhaps went too far and didn't leverage composer's ability in to include files as much as we should have. As both of these methods are about including code I think it is fine to park them in a file loaded by composer. And then eventually we should replace all of their usages with autoload-able classes. Yes that means coming up with a plan to do that but for me thats better than moving to some place. making contrib change and then having to change again when finally work out to do the things these functions enable us to do.

Also these functions are not big functions and we already load them on every request (see \Drupal\Core\DrupalKernel::loadLegacyIncludes()) so moving them to bootstrap.inc or ModuleHandler does't save us anything.

I see only the one related issue #1308152: Add stream wrappers to access .json files in extensions

No reason to load any includes except .install file (for schema only) because they very probably needs namespace registered in classloader, so only accessing composer's data using extension lists could be compromise.

Not sure a stream is a good wrapper to get a path to some extension to try load a file... or pick it from some map from compposer

moving them to bootstrap.inc or ModuleHandler does't save us anything

it prevents loading one more include

No reason to load any includes except .install file (for schema only)

This problem will solve itself as we have #3221051: [meta] Replace hook_schema() implementations with ::ensureTableExists(). After that the hook_schema can be removed.

1. +++ b/core/includes/module.inc
   @@ -15,12 +15,16 @@
   -  // Make sure the installation API is available
   -  include_once __DIR__ . '/install.inc';
    
   -  return module_load_include('install', $module);
   +  @trigger_error('module_load_install() is deprecated in drupal:9.3.0 and is removed from drupal:10.0.0. Instead, you should use \Drupal::moduleHandler()->loadInstall(). See https://www.drupal.org/project/drupal/issues/2010380', E_USER_DEPRECATED);
   +  return \Drupal::moduleHandler()->loadInstall($module);
   

   There's no need to change the underlying code here. We don't need to preserve the ability to load code from uninstalled modules in the new function.

2. +++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php
   @@ -247,6 +247,16 @@ public function moduleExists($module) {
   +    return module_load_include('install', $module);
   

   This can call $this->loadInclude()

3. +++ b/core/lib/Drupal/Core/Extension/ModuleHandlerInterface.php
   @@ -125,6 +125,17 @@ public function buildModuleDependencies(array $modules);
   +  public function loadInstall($module);
   

   Add a string typehint to $module. I'm not sure that the minimum PHP version of Drupal 9 allows for a return typehint of string|false which is a shame.

4. +++ b/core/lib/Drupal/Core/Installer/Form/SelectProfileForm.php
   @@ -93,7 +119,7 @@ public function buildForm(array $form, FormStateInterface $form_state, $install_
   -        module_load_install($extensions['profile']);
   +        $this->moduleHandler->loadInstall($extensions['profile']);
   

   This might be the one place where we need to load code that is not installed. I think we should inline the ability to do that here. There is an issue to remove this check so this functionality might not exist much longer anyway.

1. +++ b/core/includes/install.inc
   @@ -1027,7 +1027,7 @@ function drupal_requirements_severity(&$requirements) {
    function drupal_check_module($module) {
   -  module_load_install($module);
   +  \Drupal::moduleHandler()->loadInstall($module);
   

   This is one other place where we need to load code from an uninstalled module. This function places very strict requirements on a module's hook_requirements() implementation. Given the module's code is not autoloadable at this point (because the container doesn't have the module namespace added). Therefore I think it is acceptable to inline the functionality here too. If we change the functions in install.inc to methods on some service we can consider a public API for this but I'd be an advocate for not doing this. Eventually it'd be great if all this requirement checking prior to module installation was replaced by a module's composer.json.

2. +++ b/core/includes/schema.inc
   @@ -165,9 +165,10 @@ function drupal_uninstall_schema($module) {
      @trigger_error('drupal_get_module_schema() is deprecated in drupal:9.2.0 and is removed from drupal:10.0.0. No direct replacement is provided. Testing classes could use \Drupal\TestTools\Extension\SchemaInspector for introspection. See https://www.drupal.org/node/2970993', E_USER_DEPRECATED);
   +  $module_handler = \Drupal::moduleHandler();
      // Load the .install file to get hook_schema.
   -  module_load_install($module);
   -  $schema = \Drupal::moduleHandler()->invoke($module, 'schema');
   +  $module_handler->loadInstall($module);
   +  $schema = $module_handler->invoke($module, 'schema');
   

   Don't change deprecated code. There's no need to.

Looking at the current implementation of ModuleHandler::loadInclude() - I think we should move to that instead of adding a new method.

Addressing #57. #58 and #59. No interdiff because no longer adding ModuleHandler::loadInstall() so the interdiff would be twice the size of the patch. Plus the last patch is over 6 months old.

+++ b/core/includes/install.inc
@@ -1033,7 +1033,15 @@ function drupal_requirements_severity(&$requirements) {
 function drupal_check_module($module) {
-  module_load_install($module);
+  include_once __DIR__ . '/install.inc';
+  if (\Drupal::hasService('extension.list.module')) {
+    /** @var \Drupal\Core\Extension\ModuleExtensionList $module_list */

lolz - this is install.inc... plus given we're using \Drupal::service below - checking for the existence of the extension.list.module service is unnecessary. It has to exist for the module handler to exist.

+++ b/core/tests/Drupal/KernelTests/Core/File/FileSystemRequirementsTest.php
@@ -45,7 +45,7 @@ public function testSettingsExist() {
-    module_load_install('system');
+    $this->container->get('module_handler')->loadInclude('entity_test', 'install');

Whoops. Fixing this.

Now that #697946: Properly deprecate module_load_include() and move it into \Drupal::moduleHandler() service has landed we can move the test to the better location.

+++ b/core/includes/module.inc
@@ -15,8 +15,15 @@
+ * @deprecated in drupal:9.4.0 and is removed from drupal:10.0.0. Use
+ *   \Drupal::moduleHandler()->loadInclude($module, 'install') instead. Note,
+ *   the replacement no longer allows including code from uninstalled modules.
+ *
+ * @see https://www.drupal.org/node/3220952
  */
 function module_load_install($module) {
+  @trigger_error('module_load_install() is deprecated in drupal:9.4.0 and is removed from drupal:10.0.0. Instead, you should use \Drupal::moduleHandler()->loadInclude($module, \'install\'). Note, the replacement no longer allows including code from uninstalled modules. See https://www.drupal.org/project/drupal/issues/2010380', E_USER_DEPRECATED);

We deprecated module_load_include() for Drupal 11 because of the amount contrib usage. This method only has 3 pages - http://grep.xnddx.ru/search?text=module_load_install&filename= - I think that that means we can remove in Drupal 10. Will ping a release manager.

I have updated the IS and the CR.
All the code changes look good to me.
For me is it RTBC.

Committed/pushed to 10.0.x and cherry-picked to 9.4.x, thanks!