Problem/Motivation

#2208429: Extension System, Part III: ExtensionList, ModuleExtensionList and ProfileExtensionList makes it possible to deprecate system_rebuild_module_data() and replace it with calls to \Drupal::service('extension.list.module')->reset()

Proposed resolution

Replace all usages and deprecate system_rebuild_module_data() and trigger a silenced error. Also, when replacing the usages we should ensure that we don't unnecessarily rebuild the module list.

Remaining tasks

User interface changes

None

API changes

None

Data model changes

None

CommentFileSizeAuthor
#44 2926068-44.patch68.37 KBalmaudoh
#44 interdiff.txt1.08 KBalmaudoh
#42 interdiff.txt1.95 KBalmaudoh
#42 2926068-42.patch68.29 KBalmaudoh
#40 2926068-40.patch68.75 KBalmaudoh
#40 interdiff.txt2.74 KBalmaudoh
#37 interdiff.txt1.88 KBalmaudoh
#37 2926068-37.patch68.47 KBalmaudoh
#35 2926068-35.patch68.31 KBalmaudoh
#35 interdiff.txt1.11 KBalmaudoh
#34 2926068-34.patch68.31 KBalmaudoh
#34 interdiff.txt943 bytesalmaudoh
#31 2926068-31.patch67.58 KBalmaudoh
#31 interdiff.txt2.62 KBalmaudoh
#27 2926068-27.patch68.37 KBalmaudoh
#27 interdiff.txt1.38 KBalmaudoh
#26 2926068-26.patch67.25 KBalmaudoh
#26 interdiff.txt21.3 KBalmaudoh
#24 2926068-24.patch58.76 KBalmaudoh
#24 interdiff.txt12.58 KBalmaudoh
#21 2926068-21.patch51.71 KBalmaudoh
#21 interdiff.txt6.76 KBalmaudoh
#19 2926068-19.patch47.47 KBOleksiy
#19 interdiff_15-19.txt16.41 KBOleksiy
#16 2926068-15.patch33.49 KBalmaudoh
#16 interdiff.txt643 bytesalmaudoh
#13 2926068-13.patch33.49 KBalmaudoh
#2 2926068.patch48.54 KByo30
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

alexpott created an issue. See original summary.

yo30’s picture

FileSize
48.54 KB

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

almaudoh’s picture

Issue summary: View changes

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

dawehner’s picture

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

alexpott’s picture

Issue summary: View changes

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

jibran’s picture

dawehner’s picture

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.

Mile23’s picture

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

dawehner’s picture

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

That sounds like a good idea.

larowlan’s picture

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

alexpott’s picture

I think it is important that marking the code deprecated and using @trigger_error() happen in same issue. This is because marking the code without triggering the error is just a hopeful deprecation and does not solve the problem of technical debt. I suggest that part 1 is trying to replace all the usages in core without do that.

almaudoh’s picture

Status: Active » Needs review
FileSize
33.49 KB

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)

Status: Needs review » Needs work

The last submitted patch, 13: 2926068-13.patch, failed testing. View results

alexpott’s picture

+++ b/core/includes/install.core.inc
@@ -1501,7 +1501,7 @@ function install_profile_modules(&$install_state) {
-  $files = system_rebuild_module_data();
+  $files = $this->container->get('extension.list.module')->getList();

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

almaudoh’s picture

Status: Needs work » Needs review
FileSize
643 bytes
33.49 KB

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.

Status: Needs review » Needs work

The last submitted patch, 16: 2926068-15.patch, failed testing. View results

dawehner’s picture

  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?

Oleksiy’s picture

Status: Needs work » Needs review
FileSize
16.41 KB
47.47 KB

Updated according to comment #18.

Status: Needs review » Needs work

The last submitted patch, 19: 2926068-19.patch, failed testing. View results

almaudoh’s picture

Status: Needs work » Needs review
FileSize
6.76 KB
51.71 KB

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.

Status: Needs review » Needs work

The last submitted patch, 21: 2926068-21.patch, failed testing. View results

dawehner’s picture

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

almaudoh’s picture

Status: Needs work » Needs review
FileSize
12.58 KB
58.76 KB

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.

alexpott’s picture

Status: Needs review » Needs work
  1. +++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
    @@ -180,8 +188,10 @@ class ConfigImporter {
    +   * @param \Drupal\Core\Extension\ModuleExtensionList $extension_list_module
    +   *   The module extension list
    ...
    +  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) {
    
    @@ -191,6 +201,7 @@ public function __construct(StorageComparerInterface $storage_comparer, EventDis
    +    $this->moduleExtensionList = $extension_list_module;
    

    I think for BC it'd be nice if the argument here was optional. This class is not a service and things like Drush will have to change and it'll be complicated to support both Drupal 8.5.x and 8.6.x. Therefore I recommend making it optional and triggering a deprecation notice when its NULL.

  2. +++ b/core/lib/Drupal/Core/EventSubscriber/ConfigImportSubscriber.php
    @@ -303,7 +303,7 @@ protected function getThemeData() {
    +      $this->moduleData = \Drupal::service('extension.list.module')->getList();
    

    Let's inject this. As an event subscriber it's internal and constructed via the container.

  3. +++ b/core/modules/config/tests/src/Functional/ConfigImportAllTest.php
    @@ -93,8 +93,7 @@ public function testInstallUninstall() {
    -    system_list_reset();
    -    $all_modules = system_rebuild_module_data();
    +    $all_modules = \Drupal::service('extension.list.module')->reset()->getList();
    

    Let's add a comment as to why reset().

  4. +++ b/core/modules/system/tests/src/Kernel/Extension/ModuleHandlerTest.php
    @@ -25,14 +25,7 @@ class ModuleHandlerTest extends KernelTestBase {
    -    // @todo ModuleInstaller calls system_rebuild_module_data which is part of
    -    //   system.module, see https://www.drupal.org/node/2208429.
    -    include_once $this->root . '/core/modules/system/system.module';
    

    yay

  5. +++ b/core/modules/update/src/UpdateManager.php
    @@ -82,8 +90,10 @@ class UpdateManager implements UpdateManagerInterface {
    +   * @param \Drupal\Core\Extension\ModuleExtensionList $extension_list_module
    +   *   The module extension list.
    

    If this is optional needs (optional) and |null. Not sure why this is optional because UpdateManager should be constructed via the service container. So also why is update.services.yml not changing.

  6. +++ b/core/modules/user/src/PermissionHandler.php
    @@ -229,13 +229,4 @@ protected function getModuleNames() {
    -  /**
    -   * Wraps system_rebuild_module_data()
    -   *
    -   * @return \Drupal\Core\Extension\Extension[]
    -   */
    -  protected function systemRebuildModuleData() {
    -    return system_rebuild_module_data();
    -  }
    
    +++ b/core/modules/user/tests/src/Unit/PermissionHandlerTest.php
    @@ -377,25 +368,6 @@ protected function assertPermissions(array $actual_permissions) {
    -class TestPermissionHandler extends PermissionHandler {
    -
    -  /**
    -   * Test module data.
    -   *
    -   * @var array
    -   */
    -  protected $systemModuleData;
    -
    -  protected function systemRebuildModuleData() {
    -    return $this->systemModuleData;
    -  }
    -
    -  public function setSystemRebuildModuleData(array $extensions) {
    -    $this->systemModuleData = $extensions;
    -  }
    -
    -}
    

    yay

  7. +++ b/core/tests/Drupal/KernelTests/Core/Bootstrap/GetFilenameTest.php
    @@ -30,10 +30,8 @@ public function register(ContainerBuilder $container) {
    -    // Rebuild system.module.files state data.
    -    // @todo Remove as part of https://www.drupal.org/node/2186491
    -    drupal_static_reset('system_rebuild_module_data');
    -    system_rebuild_module_data();
    +    // Rebuild module list state data.
    +    \Drupal::service('extension.list.module')->reset();
    
    +++ b/core/tests/Drupal/KernelTests/Core/Config/ConfigDependencyTest.php
    @@ -219,8 +219,7 @@ public function testConfigEntityUninstall() {
    -    // @todo Remove as part of https://www.drupal.org/node/2186491
    -    system_rebuild_module_data();
    +    $this->container->get('extension.list.module')->reset();
    
    @@ -361,8 +360,7 @@ public function testConfigEntityUninstallComplex(array $entity_id_suffixes) {
    -    // @todo Remove as part of https://www.drupal.org/node/2186491
    -    system_rebuild_module_data();
    +    $this->container->get('extension.list.module')->reset();
    
    @@ -478,8 +476,7 @@ public function testConfigEntityUninstallThirdParty() {
    -    // @todo Remove as part of https://www.drupal.org/node/2186491
    -    system_rebuild_module_data();
    +    $this->container->get('extension.list.module')->reset();
    

    Should we still be looking to remove these calls?

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

    There is likely to be a few other KernelTests where this is the case. Let's file a followup to review all tests where system is the only dependency. There are 52 tests where this is the case. For example the same code can be removed from \Drupal\Tests\field\Kernel\FieldDefinitionIntegrityTest but not from \Drupal\KernelTests\Core\Theme\ThemeInstallerTest - so working through that in a follow-up makes sense to me.

almaudoh’s picture

Status: Needs work » Needs review
FileSize
21.3 KB
67.25 KB

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

almaudoh’s picture

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.

Status: Needs review » Needs work

The last submitted patch, 27: 2926068-27.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

alexpott’s picture

@almaudoh yeah those are unlikely to be able to be removed... it's the ModuleInstaller after all :) Can you re-upload the last green patch so the latest patch is the one to review? Thanks.

alexpott’s picture

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

We should test if it is NULL and if so trigger a deprecation error as per https://www.drupal.org/core/deprecation

almaudoh’s picture

Title: Deprecate system_rebuild_module_data() » Remove usages of system_rebuild_module_data() in core
Status: Needs work » Needs review
FileSize
2.62 KB
67.58 KB

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()

alexpott’s picture

Status: Needs review » Needs work

@almaudoh why wouldn't we deprecate it in this issue? This issue removes all the usages and without triggering the deprecation error we can't be sure that another issue won't sneak a usage back in.

alexpott’s picture

I think #8 to #12 has been made irrelevant by your excellent work. The patch is not that big or hard so multiple issue seems unnecessary.

almaudoh’s picture

Status: Needs work » Needs review
FileSize
943 bytes
68.31 KB

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

almaudoh’s picture

Title: Remove usages of system_rebuild_module_data() in core » Deprecate system_rebuild_module_data() and remove usages in core
FileSize
1.11 KB
68.31 KB

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

alexpott’s picture

Status: Needs review » Needs work

Nearly there... one minor thing re the deprecation in ConfigImporter.

  1. +++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
    @@ -192,6 +192,9 @@ class ConfigImporter {
        *   (optional) The module extension list
    

    It's not optional - the optionality is deprecated. The param description should mention this.

  2. +++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
    @@ -192,6 +192,9 @@ 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.', E_USER_DEPRECATED);
    

    We should point to a change record. I think we're going to need a new one for this issue. Very simple one that just says ConfigImporter will have a new required parameter in Drupal 9 and for now it is optional.

almaudoh’s picture

Status: Needs work » Needs review
FileSize
68.47 KB
1.88 KB

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

alexpott’s picture

Status: Needs review » Needs work

@almaudoh +1 on removing optional from the param description since the correct usage is not optional.

+++ b/core/modules/user/src/PermissionHandler.php
@@ -229,13 +229,4 @@ protected function getModuleNames() {
-  /**
-   * Wraps system_rebuild_module_data()
-   *
-   * @return \Drupal\Core\Extension\Extension[]
-   */
-  protected function systemRebuildModuleData() {
-    return system_rebuild_module_data();
-  }

This unfortunately is a BC break even though it is unused. Its usage was removed in #2281989: Add a fast and simple way to get module name from the module handler which was committed before 8.0.0 so we should have removed it then and as much as overriding the PermissionHandler is unlikely it possible so something might extend it and rely on this method. So we should not remove it. We should leave it in and deprecate and do return \Drupal::service('extension.list.module')->reset()->getList(); and yep I think we should do a reset because we have no idea of the usage.

Berdir’s picture

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

almaudoh’s picture

Status: Needs work » Needs review
FileSize
2.74 KB
68.75 KB

Did #38 and #39.

alexpott’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
@@ -180,8 +188,14 @@ class ConfigImporter {
+    if ($extension_list_module === NULL) {
+      @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);
+    }

@@ -191,6 +205,10 @@ public function __construct(StorageComparerInterface $storage_comparer, EventDis
+    $this->moduleExtensionList = $extension_list_module;
+    if ($this->moduleExtensionList === NULL) {
+      $this->moduleExtensionList = \Drupal::service('extension.list.module');
+    }

No need to the two ifs... normally this is done like this:

    if ($extension_list_module === NULL) {
      @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);
      $this->moduleExtensionList = \Drupal::service('extension.list.module');
    }
    else {
      $this->moduleExtensionList = $extension_list_module;
    }
almaudoh’s picture

Status: Needs work » Needs review
FileSize
68.29 KB
1.95 KB

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

Mile23’s picture

Status: Needs review » Needs work
Issue tags: +Needs followup, +@deprecated
+++ 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.

almaudoh’s picture

Status: Needs work » Needs review
Issue tags: -Needs followup
FileSize
1.08 KB
68.37 KB
almaudoh’s picture