cURL line in status report (from Simpletest module) needs more (or less?) info

[bugfolder]

Description of the bug

The Simpletest module places a line in the Status Report like this if cURL is installed:

This is not terribly informative. If cURL isn't installed, then there is an error message that explains the need,

'The testing framework could not be installed because the PHP <a href="@hash_url">hash</a> extension is disabled.'

but when it is installed, there is no indication of who needs or cares about it.

(This came up in this Ubercart issue, which used the same idiom and ended up with up to four identical (and similarly opaque) status lines.)

We could make this line a little bit more useful by adding a 'description' field that says something like "cURL is required by the Simpletest testing framework," so that people will know where that line of the report is coming from.

Although, in the interest of decluttering the status report, I'd raise the question of whether we even need to list cURL in the status report if it is properly installed. It makes sense to list things like PHP version, MySQL version, etc., but for a simple "either it's there or it's not", maybe "silence on success" would be a good philosophy.

[klonos]

Sometimes there are vulnerabilities with certain versions of cUR, and ITSEC teams need to know if their website is patched against them when auditing company assets. So I think that eventually it would be beneficial to move this into system.install and be showing the version of cURL in the status report regardless of whether simpletest is enabled or not (other contrib or custom code in the site might also be using it). The Simpletest module could then hook onto that and add something to the description to denote that it requires it.

For now, we can do something like this in simpletest.install to show the version:

    $curl_version = curl_version();
    $curl_found = is_array($curl_version) ? $curl_version['version'] . ', ' . $curl_version['ssl_version'] . ', ' . $curl_version['libssh_version'] : $t('Enabled');
  $requirements['curl'] = array(
    'title' => $t('cURL'),
    'value' => $has_curl ? $curl_found : $t('Not found'),
  );

As it is now, we cannot override/alter requirements hooks that other modules have set. Once #6241 is done though, we could move this in system.install, and have simpletest.install only append The testing framework could not be installed because the PHP cURL library is not available. to the requirement description 😉

[bugfolder]

So how about for now if we move the reporting of cURL's version (or absence) into system core (as $requirements['curl']), and then in Simpletest, we only add a separate entry $requirements['simpletest_curl'] if cURL is missing.

Then, when hook_requirements_alter() is in, we can combine both bits of information into $requirements['curl'].

Other modules that require cURL may still want to create their own $requirements entries if they want to maintain compatibility with older versions of core (see https://github.com/backdrop-contrib/ubercart/issues/450).

[findlabnet]

For the record, in one of my modules I do check that way:

function abuseipdb_report_requirements($phase) {
  $requirements = array();
  $t = get_t();
  $has_curl = function_exists('curl_init');

  if ($phase == 'install') {
    if (!$has_curl) {
      $requirements['curl']['severity'] = REQUIREMENT_ERROR;
      $requirements['curl']['description'] = $t('The "AbuseIPDB report" module could not be installed because the PHP <a href="@curl_url">cURL</a> library is not available.', array('@curl_url' => 'http://php.net/manual/en/curl.setup.php'));
    }
  }

  return $requirements;
}

[bugfolder]

@findlabnet, if you're setting the 'description' of $requirements['curl'] in hook_requirements(), that would wipe out another module's description (or vice-versa) if they both needed cURL and added their own description for $requirements['curl']. But that's another good use case for hook_requirements_alter().

[findlabnet]

But in my case only for specific module 'install' phase. Anyway, if we're going to standardize something, I'm open to suggesting/changing it.

[klonos]

So how about for now if we move the reporting of cURL's version (or absence) into system core...

Yup, that sounds like a good plan @bugfolder 👍🏼

[klonos]

@bugfolder while at it, I see that in simpletest_requirements() we define $t = get_t(); and then use $t() instead of t(), however, AFAIK the simpletest module is not enabled during installation or any other phase where t() may not be available. Do you think that we should fix that here, or scope creep?

[klonos]

...another nit: the error we are showing is this:

The testing framework could not be installed because the PHP cURL library is not available.

I don't think that the could not be installed wording is accurate here, because the module is actually installed when we show this. Right? We should be using The testing framework requires ... instead, like we are doing with the rest of the items in simpletest_requirements().

[avpaderno]

hook_requirements() is also invoked when a module is being installed. In that case, the error message given by simpletest_requirements() is correct; it is not correct when the module is updated, or during runtime.

[bugfolder]

Do you think that we should fix that here, or scope creep?

I'm fixing some other nits at the same time, so yes, here.

[bugfolder]

PR updated, ready for further testing. Also fixed some capitalization consistency, some "that/which" correction, and the $t mentioned above.

[bugfolder]

CSpell didn't like "libssh" (3 places), so I added it to .cspell/cspell.json.

[avpaderno]

It is correct to use the value returned by get_t() when $phase is equal to 'install', which happens when a module is going to be installed.

[bugfolder]

It is correct to use the value returned by get_t() when $phase is equal to 'install', which happens when a module is going to be installed.

That's true, but it's only strictly necessary if the module is going to be installed by install.php, before the full Backdrop API is available. Simpletest, though part of core, is not installed (enabled) during a core install; thus, we can rely on t() being available when it is installed later. (Which is more or less what @klonos was saying above.) See https://docs.backdropcms.org/api/backdrop/core%21modules%21system%21system.api.php/function/hook_requirements/1 for details.

[klonos]

Which is more or less what @klonos was saying above.

Yup 👍🏼

hook_requirements() is also invoked when a module is being installed. ...

But that doesn't prevent the installation AFAIK. Does it?

[klonos]

CSpell didn't like "libssh" (3 places), so I added it to .cspell/cspell.json.

Better to add the word in the custom dictionary though. See my comments in the PR as to why.

[avpaderno]

If hook_requirements() returns an error, the module is not installed.

system_modules_submit()

  // Invokes hook_requirements('install').  If failures are detected, makes sure
  // the dependent modules aren't installed either.
  foreach ($modules as $name => $module) {
    // Only invoke hook_requirements() on modules that are going to be installed.
    if ($module['enabled'] && backdrop_get_installed_schema_version($name) == SCHEMA_UNINSTALLED) {
      if (!backdrop_check_module($name)) {
        $modules[$name]['enabled'] = FALSE;
        foreach (array_keys($files[$name]->required_by) as $required_by) {
          $modules[$required_by]['enabled'] = FALSE;
        }
      }
    }
  }

install_verify_requirements()

  // If there are errors, always display them. If there are only warnings, skip
  // them if the user has provided a URL parameter acknowledging the warnings
  // and indicating a desire to continue anyway.
  // @see backdrop_requirements_url().
  if ($severity == REQUIREMENT_ERROR || ($severity == REQUIREMENT_WARNING && empty($install_state['parameters']['continue']))) {
    if ($install_state['interactive']) {
      backdrop_set_title(st('Requirements problem'));
      $status_report = st('Resolve the problems and <a href="!url">try again</a>.', array('!url' => check_url(backdrop_requirements_url($severity))));
      $status_report .= '<br><br>';
      $status_report .= theme('status_report', array('requirements' => $requirements, 'phase' => 'install'));
      return $status_report;
    }
    else {
      // Throw an exception showing any unmet requirements.
      $failures = array();
      foreach ($requirements as $requirement) {
        // Skip warnings altogether for non-interactive installations; these
        // proceed in a single request so there is no good opportunity (and no
        // good method) to warn the user anyway.
        if (isset($requirement['severity']) && $requirement['severity'] == REQUIREMENT_ERROR) {
          $failures[] = $requirement['title'] . ': ' . $requirement['value'] . "\n\n" . $requirement['description'];
        }
      }
      if (!empty($failures)) {
        throw new Exception(implode("\n\n", $failures));
      }
    }
  }

[avpaderno]

Any core module can be installed by an installation profile, either because the module is required by the profile or because the module is programmatically installed by the profile. Modules should not assume they can use t() in hook_requirements() when the phase is install.

[klonos]

If hook_requirements() returns an error, the module is not installed.

I wasn't aware of that, but it does make sense now that I think about it.

For example, adding the following code in core/modules/book/book.install will always prevent the module from being installed:

/**
 * Implements hook_requirements().
 */
function book_requirements($phase) {
  $requirements = array(
    'book' => array(
      'title' => t('Book module installation'),
      'value' => t('Always fail'),
      'severity' => REQUIREMENT_ERROR,
      'description' => t('Just testing...'),
    ),
  );

  return $requirements;
}

PS: I don't like the logic and wording used in that error (it assumes that a component and its version have been specified as 'title' and 'value' in the requirements array, which only makes sense for things like db/webserver/php versions etc.), but that's a different issue.

Anyway, the point is that this checks out, so thanks @kiamlaluno for pointing this out 👍🏼 ...I always though that:

• $phase == 'install' means the Backdrop installer is running (install.php)
• $phase == 'update' means db updates are being performed (update.php is running)
• $phase == 'runtime' means any other time when neither install.php nor update.php are running

[bugfolder]

Thanks, @kiamlaluno for the clarification and explanation. $t is now restored to simpletest_requirements().

[bugfolder]

@klonos, your PR comments are (I hope sufficiently) addressed in the revised PR. Check again?

[klonos]

@bugfolder I think we may have a deeper core problem here... I've set up your PR branch on my local in order to do some testing, and I tried to mock-trigger those requirement errors. I started by changing the if (!$has_curl) { line in simpletest_requirements() to the opposite if ($has_curl) {, and that threw the following PHP error:

TypeError: strcasecmp(): Argument #2 ($string2) must be of type string, array given in strcasecmp() (line 3430 of /app/docroot/core/modules/system/system.module).

Sure enough, after adding a few dpm()s here and there, it turns out that the 'curl' key in the requirements array ends up having arrays instead of strings for its 'title'/'value'/'description': ...and that's because of the array_merge_recursive() used in module_invoke_all().

I haven't tested, but I'm sure that this is not caused by this PR - it just helped surface the problem: if more than one modules provide the same key in the $requirements array, you end up with this situation. We just never tried to do that before.

So we'll need to come up with a solution for that first I guess. Don't you just love rabbit holes 😅

[klonos]

There you go: https://www.drupal.org/project/drupal/issues/2865599

[argiepiano]

This is a known behavior of module_invoke_all(). I had to deal with this problem when working on Entity Tokens - receiving arrays instead of strings when doing a token replacement, because of collision with core tokens that used the same token strings (e.g. [node:field_my_name] trying to be replaced both by core tokens and Entity Tokens). After many hours on this, I came to accept that there is no way to work around this problem other than avoiding having multiple hook implementations returning arrays with the same keys (BTW, this is the main reason why in D7, Token uses low dashes, and Entity Tokens uses regular dashes - otherwise you get arrays instead of string replacements).

I believe this behavior would be difficult if not impossible to change in core, but perhaps there is a way to adapt update_check_requirements() to anticipate that arrays may be returned.

[klonos]

... BTW, this is the main reason why in D7, Token uses low dashes, and Entity Tokens uses regular dashes - otherwise you get arrays instead of string replacements

Oh wow! ...now I know why. Thanks @argiepiano 🙏🏼

I believe this behavior would be difficult if not impossible to change in core, but perhaps there is a way to adapt update_check_requirements() to anticipate that arrays may be returned.

I agree on both points 👍🏼 ...I've filed #6317 to see if there's something that we can do about it.

[bugfolder]

Interesting. Meanwhile, we can here do what I've done with Ubercart's curl checks: use a module-specific key (simpletest_curl) in modules that talk about the same topic.

[bugfolder]

There are now two entries for "cURL", with the error one from Simpletest at the top. I think that's appropriate.

[klonos]

Meanwhile, we can here do what I've done with Ubercart's curl checks ...

@bugfolder yes 👍🏼 ...however can you please have a look in #6317 as I think I might have come up with an acceptable solution to this? (as well as a plan forward to improve things once we have #6241)

[bugfolder]

Even without https://github.com/backdrop/backdrop-issues/issues/6241, I think that giving Simpletest its own unique hook_requirements() key, as above (and in the current PR) is a perfectly acceptable solution. Yes, there would be two rows of this table that reference cURL (one of them an error notice), but then, even if https://github.com/backdrop/backdrop-issues/issues/6317 were the solution path, there would be two rows that reference cURL; I don't see multiple rows as being a problem.

[klonos]

I don't see multiple rows as being a problem.

I do 😅 ...I actually want to reduce the complexity and amount of rows we have in the status page. See #5863, #5864, #5758.

However, I realize that we currently have many blockers, so getting this 100% right may take some time. So in the interest of not delaying this further, I am fine with having 2 separate rows for cURL, but can we please add a @todo (ideally in the Simpletest module) to come and merge those rows later when we have #6241 and/or #6317?

[bugfolder]

I"m happy to add the @todo to keep the ball rolling on the PR, but I will note that the extra row only appears when there's a problem, and it appears at the top of the page with a red X indicating that it needs attention. In day-to-day operations, there's no additional row.

[klonos]

🤣 🤣 🤣 ...unfortunate username choices 😅

[bugfolder]

@klonos, can you do a (hopefully final) review of this PR?