Contrib module's hook_requirements are not called when installing

You say only if they are NOT installed into /modules (aka where core modules are installed). I assume all contrib modules, because they could be in:
* /modules
* /sites/all/modules
* /sites/domain.com/modules
* /profiles/profilename/modules

I'm standardizing on all profile modules in /profiles/profilename/modules -- will test this with a new profile.

this certainly looks OK. i can't easily test right now since my HEAD checkout is broken. I am going to RTBC and hope that Steven will review. Hook requirements is his handiwork, IIRC.

Here's a patch that fixes both modules and profiles.

Unfortunately, I found that my patch doesn't fix profiles. module_invoke() is not available during Drupal installation. The boot process would have to be changed or module_invoke() would have to be moved to a different file. I will be unavailable for Drupal work until mid-August, so I hope someone else will find time to fix this.

Marked http://drupal.org/node/143746 and http://drupal.org/node/189312 as duplicates of this.

subscribing

Shouldn't this issue be flagged as critical? It is more or less an API thing (after this would be fixed, hook_requirements() would behave differently for contrib modules) and changing an API after a x.0 release is generally not wise.

For the sake of discussion I'd also like to copy over the patch from duplicate thread http://drupal.org/node/189312 because it is very simple: only one line change. A remaining problem is however (cited from here):

Actually, I tested this and it breaks install. Common.inc, which has that function, is not included early in the install process.

saxofan: http://drupal.org/node/176003 solves the module_invoke() availability as well. At least it installs system module first and then does a full bootstrap, so all other modules are installed in a full bootstrap.

Guys, http://drupal.org/node/176003 was committed, so module_invoke() is available after system module was installed. I'd advise retesting this patch with the new 6.x-dev.

I tested the patch from #15 with drupal-6.x-dev and hook_requirements works now as expected/desired for default drupal installation, custom profile drupal installation with custom modules, and post install module installation.

With the patch from #10, installation is still broken

After entering the database name, user and password I get a bunch of errors like

Warning: Table 'tmp01.system' doesn't exist query: SELECT filename FROM system WHERE name = 'block' AND type = 'module' in /home/slippens/public_html/drupal-HEAD/includes/database.mysqli.inc on line 167

These errors messages are generated before table creation apparently, because the tables are available in the database.

The problem with the approach in the patch from #10 is that it uses module_load_install(), which calls module_load_include(), which calls drupal_get_path(), which calls drupal_get_filename(), which queries the system table, which is not created yet during the check for the system module.

I looked into the patch in #15 deeply. In drupal_get_install_files() it basically trades a call to file_scan_directory() on the default ./modules directory only to a call to a drupal_system_listing() which scans through all possible module folders (install profile, original modules, site specific modules). So in principle, it looks quite good. However:

- I'd replace "$module.install$" with $module .'.install$' to be clear on what different dollars mean here.

- The patch changes the 0 min depth (file_scan_directory's default) to 1 (drupal_system_listing's default). This means that install files for modules in the root of the modules folders (not in a subdir under modules) will not be found. I don't think this is to be supported, as with .install and .info files, throwing module files into the root modules folder without a subdir is ugly at best. However, it is important to note that this change is made here.

Let's test (ie. I am not marking needs work for that simple string change, which I can do on commit anyway :)

- "$module.install$" indeed looks weird, $module .'.install$' looks slightly less weird ;)

- considering the depth issue: I already commented on that in the duplicate thread (http://drupal.org/node/189312#comment-622982):

I think the min_depth=0 in the original drupal_get_install_files() implementation is a leftover from the time (drupal 4.x) that modules did not reside in their own directory. But since Drupal 5 that's not the case anymore and it is considered good style to put each module in its own directory. That's why min_depth of drupal_system_listing() defaults to 1, I guess. drupal_system_listing() was introduced in Drupal 5, while file_scan_directory() goes back to D4.6.

saxofan: please provide detailed test instructions (eg. modules I can test this with) so if nobody else tests, at least I can do. Without much interest, this is not going to b fixed otherwise in Drupal 6.

Since hook_requirements('install') is broken for contrib modules, there don't exist real modules to test it with.
I use the simple foo module (for in sites/all/modules) and the fooprofile profile (for in profiles/) in attachment to test.

Test case 1: Installation of the foo module should fail because it returns a REQUIREMENT_ERROR requirement entry.
Test case 2: Drupal installation with the fooprofile profile should fail because it requires this foo module.

the patch of #15 solves this

But for the moment I have troubles installing drupal HEAD with default profile.
After entering database name, user and password I get:

Warning: Table 'tmp01.system' doesn't exist query: SELECT filename FROM system WHERE name = 'comment' AND type = 'module' in .../public_html/drupal-HEAD/includes/database.mysqli.inc on line 167

Installation of the fooprofile profile succeeds however.
Is install of drupal HEAD broken atm?

I reopened the issue which broke the installer as you noted: http://drupal.org/node/194310#comment-662133

You mean no contrib module has a install requirements checking because it was broken until this patch? Fair enough :|
Anyway, thanks for the test cases, I'll retest as soon as we fix the installer.

mailhandler has requirements check on install. it is not ported to D6 yet, but the D5 version might be good enough for testing here ... i agree with critical priority for this.

thanks for fixing the installer, it works again for me

You mean no contrib module has a install requirements checking because it was broken until this patch?

Maybe there are modules that define install requirements, but the requirement check won't fire if the module is installed outside the core modules directory.

For a real test case, from my original bug report (duplicate of this thread):

As contributer to the image CAPTCHA module, I'd like to add a pre-install check if the GD library is availiable with FreeType support.

I managed to install mailhandler without PHP IMAP support on D5, from sites/all/modules - there's a big red warning on admin/reports once you do this, but it did get through the requirements check (which I didn't notice, because I didn't know it had the check before installing).

Hm, tested this. Before patch, the foo profile run nicely, after patch, it provided with a requirements error. The same goes for the contrib module when using the default profile (which allowed me to install). So all works nice as far as this patch goes, which signs that I should commit it.

BUT there is a little problem here. The requirements error message is not designed to accommodate for these kind of requirements in contrib modules, and comes up with bugos looking error messages. It is certainly formatted as "currently using $product_name $version", which results in "Foo requires library bar (Currently using Foo foo wants bar)" in this example. Let's fix how this message is constructed, so we can provide actually readable error messages to users instead of this mess. Otherwise enabling contribs to do these kinds of requirements checks ends up in a messed UI.

I also noticed this, but I thought the fixing the UI strings was not the critical part of this issue.

Moreover, the messed up message "Foo requires library bar (Currently using Foo foo wants bar)" is partly because in the foo test module, I did not follow the hook_requirements documentation closely enough. The requirement entry in foo.install should be more like

  $requirements['bar'] = array(
    'title' => 'bar',
    'value' => '1.3',
    'severity' => REQUIREMENT_ERROR,
    'description' => 'Foo requires library bar',
  );

Then the message is:

Foo requires library bar (Currently using bar 1.3)

Nevertheless, it seems that the requirement error message

t('Currently using !item !version', array('!item' => $requirement['title'], '!version' => $requirement['value']))

is a bit too specifically tailored towards library-versions-requirements.

Even in core, hook_requirements() is used for other things than library version checking.
For example, the cron entry on the status report is done with a requirement:

    if (is_numeric($cron_last)) {
      $requirements['cron']['value'] = $t('Last run !time ago', array('!time' => format_interval(time() - $cron_last)));
    }
    else {
      $requirements['cron'] = array(
        ....
        'value' => $t('Never run'),
      );
    }

while the docs state that the 'value' entry should be something like a version number.

Well, it needs to be fixed here as well. Do you have ideas around this?

Foo requires library bar (Currently using bar 1.3)
Looks good to me.

It appears that if the 'value' entry is not set, the error message is limited to the 'description' entry in install.php:

      if (isset($requirement['severity']) && $requirement['severity'] == REQUIREMENT_ERROR) {
        $message = $requirement['description'];
        if ($requirement['value']) {
          $message .= ' ('. st('Currently using !item !version', array('!item' => $requirement['title'], '!version' => $requirement['value'])) .')';
        }
        drupal_set_message($message, 'error');

In includes/install.inc however, this "feature" is not available:

        if (isset($requirement['severity']) && $requirement['severity'] == REQUIREMENT_ERROR) {
          drupal_set_message($requirement['description'] .' ('. t('Currently using !item !version', array('!item' => $requirement['title'], '!version' => $requirement['value'])) .')', 'error');
        }

Attached patch adds this feature to includes/install.inc (also fix from #15 is included)

This is simple solution with small code changes to solve the issue with non-library-version-requirements.
However the API documentation for hook_requirements() needs some love to reveal this feature (among others).

sorry, wrong patch in #32
this one should be better ;)

As a more structural note:
hook_requirements($phase) is both used for defining installation requirements ($phase='install') as for status reporting ($phase='runtime'). Since both are somewhat related, one could argued that they could be handled by the same hook.

The problem is that from the function name "hook_requirements" it is not obvious that this hook also can be used for status reporting. Also the hook_requirements documentation appears to be written around install requirements and with the status reporting as a minor afterthought.

In short: the problem discussed in #28 and #29 is mainly a documentation issue, and does not require core code changes I think, except for the "feature" explained in #32. It's probably too late in the Drupal6 code cycle to come up with a better function name for hook_requirements or split the hook in two (hook_requirements and hook_status).

After this issue is fixed, I'm willing to take a look at the hook_requirements documentation.

In attachment: new version of patch from #32 with tip from #20 (replace "$module.install$" with $module .'.install$').

Sorry for the patch reiteration overload,
but after some more thorough testing I found that the patch from #34 generated a "undefined index notice" due to the new E_ALL compliance mode.

This new patch addresses this.

I also fixed it in install.php, but there it seems not necessary since E_ALL compliance is not required there (yet?)

Great, this looks much better, thanks. It does not hurt to make install.php E_ALL compliant, even if it does not run under E_ALL even in Drupal 6-dev. Committed.

The same problem exists in 5.7. Could the solution be backported ?