Problem/Motivation

module_implements is missing a module enabled check before calling hooks for passed modules

Proposed resolution

add a module->exists check

Remaining tasks

none

User interface changes

none

API changes

none

CommentFileSizeAuthor
#22 2244681-22.patch2.98 KBmarthinal
#22 interdiff-2244681-16-22.txt617 bytesmarthinal
#16 module.implements.16.patch2.92 KBsun
#12 module.implements.12.patch2.92 KBsun
#5 2244681-invoke-hooks-on-enabled-modules-5.patch627 bytesneclimdul
#2 2244681-invoke-hooks-on-enabled-modules.patch689 bytesneclimdul
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Title: module_implements needs module enabled check before implementing any hooks » ModuleHandler::implementsHook() does not check whether a module is enabled
Component: base system » extension system
Related issues: +#2237831: Allow module services to specify hooks
neclimdul’s picture

neclimdul
He/Him
CreditAttribution: neclimdul commented
Issue tags: +Quickfix
FileSize
2244681-invoke-hooks-on-enabled-modules.patch689 bytes

patch

neclimdul’s picture

neclimdul
He/Him
CreditAttribution: neclimdul commented
Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 2: 2244681-invoke-hooks-on-enabled-modules.patch, failed testing.

neclimdul’s picture

neclimdul
He/Him
CreditAttribution: neclimdul commented
Status: Needs work » Needs review
FileSize
2244681-invoke-hooks-on-enabled-modules-5.patch627 bytes

um.... ok... re-roll.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Issue tags: +Needs tests

Writing a test should be rather straightforward.

+++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php
@@ -329,6 +329,12 @@ public function resetImplementations() {
+      return false;

should be FALSE

Status: Needs review » Needs work

The last submitted patch, 5: 2244681-invoke-hooks-on-enabled-modules-5.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

"Awesome" — those test failures are the same as in #2237831: Allow module services to specify hooks, which means that this fix will resolve the failures of that issue. ;-)

neclimdul’s picture

neclimdul
He/Him
CreditAttribution: neclimdul commented

I'm not sure what the theme failures are, but several of the failures are because hook_requirements is being called on a module that is not "enabled." More to come.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Issue tags: -Quickfix

Hm. Yeah, this is a lot more hairy than I thought — I completely forgot about hook_requirements() and similar straw callbacks that are invoked for extensions that are not installed (yet).

Given the analysis below, it looks like we can (temporarily) fix this by making drupal_check_module() not call into ModuleHandler::invoke(), but instead, make it use the identical code as in drupal_check_profile().

Installer: drupal_check_profile():

  $info = install_profile_info($profile);
  $requirements = array();
  foreach ($info['dependencies'] as $module) {
    module_load_install($module);
    $function = $module . '_requirements';
    if (function_exists($function)) {
      $requirements = array_merge($requirements, $function('install'));
    }
  }

Update: update_check_requirements():

  $requirements = \Drupal::moduleHandler()->invokeAll('requirements', array('update'));

update.php operates on installed/enabled modules only, so this should be fine.

Modules UI: ModulesListForm::buildModuleList():

    // Invoke hook_requirements('install'). If failures are detected, make
    // sure the dependent modules aren't installed either.
    foreach (array_keys($modules['install']) as $module) {
      if (!drupal_check_module($module)) {
        unset($modules['install'][$module]);

→ drupal_check_module():

  module_load_install($module);
  // Check requirements
  $requirements = \Drupal::moduleHandler()->invoke($module, 'requirements', array('install'));

Site note: I also wasn't aware that only the Modules UI page checks requirements, but the API does not. Ouch. (separate issue)

neclimdul’s picture

neclimdul
He/Him
CreditAttribution: neclimdul commented

Yes I agree, this is a special case and should be treated as such. Another possibility might be to make adding and removing modules to the internal list of active modules a little easier. In general that sounds like it would be useful but it sounds more problematic then manually running it for solving this case.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
module.implements.12.patch2.92 KB
  1. Cleaned up coding style.
  2. Fixed drupal_check_module() cannot use ModuleHandler::invoke(), because the module is not enabled yet.
  3. Added test.

Status: Needs review » Needs work

The last submitted patch, 12: module.implements.12.patch, failed testing.

lostkangaroo’s picture

lostkangaroo CreditAttribution: lostkangaroo commented
Status: Needs work » Needs review

12: module.implements.12.patch queued for re-testing.

Passes locally

Status: Needs review » Needs work

The last submitted patch, 12: module.implements.12.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs work » Needs review
FileSize
module.implements.16.patch2.92 KB

Let's see whether merging 8.x helps.

Status: Needs review » Needs work

The last submitted patch, 16: module.implements.16.patch, failed testing.

marthinal’s picture

marthinal CreditAttribution: marthinal commented

The problem in the test view is that we have a ".theme" where we are calling the hooks (views_pre_render and views_post_render) and the theme isn't into the list of modules. I'm thinking a way to avoid this... but yes, locally it fails too.

marthinal’s picture

marthinal CreditAttribution: marthinal commented
Status: Needs work » Needs review

16: module.implements.16.patch queued for re-testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented 
+++ b/core/modules/system/lib/Drupal/system/Tests/Extension/ModuleHandlerTest.php
@@ -0,0 +1,52 @@
+class ModuleHandlerTest extends DrupalUnitTestBase {

Is there a reason why we don't use a unit test here? Basically just install and uninstall needs all that crazyness.

The problem in the test view is that we have a ".theme" where we are calling the hooks (views_pre_render and views_post_render) and the theme isn't into the list of modules. I'm thinking a way to avoid this... but yes, locally it fails too.

Let's fix that and call the functions directly.

Status: Needs review » Needs work

The last submitted patch, 16: module.implements.16.patch, failed testing.

marthinal’s picture

marthinal CreditAttribution: marthinal commented
Status: Needs work » Needs review
FileSize
interdiff-2244681-16-22.txt617 bytes
2244681-22.patch2.98 KB
Let's fix that and call the functions directly.

@dawehner I'm not sure what you mean. Anyway, is it possible to check what hooks are called from a theme checking the enabled themes? Maybe is it possible to have the enabled themes into the config yaml?

The patch should fix a couple fails, in this case for the default theme only. And I would like to verify if breaks something... let's try

Status: Needs review » Needs work

The last submitted patch, 22: 2244681-22.patch, failed testing.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented
Status: Needs work » Needs review

22: 2244681-22.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 22: 2244681-22.patch, failed testing.

neclimdul’s picture

neclimdul
He/Him
CreditAttribution: neclimdul commented

I'm sort of glad this failed. Its failing because DUTB doesn't exist anymore. There's another problem though, this functionality is going to break a good part of the phpunit tests for ModuleHandler because we made an implicit dependency on Config through a call to globals and most of the tests are testing methods that call into implements. Don't have an answer for a good way to solve that just yet.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

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

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should 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.

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

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Version: 8.6.x-dev » 8.7.x-dev
Related issues: +#697946: Properly deprecate module_load_include() and move it into \Drupal::moduleHandler() service

Now extension lists patches are in so hooks for install/requirements could be properly decoupled

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Status: Needs work » Postponed (maintainer needs more info)
Issue tags: +Needs reroll, +Bug Smash Initiative

Is this still an issue?

If so definitely needs rerolling

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented
Status: Postponed (maintainer needs more info) » Closed (outdated)

Since there hasn't been a follow up going to close out for now.

If still an issue please reopen updating issue summary.