In D8, implementsHook calls $this->loadInclude($module, 'inc', $module . '.' . $hook_info[$hook]['group']); which happily tries to do $this->moduleList[$module] without any check in either method whether the $module is enabled.

In D7, it's even more funny:

  1. module_hook
  2. module_load_include
  3. drupal_get_path
  4. drupal_get_filename which reads {system} regardless of status.
  5. The include file of the disabled module is loaded and happily processed.

This is rarely hit because module_implements will iterate the module list and call implementsHook only with module names that happen to exist (in D7 the module_load_include call is inlined into module_implements).

But. In D8, field_views_data will iterate the existing fields and call the modules from there for views data and if you happen to have a disabled module which provided a field type, then boom! you get a notice. If you have a stray modulename.type in your DRUPAL_ROOT that gets loaded. Yes, that's an idiot edge case, but hey, it's me.

I have hit this during the update path where disabled modules are frequent which, alas, makes this critical. Of course, fixing ModuleHandler is not hard at all. I already patched this, still need to write a test, will post a patch soon.

I will file a separate issue for Views to get rid of (pseudohook)_field_views_data because it's not necessary, there's an alter already and we can change that to an alter of array types.

CommentFileSizeAuthor
#29 1948702-29.patch1.45 KBDavid_Rothstein
#29 1948702-29-TESTS-ONLY.patch849 bytesDavid_Rothstein
#22 1948702-22.patch633 bytesDavid_Rothstein
#14 1948702_14.patch716 bytesjibran
#8 Selection_106.png41.18 KBchx
#3 1948702_2-do-not-test.patch716 byteschx
#1 1948702_1_test_only.patch1.21 KBchx
#1 1948702_1.patch1.98 KBchx
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

chx’s picture

chx CreditAttribution: chx commented
Status: Active » Needs review
FileSize
1948702_1.patch1.98 KB
1948702_1_test_only.patch1.21 KB

Asserts are for wusses :P The test only patch beautifully will fail (it does locally at least) because of the notice while the patched version passes.

msonnabaum’s picture

msonnabaum CreditAttribution: msonnabaum commented
Status: Needs review » Reviewed & tested by the community

Looks quite good to me. Love the test.

chx’s picture

chx CreditAttribution: chx commented
FileSize
1948702_2-do-not-test.patch716 bytes

For D7, this is just normal IMO but will need profiling before it can get in although I believe that the cost is really, really small but it does get called a lot of times and adds a userspace function call. In D8, the performance cost is negligible because there is no userspace function call added, just a simple isset.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented 
+++ b/core/tests/Drupal/Tests/Core/Extension/ModuleHandlerUnitTest.phpundefined
@@ -0,0 +1,42 @@
+    $this->moduleHandler = new ModuleHandler;

Should be new ModuleHandler() I think.

Wouldn't care much but the test-only patch did not fail so it might need a change anyway...

Also tried locally and same result. Does it have something to do with PHPUnit?

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented

#1 That awkward moment when test only patch is green.

amateescu’s picture

amateescu CreditAttribution: amateescu commented

The same fix is RTBC here for over a week.. #1940954: ModuleHandler::loadInclude() can trigger notices

yched’s picture

yched CreditAttribution: yched commented

yup, #1940954: ModuleHandler::loadInclude() can trigger notices was the exact same problem with field_views_data.

chx’s picture

chx CreditAttribution: chx commented
FileSize
Selection_106.png41.18 KB

The pass is due to faulty phpunit integration and has nothing to do with my most excellent and really complicated test :P The linked issue is a duplicate that has no test.

chx’s picture

chx CreditAttribution: chx commented

#1949266: phpunit errors are silently eaten

yched’s picture

yched CreditAttribution: yched commented

Similar / tangentially related (ok, shameless plug) : #1941000: update_module_enable() does not update ModuleHandler::$moduleList

Dries’s picture

Dries CreditAttribution: Dries commented
Version: 8.x-dev » 7.x-dev

Committed to 8.x. Thanks chx. Moving to 7.x.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Patch (to be ported)
chx’s picture

chx CreditAttribution: chx commented
Priority: Critical » Normal
Status: Patch (to be ported) » Needs review
Issue tags: +needs profiling

Patch is in #3

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented
FileSize
1948702_14.patch716 bytes

Re-uploading #3 for testing.

The last submitted patch, 1948702_14.patch, failed testing.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented
Status: Needs work » Needs review

#14: 1948702_14.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 1948702_14.patch, failed testing.

Anonymous’s picture

(not verified) CreditAttribution: commented
Issue summary: View changes

Updated issue summary.

  • Dries committed 6122e84 on 8.3.x 
    Issue #1948702 by chx: module_hook() is broken for disabled modules and...

  • Dries committed 6122e84 on 8.3.x 
    Issue #1948702 by chx: module_hook() is broken for disabled modules and...
David_Rothstein’s picture

David_Rothstein CreditAttribution: David_Rothstein as a volunteer commented
Related issues: +#2773959: "The following [module/theme] has moved within the file system:" error after downloading new module or theme

The patch changes module_load_include(), but shouldn't it change module_hook() instead? I don't think there's anything that says you can't load a file from a disabled module if you really need to.

For module_hook(), however, the function documentation says it returns "TRUE if the module is both installed and enabled, and the hook is implemented in that module", but the code is sometimes ignoring the first half of that sentence. So it's definitely a bug. Still seems a bit scary to fix (code out there could be relying on the current behavior), but it's probably worth fixing. This is also leading to subtle bugs in some contrib modules - see the related issue.

David_Rothstein’s picture

David_Rothstein CreditAttribution: David_Rothstein as a volunteer commented
Related issues: +#2828605: feeds_system_info_alter() can triggers "The following module has moved within the file system"

That issue is related also, but here is the one I actually meant to add.

David_Rothstein’s picture

David_Rothstein CreditAttribution: David_Rothstein as a volunteer commented
Status: Needs work » Needs review
FileSize
1948702-22.patch633 bytes

Here's a patch that fixes it in module_hook().

I guess ideally we could use a test for it too.

David_Rothstein’s picture

David_Rothstein CreditAttribution: David_Rothstein as a volunteer commented
Issue tags: +Drupal 7.60 target
David_Rothstein’s picture

David_Rothstein CreditAttribution: David_Rothstein as a volunteer commented
Related issues: +#2838075: search_api_system_info_alter() can trigger "module is missing from the file system" in Drush-based module installation
Neograph734’s picture

Neograph734
he/him
Location Netherlands
CreditAttribution: Neograph734 as a volunteer commented
Status: Needs review » Reviewed & tested by the community

Marking this RTBC as well. I see no harm in this implementation and the missing module errors during installation are gone.

The last submitted patch, 3: 1948702_2-do-not-test.patch, failed testing.

stefan.r’s picture

stefan.r CreditAttribution: stefan.r commented
Issue tags: -Drupal 7.60 target +Drupal bugfix target
David_Rothstein’s picture

David_Rothstein CreditAttribution: David_Rothstein as a volunteer commented

I put this as Drupal 7.60 target because I was worried it could break things. For example, any code that calls module_hook() (or module_implements(), which in turn calls module_hook()) in an update function and then gets run on a disabled module.... Invoking hooks in update functions is generally a bad idea but in practice it can happen, especially indirectly.

In any case this could still use a test.

David_Rothstein’s picture

David_Rothstein CreditAttribution: David_Rothstein as a volunteer commented
Status: Reviewed & tested by the community » Needs review
FileSize
1948702-29-TESTS-ONLY.patch849 bytes
1948702-29.patch1.45 KB

I guess something like this should work as a test.

The last submitted patch, 29: 1948702-29-TESTS-ONLY.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 29: 1948702-29.patch, failed testing.

stefan.r’s picture

stefan.r CreditAttribution: stefan.r commented
Issue tags: -Drupal bugfix target +Drupal 7.60 target

  • Dries committed 6122e84 on 8.4.x 
    Issue #1948702 by chx: module_hook() is broken for disabled modules and...

  • Dries committed 6122e84 on 8.4.x 
    Issue #1948702 by chx: module_hook() is broken for disabled modules and...
joseph.olstad’s picture

joseph.olstad
Primary language French
CreditAttribution: joseph.olstad commented
Issue tags: -Drupal 7.60 target +Drupal 7.70 target

Bumping to 7.70. This didn't make it into 7.60

poker10’s picture

poker10 CreditAttribution: poker10 at ActivIT s.r.o. commented
Issue tags: -Drupal 7.70 target

Just for the reference, the test in #29 is failing because the module_test module was enabled before this test code is hit and the file with module_test_test_hook is therefore already included in PHP. So in module_hook() we will not reach this desired part (which was modified by the patch):

if (isset($hook_info[$hook]['group']) && module_exists($module)) {

Instead the first IF condition will return TRUE (because the file with the function is already loaded from before):

if (function_exists($function)) {
  return TRUE;
}

So either the test needs to be fixed, or the testModuleInvoke() function needs to be moved more upper.

I think it will be better to create a separate issue for D7 fix, because this still needs work.