Properly deprecate module_load_include() and move it into \Drupal::moduleHandler() service

Attached patch marks the module_load_include() function as deprecated and replaces it with a combination of require_once(), drupal_get_path(), and file_exists().

It may be worth investigating whether this change affects performance in any significant way.

Note that removing the function altogether would require changing the documentation (and perhaps the API) of the drupal_build_form() function:

 *     - files: An optional array defining include files that need to be loaded
 *       for building the form. Each array entry may be the path to a file or
 *       another array containing values for the parameters 'type', 'module' and
 *       'name' as needed by module_load_include(). The files listed here are
 *       automatically loaded by form_get_cache(). By default the current menu
 *       router item's 'file' definition is added, if any. Use
 *       form_load_include() to add include files from a form constructor.

The form_load_include() docs would likewise need to be modified:

 * Use this function instead of module_load_include() from inside a form
 * constructor or any form processing logic as it ensures that the include file
 * is loaded whenever the form is processed. In contrast to using
 * module_load_include() directly, form_load_include() makes sure the include
 * file is correctly loaded also if the form is cached.

Trying again. using include_once() instead of require_once().

Needed another if (file_exists(...)).

Trying again...

And again...

And again...

(sigh)

EDIT: Yay!

<img src="http://drupal.org/files/happydance.gif" />
EDIT: Okay, enough celebration.

The Summary does not justify this change well. I don't yet understand the problem we are solving. Lots of functions fail if used at wrong time.

Updated issue summary.

I prefer to centralize including code where feasible so that you can breakpoint and understand when code is being loaded from disk. The Summary is much better now but I guess I disagree. It is possible that module_load_include() should be broken up into different functions but not removed. my .02

It is possible that module_load_include() should be broken up into different functions but not removed.

If you have any specific suggestions as to how it should be broken up (or "fixed"), I would be happy to roll a patch.

I have one specific suggestion.
module_load_include() really helps if you have a file of the pattern "$module.something.inc".
However, if you have sth like "theme/theme.inc" in views module, then you need a stupid third argument.

A simple suggestion: Allow value of FALSE for the third argument.

function module_load_include($suffix, $module, $prefix = NULL) {
  if (!isset($prefix)) {
    $prefix = $module . '.';
  }
  elseif ($name === FALSE) {
    $prefix = '';
  }
  else {
    $prefix = '.';
  }

  if (function_exists('drupal_get_path')) {
    $file = DRUPAL_ROOT . '/' . drupal_get_path('module', $module) . '/' . $prefix . $suffix;
    if (is_file($file)) {
      require_once $file;
      return $file;
    }
  }
  return FALSE;
}

Expected usage:

// views module, "views.install"
module_load_include('install', 'views');

// views module, "theme/theme.inc"
module_load_include('theme/theme.inc', 'views', FALSE);

// instead of old-style, confusing
module_load_include('inc', 'views', 'theme/theme');

This quite a cheap gain.
We could also make up variations of this, where the difference to D7 would be bigger, but that might feel more sane.

----------

Of course, all of this would be only for procedural code. Anything OOP should be included with autoloading, esp. the Drupal 8 PSR0 stuff.

Hm, maybe my comment in #20 would be better placed in
#507396: Change module_load_include() parameters
or #1366026: replace module_load_include() with module_include() ?

Personally, I really don't think there's any point in removing this function. It's ridiculously small and simple and basically just abstracts some logic like resolving file location relative to drupal root and checking whether the file exists. IMO if we don't have a function like this we're basically gonna have the same repetitive code all over the place to achieve this.

That said, I think merging this and module_load_install() and possibly renaming it to just module_load() and clearing up parameters as suggested in #20 sounds like it could be a good thing.

suggestion in #20 is not less confusing than the original function.

ctools_include() or my suggestion #1366026: replace module_load_include() with module_include() is much more usable.

Update using Issue summary template.

The proposed patch is extremely old and needs a reroll. It looks like there are still quite a few usages of module_load_include() in 8.x.

15: deprecate-module_load_include-697946-15.patch queued for re-testing.

corrected invalid file names passed to include.

Needs a reroll, and:

+++ b/core/includes/module.inc
@@ -128,9 +133,11 @@ function module_load_install($module) {
  *
+ * @deprecated
+ *

Use a proper @deprecated, like:

 * @deprecated in Drupal 8.x, will be removed before Drupal 9.0.
 *   Use ......

Not sure if we should replace module_load_include with include_once.
I know about "loadInclude()" but it only works with enabled modules.
So, My query is, should we replace include_once with loadInclude() ? Or is there some thought behind using include_once ?

Attached the patch though.

Some nits

1. +++ b/core/includes/install.core.inc
   @@ -1739,7 +1739,7 @@ function install_import_translations(&$install_state) {
   -    // for contrib modules happens in install_import_translations_remaining.
   +    // for contrib modules happens in install_finish_translations.
   

   This seems out of scope.

2. +++ b/core/modules/locale/locale.module
   @@ -359,12 +359,13 @@ function locale_cron() {
   +<<<<<<< HEAD
   

   Left over from reroll?

For the above 2 issues pointed.
#1 - I have removed the comment.
#2 - Thats my bad, i should have reviewed the patch better.

@vaibhavjain,

If you can supply an interdiff that would make reviewing the patch a lot easier.

@legolasbo,
Yep, I should have added that.
Attached it now though.

@vaibhavjain,

The testbot automatically tests any *.patch files. Next time, please upload interdiffs in the following format: [issuenumber]-[originalcomment]-[newcomment]-interdiff.txt in your case that would have been 697946-38-42-interdiff.txt

I'm leaving this needs review for now, because I haven't had the time to actually apply the patch and go over it.

1. +++ b/core/includes/module.inc
   @@ -118,9 +123,12 @@ function module_load_install($module) {
     *
   - * @return
   + * @return bool
     *   The name of the included file, if successful; FALSE otherwise.
     *
   

   This should be @return string|bool

   https://www.drupal.org/coding-standards/docs#types

2. +++ b/core/lib/Drupal/Core/Form/FormState.php
   @@ -47,12 +47,11 @@ class FormState implements FormStateInterface {
       *   - files: An optional array defining include files that need to be loaded
   -   *     for building the form. Each array entry may be the path to a file or
   -   *     another array containing values for the parameters 'type', 'module' and
   -   *     'name' as needed by module_load_include(). The files listed here are
   -   *     automatically loaded by \Drupal::formBuilder()->getCache(). By default
   -   *     the current menu router item's 'file' definition is added, if any. Use
   -   *     self::loadInclude() to add include files from a form constructor.
   +   *     for building the form. Each array entry may be the path to a file. The
   +   *     files listed here are automatically loaded by
   +   *     \Drupal::formBuilder()->getCache(). By default the current menu router
   +   *     item's 'file' definition is added, if any. Use self::loadInclude() to
   +   *     add include files from a form constructor.
       *   - form_id: Identification of the primary form being constructed and
   

   The original docs here mention sub-arrays. This info should be carried forward if it's still relevant. I couldn't find any discussion on this issue about it, so I don't know. I'm not familiar enough with how the FormState class works to know if the change is better.

Hey Mile23,
Not sure what is to be done for #2.
I am attaching the patch with #1 rectified.

Interdiff.

Updating status.

#48 rerolled. Hope it is correct.

THanks @pguillard that is a perfect reroll.

There are a couple more references in tests and comments that could be replaced as well.

core/includes/update.inc:238
core/modules/locale/src/Tests/LocaleUpdateDevelopmentReleaseTest.php:23
core/modules/system/src/Tests/Update/DbUpdatesTrait.php:56

Also, maybe a stupid question but why are we using include_once instead of the thing we are recommending to use which is \Drupal::moduleHandler()->loadInclude()?

And last, please don't use include_once as a function, it's a statement and there should be no parenthesis, this is a nitpick but we haven't used it like that in core yet so probably shouldn't start in this patch, the parens are superfluous.

Thanks @joelpittet.

Here is the new patch applying your suggestions from #54.

I guess you are right concerning \Drupal::moduleHandler()->loadInclude(), but in doubt, I didn't replace it.

Thanks:) Hopefully someone else who worked on this or knows can confirm that.

Right, at the moment module_load_include() is deprecated in favor of ModuleHandler::loadInclude(). We could decide that since module's include files are on the decline we should deprecate ModuleHandler::loadInclude() as well, but then we should document that on the method.

Some further thoughts:
- Seeing that this is a 20KB patch it seems there is still a lot of usage of this, so maybe we should postpone this to when we have even less include files at some point in the D9 cycle.
- At least in classes (specifically: non-test classes) using ModuleHandler::loadInclude() is better than both module_load_include() and include_once drupal_get_path(...) ... because the ModuleHandler can be injected making the code testable. WizardPluginBase is one example I found in the patch (there aren't many, though). So those should in my opinion be either left alone or converted to $this->moduleHandler->loadInclude() where $this->moduleHandler has to be injected somehow.
- The changes to generate-d6-content.sh and generate-d7-content.sh I *think* could be fine in this particular case, but those scripts are supposed to run on a D6/D7 code base, so as a general rule of thumb those should be left alone. Just as a general note.

We should not introduce more usages of drupal_get_path() since we want to remove that function as well. See #2347783: Deprecate drupal_get_path() and drupal_get_filename() and replace with ExtensionList::getPath() and ExtensionList::getPathname().
With the completion of the extension system in #2208429: Extension System, Part III: ExtensionList, ModuleExtensionList and ProfileExtensionList, there is a cleaner way to include files without using drupal_get_path() that's being introduced in that patch.

I replaced include_once drupal_get_path() with ModuleHandler::loadInclude()

What do you think ?

Another try..

Patch re-rolled

Was the comment in install_import_translations() accidentally changed in the last re-roll?

Now that #2208429: Extension System, Part III: ExtensionList, ModuleExtensionList and ProfileExtensionList is in, there should be more impetus to finish off this clean-up.

Please review the patch

Changes were added for #77 patch:
1.Parameters were fixed in '\Drupal::moduleHandler()->loadInclude()'.
2.Removed changes for examples in module.inc because they for module_load_include(). They are correct.

I doubt about my changes in
/core/modules/system/src/Tests/Update/DbUpdatesTrait.php
and /core/modules/system/tests/src/Functional/Update/DbUpdatesTrait.php
are they correct or not? check please

+++ b/core/includes/module.inc
@@ -117,15 +117,20 @@ function module_load_install($module) {
+ * @deprecated in Drupal 8.x, will be removed before Drupal 9.0.

Needs a CR and a link to a CR in the deprecation message.

Added CR https://www.drupal.org/node/2948698
and some more changes to try fixing part bugs in the tests

Unpublished CR cos not not commited, polished it

+++ b/core/modules/locale/locale.batch.inc
@@ -179,7 +179,7 @@
-        \Drupal::moduleHandler()->loadInclude('locale' , 'inc', 'bulk.inc');
+        \Drupal::moduleHandler()->loadInclude('locale' , 'inc', 'locale.bulk');

@@ -214,7 +214,7 @@
-  \Drupal::moduleHandler()->loadInclude('locale', 'inc', 'bulk.inc');
+  \Drupal::moduleHandler()->loadInclude('locale', 'inc', 'locale.bulk');

this kind of changes makes #507396: Change module_load_include() parameters obsolete
Looks better to introduce new "shiny" interface and stop using 3 params

Reroll of #83

Fixed re-roll, mostly all places in core using long format for the method - so normalized in patch

It still needs to fix loading .install files

+++ b/core/modules/views/src/Plugin/views/wizard/WizardPluginBase.php
@@ -621,7 +621,7 @@ protected function rowStyleOptions() {
-    module_load_include('inc', 'views_ui', 'admin');
+    \Drupal::moduleHandler()->loadInclude('views_ui', 'inc', 'admin');

@@ -926,7 +926,7 @@ protected function defaultDisplayFiltersUser(array $form, FormStateInterface $fo
-      module_load_include('inc', 'views_ui', 'admin');
+      \Drupal::moduleHandler()->loadInclude('views_ui', 'inc', 'admin');

Follow-up to remove because no views_ui.admin.inc exists #3017299: Remove unused module_load_include() in view_ui module

Closed #507396: Change module_load_include() parameters as really obsolete

Meanwhile it becomes clearer to me that loading include file and .install should be separate methods
Loading install happens for installed and uninstalled modules
So for not installed modules it can be executed also in context of all scanned extensions, not only installed

Trying to decouple

Looks we already have collision with execution of hooks for uninstalled modules
- to get schema - probably should be made only for installed modules, OTOH at install time not clear
- to get module requirements - exactly before install

It missing test for deprecation

+++ b/core/includes/module.inc
@@ -115,27 +122,23 @@ function module_load_install($module) {
+  @trigger_error("module_load_include() is deprecated in Drupal 8.7.0 and will be removed before Drupal 9.0.0. Instead,
+  you should use \Drupal::moduleHandler()->loadInclude(). See https://www.drupal.org/project/drupal/issues/697946", E_USER_DEPRECATED);

this should be single line

• Patch rerolled
• Some new occurrences found
• Single line from #96 fixed

Rerolled.
Fixed CS issue with a deprecation message.
Added the legacy test.

Fix broken test and few nits, I think it's fine to have module handler from \Drupal - scope issue #3015650: Deprecate tracker_page() and allow tracker.pages.inc to be removed in Drupal 9

One more clean-up

Finally green!

At least this needs an issue summary update. The issue summary is very old and it's goal initially apparently was to remove this function completely. the patch now is "just" a 1:1 deprecation in favor of a method on a service.

1. +++ b/core/lib/Drupal/Core/Form/FormState.php
   @@ -46,10 +46,11 @@ class FormState implements FormStateInterface {
   -   *     'name' as needed by module_load_include(). The files listed here are
   -   *     automatically loaded by \Drupal::formBuilder()->getCache(). By default
   -   *     the current menu router item's 'file' definition is added, if any. Use
   -   *     self::loadInclude() to add include files from a form constructor.
   +   *     'name' as needed by \Drupal::moduleHandler()->loadInclude(). The files
   +   *     listed here are automatically loaded by
   +   *     \Drupal::formBuilder()->getCache(). By default the current menu router
   +   *     item's 'file' definition is added, if any. Use self::loadInclude() to
   +   *     add include files from a form constructor.
       *   - form_id: Identification of the primary form being constructed and
       *     processed.
   

   not really our problem here, but this description is very outdated. There is no such thing as a menu router item file definition in Drupal 8.

   And I guess that this files thing almost unused too in D8. This was necessary when form callbacks were hidden away in optional include files and needed to be loaded, but now that should mostly/completely be on classes which are autoloaded.

   From a quick glance, there seems to be exactly one thing in D8 that still relies on this, and that's the theme-settings.php feature to customize the theme settings for. If there is no issue yet then lets open one to deprecate this?

2. +++ b/core/lib/Drupal/Core/Form/FormStateInterface.php
   @@ -67,7 +67,7 @@ public function setCompleteForm(array &$complete_form);
       *
   -   * @see module_load_include()
   +   * @see \Drupal::moduleHandler()->loadInclude()
       */
   

   can we really resolve this as a link? I doubt that, lets use interface::method?

3. +++ b/core/modules/locale/tests/src/Kernel/LocaleDeprecationsTest.php
   @@ -23,7 +23,7 @@ class LocaleDeprecationsTest extends KernelTestBase {
      public function testLocaleTranslationManualStatusDeprecation() {
   -    module_load_include('pages.inc', 'locale');
   +    \Drupal::moduleHandler()->loadInclude('locale', 'inc', 'locale.pages');
        $this->assertNotNull(\locale_translation_manual_status());
      }
   

   not surprised that most of this patch is locale/content_translation, way too many include files in those modules.

4. +++ b/core/modules/node/node.module
   @@ -766,13 +766,13 @@ function node_user_cancel($edit, UserInterface $account, $method) {
            ->condition('uid', $account->id())
            ->execute();
   -      module_load_include('inc', 'node', 'node.admin');
   +      \Drupal::moduleHandler()->loadInclude('node', 'inc', 'node.admin');
          node_mass_update($nids, ['status' => 0], NULL, TRUE);
          break;
   

   yeah, this issue is pretty good at pointing out weird left-over functions :) That function and its helper things are the last things left in node.admin.inc.

5. +++ b/core/modules/tracker/src/Controller/TrackerPage.php
   @@ -13,7 +13,7 @@ class TrackerPage extends ControllerBase {
       */
      public function getContent() {
   -    module_load_include('inc', 'tracker', 'tracker.pages');
   +    \Drupal::moduleHandler()->loadInclude('tracker', 'inc', 'tracker.pages');
        return tracker_page();
      }
   

   That reminds me of #3015650: Deprecate tracker_page() and allow tracker.pages.inc to be removed in Drupal 9, which needs some fixes *hint* :)

6. +++ b/core/modules/views/src/Plugin/views/wizard/WizardPluginBase.php
   @@ -622,7 +622,7 @@ protected function rowStyleOptions() {
      protected function buildFilters(&$form, FormStateInterface $form_state) {
   -    module_load_include('inc', 'views_ui', 'admin');
   +    \Drupal::moduleHandler()->loadInclude('views_ui', 'inc', 'admin');
    
        $bundles = $this->bundleInfoService->getBundleInfo($this->entityTypeId);
        // If the current base table support bundles and has more than one (like user).
   @@ -927,7 +927,7 @@ protected function defaultDisplayFiltersUser(array $form, FormStateInterface $fo
   
   @@ -927,7 +927,7 @@ protected function defaultDisplayFiltersUser(array $form, FormStateInterface $fo
          // Figure out the table where $bundle_key lives. It may not be the same as
          // the base table for the view; the taxonomy vocabulary machine_name, for
          // example, is stored in taxonomy_vocabulary, not taxonomy_term_data.
   -      module_load_include('inc', 'views_ui', 'admin');
   +      \Drupal::moduleHandler()->loadInclude('views_ui', 'inc', 'admin');
          $fields = Views::viewsDataHelper()->fetchFields($this->base_table, 'filter');
   

   this file doesn't exist anymore, so that load include is bogus and only doesn't fail because we ignore missing files (should we?)

   And the description above is also bogus, there is no taxonomy_vocabulary table in D8 and the machine name now is the ID ;)

   Yes, kinda off topic, but if we do a separate issue then it's weird to update those calls here?

About #107.6 this file still here https://git.drupalcode.org/project/drupal/blob/HEAD/core/modules/views_u...
I recall few issues about its functions but probably it needs some meta to get rid of it

- #3017299: Remove unused module_load_include() in view_ui module
- #3035340: Deprecate core/modules/views_ui/admin.inc

It blocks #3012523: Convert the update.inc file functions to a class

Rerolled after #3015650: Deprecate tracker_page() and allow tracker.pages.inc to be removed in Drupal 9

Also fixed #107.2

For #107.6 there's issues already - this include still there and used
#3017299: Remove unused module_load_include() in view_ui module
#3035340: Deprecate core/modules/views_ui/admin.inc

Just reroll with version update in the deprecation messages.

Removed module_load_include() from Drupal, kindly review.

1. +++ b/core/includes/module.inc
   @@ -29,9 +36,9 @@ function module_load_install($module) {
   - *   module_load_include('inc', 'node', 'content_types');
   + *   \Drupal::moduleHandler()->loadInclude('content_types', 'inc', 'node');
   

   This change is wrong. Parameters in the wrong order.

2. +++ b/core/includes/module.inc
   @@ -47,27 +54,22 @@ function module_load_install($module) {
   + @trigger_error("module_load_include() is deprecated in drupal:9.1.0 and is removed from drupal:10.0.0. Instead, you should use \Drupal::moduleHandler()->loadInclude(). See https://www.drupal.org/project/drupal/issues/697946", E_USER_DEPRECATED);
   

   When a patch adds a @trigger_error(), there also has to be testing for the deprecated message. See: #3038513: Move drupal_generate_test_ua() into the test system with the code:

   +++ b/core/tests/Drupal/FunctionalTests/UserAgentLegacyTest.php
   @@ -0,0 +1,32 @@
   +  /**
   +   * Test drupal_valid_test_ua() and drupal_generate_test_ua() functions.
   +   *
   +   * @expectedDeprecation drupal_valid_test_ua() is deprecated in drupal:9.1.0 and is removed from drupal:10.0.0. Use \Drupal\Core\Test\UserAgent::validate(). See https://www.drupal.org/node/3044173
   +   * @expectedDeprecation drupal_generate_test_ua() is deprecated in drupal:9.1.0 and is removed from drupal:10.0.0. Use \Drupal\Core\Test\UserAgent::generate(). See https://www.drupal.org/node/3044173
   +   */
   +  public function testDrupalTestUa() {
   +    $test_prefix = drupal_generate_test_ua(drupal_valid_test_ua());
   +    $this->assertNotEmpty($test_prefix);
   +    $this->assertNotFalse(drupal_valid_test_ua($test_prefix));
   +  }
   

3. +++ b/core/lib/Drupal/Core/Form/FormState.php
   @@ -46,7 +46,7 @@ class FormState implements FormStateInterface {
   +   *     'name' as needed by \Drupal::moduleHandler()->loadInclude(). The files listed here are
   
   +++ b/core/lib/Drupal/Core/Form/FormStateInterface.php
   @@ -49,10 +49,10 @@ public function setCompleteForm(array &$complete_form);
   +   * Use this function instead of \Drupal::moduleHandler()->loadInclude() from inside a form
   ...
   +   * \Drupal::moduleHandler()->loadInclude() directly, this method makes sure the include file is
   

   The comment is now longer then the maximum of 80 characters. See: https://www.drupal.org/docs/develop/standards/coding-standards#linelength

Updated the the IS and the CR.

Hi @daffie,

Could you please provide full test cast I am trying to run but not luck attach the screenshot.

+++ b/core/tests/Drupal/FunctionalTests/UserAgentLegacyTest.php
@@ -0,0 +1,32 @@
+  /**
+   * Test drupal_valid_test_ua() and drupal_generate_test_ua() functions.
+   *
+   * @expectedDeprecation drupal_valid_test_ua() is deprecated in drupal:9.1.0 and is removed from drupal:10.0.0. Use \Drupal\Core\Test\UserAgent::validate(). See https://www.drupal.org/node/3044173
+   * @expectedDeprecation drupal_generate_test_ua() is deprecated in drupal:9.1.0 and is removed from drupal:10.0.0. Use \Drupal\Core\Test\UserAgent::generate(). See https://www.drupal.org/node/3044173
+   */
+  public function testDrupalTestUa() {
+    $test_prefix = drupal_generate_test_ua(drupal_valid_test_ua());
+    $this->assertNotEmpty($test_prefix);
+    $this->assertNotFalse(drupal_valid_test_ua($test_prefix));
+  }

@shaktik: I think you have a syntax error in your code. You shall have to fix that first before the test will run. Also that issue has not landed. I only added it as an example of how to add testing for @trigger_error().

Sorry @shaktik I couldn't work out what changes you were making from #112. Next time can you please add an interdiff as outlined in https://www.drupal.org/documentation/git/interdiff

I've basically taken #112 and simplified the legacy test and got it working.

Thanks, @kim.pepper for quick help/fix, will be taken care of next time.

All the code changes look good.
All my points are addressed.
Only after installing the patch and doing a code base search for "module_load_include(", I find the the function is still being used in "core/scripts/generate-d6-content.sh" and "core/scripts/generate-d7-content.sh". I should have checked for this earlier.
The patch is almost RTBC for me.

Aren't those files for d6 and d7 where the function exists?

Aren't those files for d6 and d7 where the function exists?

No, the function module_load_include() lives in: core/includes/module.inc.

@kim.pepper: You are right. The scripts are not to be used with a Drupal 9 site, but should be used on a Drupal 6/7 site. As stated in the docblocks of both scripts.

With that the patch is for me RTBC.

+++ b/core/includes/module.inc
@@ -20,7 +20,14 @@ function module_load_install($module) {
-  return module_load_include('install', $module);
+  if (function_exists('drupal_get_path')) {
+    $file = DRUPAL_ROOT . '/' . drupal_get_path('module', $module) . "/$module.install";
+    if (is_file($file)) {
+      require_once $file;
+      return $file;
+    }
+  }
+  return FALSE;

Interesting; why aren't we using the service here? This could use an inline comment if nothing else to explain why we're not using the service.

I think we can use a service here.

So I have added a patch that uses a service to get module path.

If we can't use service here then we can ignore this patch.

What @xjm wants to know is why we do not use the service here. What would happen when we do:

+++ b/core/includes/module.inc
@@ -20,7 +20,14 @@ function module_load_install($module) {
-  return module_load_include('install', $module);
+  return \Drupal::moduleHandler()->loadInclude($module, 'install');

instead of what the patch does in comment #127:

+++ b/core/includes/module.inc
@@ -20,7 +20,14 @@ function module_load_install($module) {
-  return module_load_include('install', $module);
+  if (function_exists('drupal_get_path')) {
+    $file = DRUPAL_ROOT . '/' . drupal_get_path('module', $module) . "/$module.install";
+    if (is_file($file)) {
+      require_once $file;
+      return $file;
+    }
+  }
+  return FALSE;

When it works with the service then use the service and when it does not, then add a comment with why it does not work in the patch.

Updated patch to add

+++ b/core/includes/module.inc
@@ -20,7 +20,14 @@ function module_load_install($module) {
-  return module_load_include('install', $module);
+  return \Drupal::moduleHandler()->loadInclude($module, 'install');

instead of

+++ b/core/includes/module.inc
@@ -20,7 +20,14 @@ function module_load_install($module) {
-  return module_load_include('install', $module);
+  if (function_exists('drupal_get_path')) {
+    $file = DRUPAL_ROOT . '/' . drupal_get_path('module', $module) . "/$module.install";
+    if (is_file($file)) {
+      require_once $file;
+      return $file;
+    }
+  }
+  return FALSE;

--- Checking the result

In #92

Loading include file and .install should be separate methods
Loading install happens for installed and uninstalled modules
So for not installed modules, it can be executed also in the context of all scanned extensions, not only installed

that why

+++ b/core/includes/module.inc
@@ -20,7 +20,14 @@ function module_load_install($module) {
-  return module_load_include('install', $module);
+  if (function_exists('drupal_get_path')) {
+    $file = DRUPAL_ROOT . '/' . drupal_get_path('module', $module) . "/$module.install";
+    if (is_file($file)) {
+      require_once $file;
+      return $file;
+    }
+  }
+  return FALSE;

this change is included instead of use of service.

Updated patch to include the inline comment.

1. +++ b/core/includes/module.inc
   @@ -47,27 +57,22 @@ function module_load_install($module) {
     * @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.
   

   This @todo has not been solved. This seems like an API change, if previously we could load arbitrary files from disabled modules, but now we cannot (this seems to be why module_load_install() needs to be fixed).

2. +++ b/core/modules/locale/locale.module
   @@ -453,8 +453,8 @@ function locale_system_remove($components) {
   -    \Drupal::moduleHandler()->loadInclude('locale', 'bulk.inc');
   ...
   +    \Drupal::moduleHandler()->loadInclude('locale', 'inc', 'locale.bulk');
   

   This doesn't really need to change, is there a preferred style here?

@longwave
Regarding 2 points, It's good to have as like

// Load node.admin.inc from the node module.
$this->loadInclude('node', 'inc', 'node.admin');

As given name = NULL as per definition, and mostly followed the same way
public function loadInclude($module, $type, $name = NULL)

#139.1: Tried to addess this item. Currently allowed to load only install files for disabled modules (which situation may require to load any other file from the disabled module?). Reviews are welcome.

#139.2: I prefer to not use workaround of the ways how the function designed to be used. So +1 to keep this change.

Add a number of comments on the MR, therefore changing the status to NW.

The deprecation need to be changed from 9.1.0 to 9.2.0.

Updated IS and CR. Also addressed review comments.

Almost RTBC.

@voleger: Thank you for working on the MR.

There are still unresolved threads. Back to needs work.

I bet fixing #2010380: Deprecate module_load_install() and replace it with ModuleHandler::loadInstall could minimize changes

Yes, I agree. I think it would be cleaner to keep the two changes separate. This issue was just about module_load_include() until about 6 month ago when it was expanded, and it now conflicts with #2010380: Deprecate module_load_install() and replace it with ModuleHandler::loadInstall.

The big issue with module_load_include() IMO is that it was always used for loading code from both installed modules and uninstalled modules. That ability needs to be preserved, as contrib may depend on it. But doing a special-case if ($type === 'install') like this patch does is really not a good thing to do as far as I'm concerned, since it special-cases module_load_include() just for the implementation of module_load_install(). Instead, if the implementation of module_load_install() needs different code, that should be in module_load_install().

This brings up a question of loading includes (no matter which kind it's)

There's 2 hard coded includes - .install and tokens, views, templates and "broader" module_load_include() it's all sounds like one method to module handler

Opened one more MR with which contains module_load_include() related changes only, left previous MR as is.

Rebased and updated MR 1222

Addressed review comments

All the code changes look good to me.
The method is deprecated in D10 and removed in D11.
I have updated the CR.
For me it is RTBC.

I don't think this is RTBC. The concern from #139 has not been addressed:

+++ b/core/includes/module.inc
@@ -47,27 +57,22 @@ function module_load_install($module) {
  * @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.

This @todo has not been solved. This seems like an API change, if previously we could load arbitrary files from disabled modules, but now we cannot (this seems to be why module_load_install() needs to be fixed).

I also expressed this same concern in #155:

The big issue with module_load_include() IMO is that it was always used for loading code from both installed modules and uninstalled modules. That ability needs to be preserved, as contrib may depend on it.

Also, the CR needs to be updated.

Addressed #163

You are right @TR (in comment #163), that still needs to be fixed. Only now it does not work and this issue does not fix that. To me this is out of scope for this issue. I have created a followup issue for it: #3256205: \Drupal::moduleHandler()->loadInclude() should also work with disabled and/or uninstalled modules.. Changing the status back to needs work for moving the @todo to the docblock of \Drupal::moduleHandler()->loadInclude().

@voleger: If we are going to address #163 in this issue can we then also add testing for it. To make sure it fixes the problem.

#166 Sure here the test only patch

Added test into MR

So, we have proof that #139 was addressed. Needs review.

The problem from comment #163 has been addressed and has testing for it.
Back to RTBC.

The module handler loading themes should be a big red flag here. That's a massive change and not in a good direction. It points to an incorrect architecture or the method being used in the wrong places. I think it is a mistake to allow the module handler to work with anything that is not installed and anything that is not a module.

I'd updated the test which shows that the module handler only deals with the module extensions and includes the file from the uninstalled extension only when the developer provides confirmation that is intended action.

The testbot is not happy.

It is happy now, but in that case, @alexpott is right - module_load_include is used not only by module extensions.

Just one little nitpick. After that it is RTBC for me. I was not allowed to make the change myself in the MR.

I still think that the module handler has no business processing uninstalled modules or profiles. I think that code that works with uninstalled extensions should have to go an extra mile in order to include the code and we should not use our generic module code includer to do that.

I.e. all the additional logic to load uninstalled stuff should go in module_load_install() and not in ModuleHandler. And then when we refactor module_load_install() we come up with some other install-only service that does this.

I still think that the module handler has no business processing uninstalled modules or profiles

I am perfectly happy with this conclusion EXCEPT that it represents a reversal of what we did in D8, D7 etc. In previous versions of core, it was clearly INTENTIONAL that the module handler dealt with uninstalled module includes, and it was documented that way in the code comments.

Thus, if this behavior is going to be changed, then the change should ALSO be intentional and the change should be made explicit in the CR.

I made both of these points above: Support for uninstalled module includes was unknowingly dropped in the above patches, it was not discussed, and the patch authors clearly weren't aware of it. This change was also left out of the CR.

@TR the module handler doesn't exist in D7 and in D8 where it was introduce we intentionally made it only handle installed modules. This change is making it handle uninstalled modules and that's exactly what I think it should not do.

Dude, this issue has been open for 12 years. Instead of picking on the wording, how about interpreting my post in context? I and others were far more precise above and in the related issues. Seems like a massive waste of time to re-explain everything in every subsequent comment.

As mentioned above, this @todo about module_load_include() has been in the Drupal API documentation for a very long time:

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

This is precisely what the current issue is about - replacing module_load_include() with a method in the module_handler service. From this @todo, it's clear that the module_handler service was meant to preserve the functionality of module_load_include().

@longwave from #139 (May 2020):

This @todo has not been solved. This seems like an API change, if previously we could load arbitrary files from disabled modules, but now we cannot (this seems to be why module_load_install() needs to be fixed).

It's a simple ask - if the behavior is going to be changed it should be intentional and documented. There are far too many core issues where this doesn't happen and we have regressions that never get fixed. Let's do a better job this time.

We're adding unnecessary complexity and functionality to ModuleHandler for the benefit of only module_load_install(). Let's not do that. There's no reason to allow the ModuleHandler to include files from modules that are not installed. The @todo is not correct. We shouldn't allow the ModuleHandler to load code from uninstalled modules because that code has no business being loaded unless we're in very very specific situations which are all controlled via module_load_install().

It is a massive improvement to Drupal's architecture that the only files the can be included from uninstalled modules are .install files and we just don't support including files from uninstalled things.

The patch attached is what I think we should do.

Hmm... actually the previous patches have actually broken the legacy code's ability to include code from uninstalled modules... so I've fixed that by reverting the unnecessary changes to module_load_include(). I've also simplified module_load_install() to the mere necessary.

Missed a bit.

Amusingly even module_load_install() doesn't need to deal with uninstalled code so we can follow this up with deprecating module_load_install() with moduleHandler::loadInclude() with no changes to it.