Deprecate system_rebuild_module_data() and remove usages in core

Here is my patch, I rerolled the parent of this issue, for not getting conflicts.
See, and review it, Thanks!

@yo30: You should wait until the parent issue is committed, then you can make these changes.

If you do want to continue with this issue, then you have to find all usages of system_rebuild_module_data() and replace them with \Drupal::service('extension.list.module')->reset() or \Drupal::service('extension.list.module')->reset()->list() depending on if the return value is assigned to another variable or not. See the changelog at https://drupal.org/node/2709919.

You shouldn't reattach patch from the parent issue here.

To be honest we should also think about, whether there are places which don't need to reset the list.

This seems duplicate of #2873051: Replace usages of system_rebuild_module_data with the extension.list.module service

Just doing the reset() thing is IMHO not the right idea. There should be not many places which should actually do that.

If someone want to work on it, I'm happy to assist. Please ping me on slack if you need information/guidance.

Maybe limit scope to just performing the deprecation part and not the removal and trigger error part?

Maybe limit scope to just performing the deprecation part and not the removal and trigger error part?

That sounds like a good idea.

Happy with a two stage process - lets just make sure we have a follow up for the second half

Let's start with replacement of system_rebuild_module_data() with ModuleExtensionList::getList(). Trying on a best effort to ::reset() only where necessary (based on code context and comments)

No $this here. Before uploading a patch it might be worth trying a test or an install as you would have caught this.

Fixed that now - copy and paste error. I was actually very sleepy and reaching for the bed at the point I uploaded that patch.

1. +++ b/core/includes/install.core.inc
   @@ -1501,7 +1501,7 @@ function install_profile_modules(&$install_state) {
      $modules = \Drupal::state()->get('install_profile_modules') ?: [];
   -  $files = system_rebuild_module_data();
   +  $files = \Drupal::service('extension.list.module')->getList();
   

   Given this is part of the installer it might need the reset.

2. +++ b/core/includes/update.inc
   +++ b/core/includes/update.inc
   @@ -43,7 +43,7 @@ function update_check_incompatibility($name, $type = 'module') {
   
   @@ -43,7 +43,7 @@ function update_check_incompatibility($name, $type = 'module') {
   -    $modules = system_rebuild_module_data();
   +    $modules = \Drupal::service('extension.list.module')->reset()->getList();
   

   I would have guessed this one wouldn't need it

3. +++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
   @@ -369,7 +369,7 @@ protected function createExtensionChangelist() {
        // Get a list of modules with dependency weights as values.
   -    $module_data = system_rebuild_module_data();
   +    $module_data = \Drupal::service('extension.list.module')->getList();
   
   +++ b/core/lib/Drupal/Core/EventSubscriber/ConfigImportSubscriber.php
   @@ -303,7 +303,7 @@ protected function getThemeData() {
        if (!isset($this->moduleData)) {
   -      $this->moduleData = system_rebuild_module_data();
   +      $this->moduleData = \Drupal::service('extension.list.module')->getList();
   

   Should we inject the service?

4. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
   @@ -329,7 +329,7 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
        // Get all module data so we can find dependencies and sort.
   -    $module_data = system_rebuild_module_data();
   +    $module_data = \Drupal::service('extension.list.module')->getList();
   

   +1 for not resetting this here. We have not changed the overall state of the system

5. +++ b/core/modules/config/tests/src/Functional/ConfigImportAllTest.php
   @@ -94,7 +94,7 @@ public function testInstallUninstall() {
        system_list_reset();
   

   Do we still need the system_list_reset() call?

Updated according to comment #18.

Reverted the ->reset() in #19. Also did constructor injection for some of the classes.

One thing to decide is constructor injection for all the service classes, whether those would be a BC break.

One really nice thing would be to document why we need a ->reset() in all the cases.

Fixed all the test fails.

One thing to decide is constructor injection for all the service classes, whether those would be a BC break.

For this I just added an optional parameter with a getter that can initialize the value with \Drupal::service('extension.list.module') if it wasn't injected. This way the service constructor can stay backward compatible. The only exception is the RequiredModuleUninstallValidator where the constructor a non-optional constructor argument is added - this class is part of the extension system.

One really nice thing would be to document why we need a ->reset() in all the cases.

Done this too. As a matter of fact, we only needed to call ->reset() in five places.

Thanks for the review:
1. Done
2. Injected
3. Since I couldn't see a good reason, I removed the ->reset() and the test still passed. Great! So I went a step further and checked all the other tests that had reset() and found that it wasn't needed in many more places. Removed all of them. Tests still pass. I guess this is primarily because ModuleHandler::install() already does a reset and that is why most of these calls were made. +1 for better structured code.
4. \o/
5. Fixed. No more optional.
6. \o/
7. Removed all of them and more of the same.
8. Created #2943436: Review all tests where system module is the only dependency

This may be overreaching, but a couple more ->reset()s in ModuleInstaller that I don't think are needed. Tested locally and passed. Let's see what testbot says.

Reverted #27 and added a @trigger_error for #30

Also retitling to reflect actual scope per #12. I will open a follow-up to actually deprecate and @trigger_error for system_rebuild_module_data()

Good point @alexpott :) Thanks. TBH, I was hoping you would say "let's just deprecate it in this issue". So done.

Sorry, I mixed up the url in the deprecation message. Fixed it now to point to the change record.

#36 done. Change record is at https://www.drupal.org/node/2943918
#36.1: Since the optional usage is not recommended, I have removed (optional) and instead explained it in the @param description. I hope that is ok.

+++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
@@ -191,6 +205,7 @@ public function __construct(StorageComparerInterface $storage_comparer, EventDis
     $this->themeHandler = $theme_handler;
     $this->stringTranslation = $string_translation;
+    $this->moduleExtensionList = $extension_list_module;

The common approach is to initialize "optional" dependencies directly in the constructor, the we can avoid the lazy-loading protected method.

Did #38 and #39.

Ah, well. Didn't really think that mattered that much...

+++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
@@ -180,8 +188,18 @@ class ConfigImporter {
+   * @param \Drupal\Core\Extension\ModuleExtensionList|null $extension_list_module
+   *   The module extension list. This is left optional for BC reasons, but the
+   *   optional usage is deprecated and will become required in Drupal 9.0.0.

Is there a follow-up against D9 to make this change? We should make one and add a @todo. Also link the follow-up from the change record.

Created follow up #2947083: Make ModuleExtensionList argument required in \Drupal\Core\Config\ConfigImporter and added @todo and to change record.

Reroll

A new usage of ConfigImporter was added using the signature this patch has deprecated.

1. +++ b/core/lib/Drupal/Core/Extension/module.api.php
   @@ -131,7 +131,7 @@ function hook_module_implements_alter(&$implementations, $hook) {
   - * This hook is invoked in _system_rebuild_module_data() and in
   + * This hook is invoked in \Drupal\Core\Extension\ExtensionList::getList() and in
   

   line length overflows 80 chars

2. +++ b/core/modules/system/system.module
   @@ -1046,8 +1046,14 @@ function _system_rebuild_module_data() {
   +  @trigger_error("system_rebuild_module_data() is deprecated in Drupal 8.6.0 and will be removed before Drupal 9.0.0. Instead, you should use \\Drupal::service('extension.list.module')->getList(). See https://www.drupal.org/node/2709919", E_USER_DEPRECATED);
   

   Two \\

Rerolled.
Addressed #50.2 Replaced \\ => \
#50.1 - outdated.

Rerolled again.

Sure

Missed use core/modules/system/src/Form/ModulesListForm.php

It looks like parts with cache cleaning needs more detailed review but here's a quick skim

1. +++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
   @@ -180,8 +188,20 @@ class ConfigImporter {
   +      @trigger_error('Invoking the ConfigImporter constructor without the module extension list parameter is deprecated in Drupal 8.6.0 and will no longer be supported in Drupal 9.0.0. The extension list parameter is now required in the ConfigImporter constructor. See https://www.drupal.org/node/2943918', E_USER_DEPRECATED);
   

   update to 8.7.0 needed

2. +++ b/core/lib/Drupal/Core/Extension/RequiredModuleUninstallValidator.php
   @@ -41,11 +49,17 @@ public function validate($module) {
   +    try {
   +      return $this->moduleExtensionList->get($module)->info;
   +    }
   +    catch (\InvalidArgumentException $e) {
   +      // @todo Replace with dedicated exception
   +      // https://www.drupal.org/project/drupal/issues/2940203
   +      return [];
   

   Could use to check that module exists instead of exception

3. +++ b/core/modules/filter/tests/src/Functional/FilterFormTest.php
   @@ -71,8 +71,6 @@ public function testFilterForm() {
   -    // Force rebuild module data.
   -    \Drupal::service('extension.list.module')->reset();
   

   now clear why removed

4. +++ b/core/modules/filter/tests/src/Kernel/FilterAPITest.php
   @@ -488,7 +488,7 @@ public function testDependencyRemoval() {
   -    $module_data = \Drupal::service('extension.list.module')->reset()->getList();
   +    $module_data = \Drupal::service('extension.list.module')->getList();
   

   looks wrong change

5. +++ b/core/modules/user/src/PermissionHandler.php
   @@ -233,9 +233,14 @@ protected function getModuleNames() {
   +   * @deprecated in Drupal 8.6.0 and will be removed before Drupal 9.0.0. This
   +   *   method is no more required because system_rebuild_module_data() has been
   +   *   removed from Drupal 8.6.0. See https://www.drupal.org/node/2709919
   ...
      protected function systemRebuildModuleData() {
   -    return system_rebuild_module_data();
   +    @trigger_error('PermissionHandler::systemRebuildModuleData() is deprecated in Drupal 8.6.0 and will be removed before Drupal 9.0.0. It is no more required because system_rebuild_module_data() has been removed from Drupal 8.6.0. See https://www.drupal.org/node/2709919', E_USER_DEPRECATED);
   

   8.7.0 and looks could be removed

Fix install tests and address comments #61

Fix installer

From interdiff at #62

1. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
   @@ -335,7 +335,7 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
   +    $module_data = \Drupal::service('extension.list.module')->reset()->getList();
   

   I don't think we need the ->reset() here.

2. +++ b/core/lib/Drupal/Core/Extension/RequiredModuleUninstallValidator.php
   @@ -52,14 +54,10 @@ public function validate($module) {
   -    catch (\InvalidArgumentException $e) {
   

   How are we handling the exception now?

I agree that uninstall should not rebuild list - patch does it
#65.2

+++ b/core/lib/Drupal/Core/Extension/RequiredModuleUninstallValidator.php
@@ -52,14 +54,10 @@ public function validate($module) {
+    if ($this->moduleExtensionList->exists($module)) {

the exists() cares about it

Fixed 2 and added note for 3.

A few minor nitpicks, setting to needs work for that, otherwise mostly just some more or less random thoughts...

1. +++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
   @@ -180,8 +188,20 @@ class ConfigImporter {
   +    if ($extension_list_module === NULL) {
   +      @trigger_error('Invoking the ConfigImporter constructor without the module extension list parameter is deprecated in Drupal 8.7.0 and will no longer be supported in Drupal 9.0.0. The extension list parameter is now required in the ConfigImporter constructor. See https://www.drupal.org/node/2943918', E_USER_DEPRECATED);
   +      $this->moduleExtensionList = \Drupal::service('extension.list.module');
   +    }
   +    else {
   +      $this->moduleExtensionList = $extension_list_module;
   

   our deprecation messages for constructor arguments are pretty inconsistent but yay for having one.

2. +++ b/core/lib/Drupal/Core/EventSubscriber/ConfigImportSubscriber.php
   @@ -38,10 +39,13 @@ class ConfigImportSubscriber extends ConfigImportValidateEventSubscriberBase {
       * Constructs the ConfigImportSubscriber.
       *
   +   * @param \Drupal\Core\Extension\ModuleExtensionList $extension_list_module
   +   *   The module extension list.
       * @param \Drupal\Core\Extension\ThemeHandlerInterface $theme_handler
       *   The theme handler.
       */
   -  public function __construct(ThemeHandlerInterface $theme_handler) {
   +  public function __construct(ModuleExtensionList $extension_list_module, ThemeHandlerInterface $theme_handler) {
   +    $this->moduleExtensionList = $extension_list_module;
        $this->themeHandler = $theme_handler;
      }
   

   I suppose we expect that nobody is extending this, which is probably fair, but adding arguments first is still a bit uncommon although I suppose it makes sense for being the more important one?

3. +++ b/core/modules/config/src/Form/ConfigSingleImportForm.php
   @@ -138,8 +146,10 @@ class ConfigSingleImportForm extends ConfirmFormBase {
       */
   -  public function __construct(EntityManagerInterface $entity_manager, StorageInterface $config_storage, RendererInterface $renderer, EventDispatcherInterface $event_dispatcher, ConfigManagerInterface $config_manager, LockBackendInterface $lock, TypedConfigManagerInterface $typed_config, ModuleHandlerInterface $module_handler, ModuleInstallerInterface $module_installer, ThemeHandlerInterface $theme_handler) {
   +  public function __construct(EntityManagerInterface $entity_manager, StorageInterface $config_storage, RendererInterface $renderer, EventDispatcherInterface $event_dispatcher, ConfigManagerInterface $config_manager, LockBackendInterface $lock, TypedConfigManagerInterface $typed_config, ModuleHandlerInterface $module_handler, ModuleInstallerInterface $module_installer, ThemeHandlerInterface $theme_handler, ModuleExtensionList $extension_list_module) {
        $this->entityManager = $entity_manager;
        $this->configStorage = $config_storage;
   

   there seems to be one subclass of this in contrib, but it doesn't override the constructor.

4. +++ b/core/modules/config/src/Form/ConfigSync.php
   @@ -129,8 +137,10 @@ class ConfigSync extends FormBase {
       */
   -  public function __construct(StorageInterface $sync_storage, StorageInterface $active_storage, StorageInterface $snapshot_storage, LockBackendInterface $lock, EventDispatcherInterface $event_dispatcher, ConfigManagerInterface $config_manager, TypedConfigManagerInterface $typed_config, ModuleHandlerInterface $module_handler, ModuleInstallerInterface $module_installer, ThemeHandlerInterface $theme_handler, RendererInterface $renderer) {
   +  public function __construct(StorageInterface $sync_storage, StorageInterface $active_storage, StorageInterface $snapshot_storage, LockBackendInterface $lock, EventDispatcherInterface $event_dispatcher, ConfigManagerInterface $config_manager, TypedConfigManagerInterface $typed_config, ModuleHandlerInterface $module_handler, ModuleInstallerInterface $module_installer, ThemeHandlerInterface $theme_handler, RendererInterface $renderer, ModuleExtensionList $extension_list_module) {
        $this->syncStorage = $sync_storage;
   

   Finding 2-3 subclasses, one in config_distro even overrides create() but they do it in a way that's compatible with changes

5. +++ b/core/modules/field/tests/src/Kernel/FieldDefinitionIntegrityTest.php
   @@ -197,7 +197,7 @@ protected function checkDisplayOption($entity_type_id, $field_id, BaseFieldDefin
       */
      protected function modulesWithSubdirectory($subdirectory) {
   -    $modules = system_rebuild_module_data();
   +    $modules = $this->container->get('extension.list.module')->getList();
        $modules = array_filter($modules, function (Extension $module) use ($subdirectory) {
          // Filter contrib, hidden, already enabled modules and modules in the
   

   I'll never understand understand why everyone is using $this->container in tests...

6. +++ b/core/modules/help/tests/src/Kernel/HelpEmptyPageTest.php
   @@ -32,7 +32,7 @@ public function register(ContainerBuilder $container) {
       */
      public function testEmptyHookHelp() {
   -    $all_modules = system_rebuild_module_data();
   +    $all_modules = $this->container->get('extension.list.module')->getList();;
   

   double semicolon nitpick

7. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceRestTestCoverageTest.php
   @@ -29,7 +29,7 @@ class EntityResourceRestTestCoverageTest extends BrowserTestBase {
    
   -    $all_modules = system_rebuild_module_data();
   +    $all_modules = $this->container->get('extension.list.module')->getList();;
        $stable_core_modules = array_filter($all_modules, function ($module) {
   

   another one

8. +++ b/core/modules/system/src/Form/ModulesUninstallConfirmForm.php
   @@ -66,12 +74,15 @@ class ModulesUninstallConfirmForm extends ConfirmFormBase {
       *   The configuration manager.
       * @param \Drupal\Core\Entity\EntityManagerInterface $entity_manager
       *   The entity manager.
   +   * @param \Drupal\Core\Extension\ModuleExtensionList $extension_list_module
   +   *   The module extension list
   

   missing "."

   Somewhat interesting that ModuleExtensionList doesn't have an interface and even has an explicit @internal and documentation about how it's not yet stable, considering that we're adding a ton of injections and type hints on it now...

9. +++ b/core/modules/system/src/Form/ModulesUninstallForm.php
   @@ -78,11 +90,12 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   -    // Get a list of all available modules.
   -    $modules = system_rebuild_module_data();
   -    $uninstallable = array_filter($modules, function ($module) use ($modules) {
   -      return empty($modules[$module->getName()]->info['required']) && $module->status;
   -    });
   +    // Get a list of all available modules that can be uninstalled.
   +    $uninstallable = array_filter($this->moduleExtensionList->getList(),
   +      function ($module) {
   +        return empty($module->info['required']) && $module->status;
   +      }
   +    );
   

   merging this all in one statement makes it tricky to review, especially due to the $modules change, which seems to be fine. The newline *before* the anonymous function call is a bit unusual though?

10. +++ b/core/modules/system/tests/src/Kernel/System/InfoAlterTest.php
    @@ -22,14 +22,14 @@ class InfoAlterTest extends KernelTestBase {
        */
       public function testSystemInfoAlter() {
         \Drupal::state()->set('module_required_test.hook_system_info_alter', TRUE);
    -    $info = system_rebuild_module_data();
    +    $info = \Drupal::service('extension.list.module')->getList();
    

    interestingly, this test for example uses the (preferred by me) Drupal::service(), looks like quite a few people worked on the patch over time, resulting in some inconsistencies like this, injection order and so on.

11. +++ b/core/tests/Drupal/KernelTests/Core/Config/ConfigImportRecreateTest.php
    @@ -50,7 +50,8 @@ protected function setUp() {
       }
    

    we sure have a lot of new ConfigImporter(...) in all these tests, might be worth to have a helper trait or base class for these config tests?

...I'll never understand understand why everyone is using $this->container in tests...

...interestingly, this test for example uses the (preferred by me) Drupal::service(),...

I have had this dilemma myself. So, question - what is the recommended option to use in tests (since I see an equal number of both \Drupal::service('xyz') and $this->container->get('xyz')

This would help in other patches as well.

Usage of services in tests is neverending story #2066993: Use magic methods to sync container property to \Drupal::getContainer in functional tests and #2644256: Replace \Drupal:: with $this->container->get() in test classes of simpletest module

True, but I didn't set it to needs work because of that but to fix those nitpicks.

Re-roll and address #69 5-9

IIRC service() vs container was supposed to use container in kernel tests and service() in other ones
I changed usage to notion used in each specific file to be consistent - so it will be easy to replace when there will be conclusion in one of issues from #71

Fixed failed test

@Berdir it really needs kind of trait for config importer in #3017360: Replace theme handler with theme installer in ConfigImporter

1. +++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
   @@ -180,8 +188,20 @@ class ConfigImporter {
   +  public function __construct(StorageComparerInterface $storage_comparer, EventDispatcherInterface $event_dispatcher, ConfigManagerInterface $config_manager, LockBackendInterface $lock, TypedConfigManagerInterface $typed_config, ModuleHandlerInterface $module_handler, ModuleInstallerInterface $module_installer, ThemeHandlerInterface $theme_handler, TranslationInterface $string_translation, ModuleExtensionList $extension_list_module = NULL) {
   ...
   +      $this->moduleExtensionList = $extension_list_module;
   

   May I ask why we sometimes use extension_list_module and sometimes moduleExtensionList. Is there any reason to not have more symmetry?

2. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
   @@ -83,7 +83,9 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
        if ($enable_dependencies) {
          // Get all module data so we can find dependencies and sort.
   -      $module_data = system_rebuild_module_data();
   +      // The module list needs to be reset so that it can re-scan and include
   

   May I ask whether someone did some benchmarking? I could imagine that this helps a bit with installing an install profile.

3. +++ b/core/tests/Drupal/KernelTests/Core/Extension/ModuleInstallerTest.php
   @@ -15,15 +15,6 @@
   -   * Modules to install.
   -   *
   -   * The System module is required because system_rebuild_module_data() is used.
   -   *
   -   * @var array
   -   */
   -  public static $modules = ['system'];
   

   Nice!

Reroll, #75.1 because argument following service name but variable following class name

Clean-up constructor argument (should be last)

+++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
@@ -180,8 +188,20 @@ class ConfigImporter {
+    if ($extension_list_module === NULL) {
+      @trigger_error('Invoking the ConfigImporter constructor without the module extension list parameter is deprecated in Drupal 8.7.0 and will no longer be supported in Drupal 9.0.0. The extension list parameter is now required in the ConfigImporter constructor. See https://www.drupal.org/node/2943918', E_USER_DEPRECATED);
+      $this->moduleExtensionList = \Drupal::service('extension.list.module');
+    }
+    else {
+      $this->moduleExtensionList = $extension_list_module;
+    }

We could simplify it now to be able to remove only 4 lines instead of removing 4 lines and modifying of 3 lines in Drupal 9 depreciation cleanup.
Let's keep 'if' construction without 'else'.

Patch rerolled and fixes are implemented

Changed version and added test

and comment

And fix typo

All comments were addressed. Looks good.

Fixed #86. Regarding #87, I don't think we should wait for the policy issue as that waits for a coding standards rule. For actual deprecations we should go with the current deprecation policy and this patch fulfills that policy. When a new policy will be in place we, probably, will bulk update all the existing deprecations to use it.

New occurrence in HEAD.

Re-RTBC since committer comment in #86 has been addressed and test is passing.

Random fails.

Failure caused by unrelated #3055648: Frequent random fail in \Drupal\Tests\media_library\FunctionalJavascript\MediaLibraryTest

reroll for 2 hunks

reroll

+++ b/core/modules/update/src/UpdateManager.php
@@ -129,7 +140,7 @@ public function getProjects() {
-        $module_data = system_rebuild_module_data();
+        $module_data = $this->moduleExtensionList->reset()->getList();

@alexpott do you mean to use private getter here?

Here it is

This issues has been once RTBCed, in #91. I'm reviewing and RTBCing for changes requested in #98.