Fatal error if update module is disabled

The exact error message this (on nginx): PHP message: PHP Fatal error: Call to undefined function update_get_projects() in sites/all/modules/contrib/nagios/nagios.module on line 421

So if the update module is disabled, every page that calls update_get_projects() will broke (eg. admin/modules), and the module is also useless.

I think, the correct way of solve this problem is to put a dependency into the .info file. Patch attached.

Corrected the name of the patch, and now includes an update hook to enable the update module.

I'm not sure a dependency is desired here. It might be better to wrap these function calls in module_exists('update') checks? Opinions welcome...

Here's an alternative patch/idea.

From what I can see, we directly call three functions provided by the update module:

• update_get_projects()
• update_get_available()
• update_calculate_project_data()

I've attached a patch that adds a wrapper function for each of these three. Each new wrapper function first checks to see if the update module is enabled. If it is, then the update function is called straight away and the output returned. If the update module is disabled, then the required update files are included before the functions are called.

So update_get_projects() becomes _nagios_update_get_projects(), and so on.

Might be a better approach rather than requiring the update module to be enabled.

Not the most elegant solution, but I've tested the modules page, /nagios, drush nagios-updates and drush nagios-check requirements with the update module both enabled and disabled, and got no errors.

Actually, wrapping those update functions might also allow us to do some automated testing. By passing an extra parameter into the functions, we could return dummy update data simulating specific states.

I can confirm the bug and think the solution by @galooph is the better way to solve.
The patch of #4 is working and solving the bug: RTBC +1.

@galooph #5:
I agree that this is also a good idea.

Works as expected.

Thanks all for the work and the feedback. Committed to the 7.x-1.x branch.

I haven't looked at how the Drupal 8 version of the module currently handles this, but given this patch wasn't applied to the Drupal 7 branch when the Drupal 8 port was done, I'd imagine the bug persists there...?

Can anyone verify what happens when a minimal install profile is used and nagios is enabled? I am getting:

PDOException: SQLSTATE[42S02]: Base table or view not found: 1146 Table 'd7_mQW1fdfH_zd9Q5RaY.cache_update' doesn't exist: DELETE FROM {cache_update} WHERE (cid = :db_condition_placeholder_0) ; Array ( [:db_condition_placeholder_0] => update_project_projects ) in _update_cache_clear() (line 865 of /var/www/html/modules/update/update.module).

using drupal 7.42
nagios 7.x-1.x-dev

My guess is that because the update module is never enabled in the first place, the schema for the update module is never installed, therefor some of these calls are failing,

Hmmm, interesting. I think we need to go back to the module_exists() idea then. We'll look into it asap!

OK, so I see the problem with the Drupal 7 code. Dan's code tries to load the update module even if it's not enabled, but doesn't account for the fact the schema might be absent:
http://cgit.drupalcode.org/nagios/commit/?id=e5cf290

So I guess we can't do that. The nicest thing to do would be to check for the schema and error out with a watchdog message if there's no schema. I guess db_table_exists() ought to do it. I'll have a go... :-)

Attached is a patch that wraps most of the code relating to Drupal update checking in a db_table_exists() call checking for the 'cache_update' table.

One thing that makes me wonder is I looked at nagios_check_requirements() function and it also makes a number of update related calls. On closer inspection, it seems to *rely* on the core update module to build a list of enabled modules to invoke hook_requirements() against. This feels wrong to me because while we may not want to update a module, that doesn't mean we don't want to check it's requirements.

The bigger problem is the commenting in the code states:

  // module_invoke_all('requirements', 'runtime') returns an array that isn't
  // keyed by the module name, eg we might get a key 'ctools_css_cache'.
  // We have no way of knowing which module set this, and we can't guess based
  // on the name, as removing everything that begins with 'ctools_' might remove
  // data from other ctools sub-modules that we want to still monitor.
  // The only safe way is to use module_invoke, calling each module in turn.

And the way this currently works is by calling an update module function:

  $project_data = _nagios_update_get_projects();

  // [...] etc. etc.

/**
 * Wrapper function to ensure update_get_projects() can always be accessed,
 * even if the core update module is disabled.
 */
function _nagios_update_get_projects() {
  if (!module_exists('update')) {
    module_load_include('inc', 'update', 'update.compare');
    // Also need to load update.module for _update_cache_clear().
    drupal_load('module', 'update');
  }
  return update_get_projects();
}

This will always fail if update module was never initially installed. So unless there's another way to do this, without using the update module, we basically have a hard dependency on update.module in the Nagios module and all this code to try and work around the fact it might not be enabled is null and void. We should just declare a dependency and assume it is enabled!

Would love some thoughts on this...

Actually, what am I talking about? We can implement a slightly altered version of update_get_projects() - it's a simple enough function - and we still won't need an actual dependency (which I really don't want):
https://api.drupal.org/api/drupal/modules!update!update.compare.inc/func...

Replacement patch attached.

So there's a call to _nagios_update_get_available() as well, which tries to interact with update.module's cache. Working on a way around that too.

This code here ultimately calls update_get_available() which is heavily tied to the update module's caching in a way that means unpicking it will be ugly:

  // Copied from update_requirements(). Get available update data for projects.
  $data = array();
  // TODO: The TRUE param should be made configurable when writing a drush
  // command, so we don't rely on cached data.  if ($available = _nagios_update_get_available(TRUE)) {
    $data = _nagios_update_calculate_project_data($available);
  }

I think there's not much choice but to just accept this will break without the update module and write some warnings into watchdog.

New patch, this one includes an extra requirements check to actively warn people if the Update module is disabled and also should at least stop *errors* if it's never been installed. (Last patch still had some circumstances where the Nagios status page threw a db error because of the missing update module cache table.)

I could still use another opinion, but this works for me now on simplytest.me - I can uninstall the Update module completely and Nagios still works and doesn't error. Plus it warns on the status report page that you should enable the Update module for the best performance of the module.

Yep, RTBC. Thanks Greg!

Alright, Drupal 7 fix in, Drupal 8 will still need comparing and checking.

I have just fixed this in Drupal 8; will push shortly.

Could we get a new release for 7.x that includes this fix?

Thanks for the nudge. This fix is now part of the Drupal 7 release Nagios 7.x-1.4.