Add a property to .info files to allow modules to specify required php extensions

I think this was originally drawn up against the 7.x branch. Adjusting for the moment to see how it does. Can switch back later if it continues to fail.

#1: add-php_ext-param-to-dot-info-file-properties-1322890-1.patch queued for re-testing.

Please credit @willvincent, not I, if this is rolled up.

Tagging for contributor experience, since this isn't necessarily a DX only problem.
Adding this functionality will help prevent someone from starting the installation or executing a module's code that requires a specific PHP extension.

This makes sense and seems to be beneficial addition.

core = 8.x
php_ext[] = gd

The proper name "php_ext" looks and reads a bit strange. It's also an abbreviation, which we generally try to avoid. Additionally, it is a multi-value property, so the name should actually be plural.

Can we think of a better name? E.g., "php extensions"? Or why not simply "extensions"? Or would that be too vague?

Second, I wonder whether this location is really the only place to adjust. Isn't that limited to the Modules page output? I may be mistaken, but isn't there some other code somewhere else that runs along the lines of hook_requirements()?

@sun,
hook_requirements() is useful, but only takes effect when you're actively trying to install something. Noting an incomplete requirement beforehand is IMHO a better experience.

"extensions" seems a bit vague, so "php extensions" doesn't seem all that bad. Natural language is typically easier to remember.

re: #12: hook_requirements():

Well, the problem is that just outputting the lack of a PHP extension is insufficient. A module needs to be actively prevented from being installed, as with any other requirement.

I see that the code being touched here results in an appropriate UI change on the Modules page (Red X instead of checkbox).

Looking some further into the module system and System module, it actually looks like we're performing these checks only within the Modules page/form (OMG):

http://api.drupal.org/api/drupal/core!modules!system!system.admin.inc/fu...
http://api.drupal.org/api/drupal/core!modules!system!system.admin.inc/fu...

Which in turn means that this patch should actually be complete. I hope someone else can double-check and confirm that.

That said, there's one last detail left to bikeshed:

AFAIK, we're using spaces instead of underscores in .info files (e.g., "base theme"), so the property name would actually have to be "php extensions".

AFAICS, tools like Drush will have to (re-)implement this check on their own currently, so I guess there's not much that can be double-checked.

I'd highly appreciate if we'd create a follow-up issue to properly untangle that compatibility/requirements checking from System module (e.g., by moving the non-form related code into module.inc). That would be lovely.

Done #1728314: Move module/extension compatibility/requirements checking from System module into module.inc

Hm. I just wanted to RTBC this, but actually -- it is possible to test the expected behavior and thus we should add tests for it.

Cool, I'll try writing the tests for these in the next couple of days if Will doesn't beat me to it. Can anyone identify exactly what should be tested?

If we add this, what's the point of hook_requirements($phase = 'install')?

Also, does Composer have anything like this?

hook_requirements() is still used for more complex requirements checks.

Composer has support for specifying dependencies on PHP extensions (via 'requires', I think), but I'm not sure how that is relevant here?

(Apparently, support for dependencies on PHP extensions is not in the official documentation, and was only tacked on through a hack recently here: https://github.com/composer/composer/issues/312 -- alas, they special-cased the ext-* package namespace...)

That said, would it be too crazy if we'd change this property into a sub-keyed notation, so as to allow to differentiate between required/supported dependencies?

E.g., something along the lines of:

php extensions[requires][] = curl
php extensions[supports][] = apc
php extensions[supports][] = uploadprogress

Just raising the idea. That might allow (but not 100% sure) to simplify some of the hook_requirements() implementations. — Not really sure, because the optionally supported extensions are usually amended with a description that clarifies how they're helpful and better, so if we'd replace those checks with simple properties, there'd be no way to do that. Perhaps the separation by requires/supports is not a good idea after all...

The composer question was in relation to #1398772: Replace .info.yml with composer.json for extensions if we ever do that for the package management stuff in modules.

Things like supports are generally handled by runtime status messages as well, so I don't think it'd necessarily help to have that in .info files.

Wondering a bit about stuff like gd vs. imagemagick (and bcmath vs. whatever the other one is that OpenID uses), but maybe that's what hook_requirements() stays useful for.

Setting status to NW and adding needs tests tag.

This would be very very very helpful for #1447736: Adopt Guzzle library to replace drupal_http_request(), since that issue is essentially only blocked on adding the cURL requirement to a couple of core modules.

Also, we should add a version check, like modules dependencies:

extensions[requires][] = curl (>1.0)
extensions[supports][] = apc (>3.0.0)
extensions[supports][] = uploadprogress (2.1)

The version of an installed extension is retrieved through PHP's phpversion().

Let's handle versions in a separate follow-up issue.

You should provide a test that the functionality is working:

I think it could work like this:

a) Add a new test module and add a non-existing extension
b) Add an always required extension to another module

Change or Copy:

core/modules/system/lib/Drupal/system/Tests/Module/RequiredTest.php

And just make sure for a) the checkbox is disabled and for b) it is enabled.

That should be all that is needed.

Thanks!

I think it should be more like:

  $module = 'test_module_name';
  $info = $module_info[$module];
  $field_name = "modules[{$info['package']}][$module][enable]";

  $this->assertFieldByXPath("//input[@name='$field_name' and @disabled='disabled' and @checked='checked']", '', format_string('Field @name was disabled and checked.', array('@name' => $field_name)));

But the hidden makes that really problematic.

Suggestion:

Unhide the modules for now, make the test pass (locally), then merge your work into:

#1447736: Adopt Guzzle library to replace drupal_http_request()

and implement the test with

"aggregator needs curl" && extension_loaded('curl')

So that in either case:

curl present -> aggregator enabled

and

curl not present -> aggregator disabled

the test succeeds.

#42: Uh, hi! :-)

First of all I am no "real" core developer, I just happen to have a little interest in core things, like you.

Second, thanks for all the work. It is a nice patch. I definitely did not want to frustrate you. I am sorry if it came across this way.

Yes, it is true that the requirements of a patch are high and testing can be very difficult to grok. ( When I wrote my first tests I was like WTF! )

But it is totally worth it, because soo often tests prevent severe failures later on.

Again, thanks a lot for a nice patch!

Marked #1859536: Testing/Aggregator module warning: Requirements check comparison output two 'strings' (cURL) as an array as duplicate of this bug.

I have not read all this issue completely, not yet.

Does this issue deal with integrating requirements information in the Status page?

Fro example, a requirement that is met may display details in the status page, like the version number of the required PHP module, or, like for cURL, display a warning in the Status page and offer help on how to install cURL.

We define the requirements (in the .info file that would be easier than in the .install file), and then we act upon the requirements (sometimes adding a row in the Status page).

@xmacinfo: I'm pretty sure you meant to link to some other issue on your comment there. You accidentally linked to this very same here ;)

Indeed. I fixed the link above.

Thanks.

Fro example, a requirement that is met may display details in the status page, like the version number of the required PHP module, or, like for cURL, display a warning in the Status page and offer help on how to install cURL.

The problem with this is that once you are missing an extension for an enabled module, you are already doomed, i.e. there is no way to prevent a fatal as that module may call a function from the extension, for example. Therefore you need to tell people that an extension is missing *before* you enable a module, which is what the current patch does: It adds that information to the Modules page.

The problem with this is that once you are missing an extension for an enabled module, you are already doomed,

This not always the case. The requirements in the .install files can display a status message in the Status page or a warning. If cURL is not available, the website is not doomed. It is still fully functional. Only some file-based function will fail or emit errors.

If we display errors only on the modules page, we leave out most of the Status page information. In that sense, that would be a regression.

This not always the case. The requirements in the .install files can display a status message in the Status page or a warning. If cURL is not available, the website is not doomed. It is still fully functional. Only some file-based function will fail or emit errors.

Well, it might not always be the case, but I would still consider it a problem if we're
*sometimes* doomed, with no reason to control when. In the example of cURL it's obviously hard to imagine a hardcoded call to curl_init() or whatever on every page, but since it's possible it's what we have to expect. Therefore we *cannot* load the module file of a module that depends on a missing extension. .install file, fine; we can document to only use native PHP functions there, that shouldn't be a problem.

What we could do is find all required extensions by all *disabled* modules and list the missing ones on the status page as a warning, such as "Module A (disabled) requires extension foo, so you currently can't enable it." Would that be a compromise? :-)

What we could do is find all required extensions by all *disabled* modules and list the missing ones on the status page as a warning, such as "Module A (disabled) requires extension foo, so you currently can't enable it." Would that be a compromise? :-)

Well, that will be difficult with distributions.

I think this should be done, but only for enabled modules as those make the problem.

Think of scenario:

* Move to new server
* Import DB
* Aggregator is enabled, curl is disabled
* Check status report: Warning: Aggregator needs curl enabled.

=> WIN!

@Fabianx: I've to agree with @tstoeckler here - IF a module requires PHP extension X, you have a >50% chance of completely blowing up your system if the extension is not loaded. In turn, you likely won't be able to reach that Status report page.

But anyway, friends, let's not over-engineer this. Let's use the exact same approach that we have for (missing) module dependencies right now. That should be sufficient for most cases, the rest we can figure out later.

Also bear in mind that possible tests are very limited by design here - AFAICS, the only test we can reasonably expect from this issue/patch is a test that verifies that a module that requires a non-existing PHP extension "foo" cannot be installed. More sophisticated tests than that require in-depth hackery of various subsystems.

And I'm not even sure whether it is worth to provide any more detailed tests in the first place, since the moment you install a module that requires a PHP extension that isn't required by Drupal core, you are effectively entering custom waters. Drupal cannot, should not, and will not help you to correct blatant mistakes when staging a site from A to B and blatantly forgetting that the code requires the PHP extension "foo" to exist. That's a level of validation we cannot reasonably provide. We could as well validate that Drupal runs on a "correctly configured" Apache webserver... Nope, no way, out of bounds.

IF a module requires PHP extension X, you have a >50% chance of completely blowing up your system if the extension is not loaded. In turn, you likely won't be able to reach that Status report page.

I am totally on board with that and don't think we need it for enabled modules, I was just opposed to showing it for disabled modules on the status page ...

All I want to highlight is that the same function that currently checks for requirements also builds the status page.

Here the PHP requirement builds the display of the PHP version number and the link to the PHP status page.

 // Test PHP version and show link to phpinfo() if it's available
  $phpversion = phpversion();
  if (function_exists('phpinfo')) {
    $requirements['php'] = array(
      'title' => $t('PHP'),
      'value' => ($phase == 'runtime') ? $phpversion .' ('. l($t('more information'), 'admin/reports/status/php') .')' : $phpversion,
    );
  }

My original concern: if we move the requirement properties to .info files, will there be provision to build at the same time the Status page?

I am not arguing about displaying warning for disabled PHP modules.

I don't understand when and how and what you want to display on the Status page. As I've tried to make clear, showing missing extensions for *enabled* modules is not on the table IMO, as that's already too late.

I would like to work on and propose a TouchPoint functionality, but not in this issue. I'll refers to the OSGi TouchPoint concept which allows native touchpoint to integrates with a system. I would love to provides a set of touchpoints like: PECLTouchPoint, PEARTouchPoint, ComposerTouchPoint, AptitudeTouchPoint, YumTouchPoint, etc. which install, configure and load such extensions.

To see the big picture at a module installation, we could introduce a dependency resolver which would fetch the drupal module dependencies from drupal.org with their corresponding version (maybe?) and install the corresponding php extensions and/or native package that are needed from TouchPoints. This would be an addition to the module manager.

For instance a PECLTouchPoint would be roughly like /core/vendor/doctrine/common/bin/travis-setup.php:

<?php
public function install($name)
    {
        if (array_key_exists($name, $this->extensions)) {
            $extension = $this->extensions[$name];


            echo "== extension: $name ==\n";

            foreach ($extension['php_version'] as $version) {
                if (!version_compare($this->phpVersion, $version[1], $version[0])) {
                    printf(
                        "=> not installed, requires a PHP version %s %s (%s installed)\n",
                        $version[0],
                        $version[1],
                        $this->phpVersion
                    );

                    return;
                }
            }

            $this->system(sprintf("wget %s > /dev/null 2>&1", $extension['url']));
            $file = basename($extension['url']);
            $this->system(sprintf("tar -xzf %s > /dev/null 2>&1", $file));
            $folder = basename($file, ".tgz");
            $folder = basename($folder, ".tar.gz");
            $this->system(sprintf(
                'sh -c "cd %s && phpize && ./configure %s && make && sudo make install" > /dev/null 2>&1',
                $folder,
                implode(' ', $extension['cfg'])
            ));
            foreach ($extension['ini'] as $ini) {
                $this->system(sprintf("echo %s >> %s", $ini, $this->iniPath));
            }
            printf("=> installed (%s)\n", $folder);
        }
    }
?>

I'll work on a sandbox for this but anyone excited by this project is welcome to work with me.

I don't understand when and how and what you want to display on the Status page. As I've tried to make clear, showing missing extensions for *enabled* modules is not on the table IMO, as that's already too late.

I am not arguing about displaying warning for disabled PHP modules. I do not want to show missing extensions for *enabled* modules either.

I want to know how the Status Page will be build if we move the requirement check to the info files. Will the Status page be gone?

@willvincent: That was my understanding. Thanks for the clarification.

I don't really get what people have against showing it on the status page either, *additionally* to a pre-install check.

I think it would be very useful to have an ok message giving you list of all extensions required by your installation. Because then you have a single place to look for extensions that your new server needs to support in case you move it around. And if we have that, it's easy to change it to red if something is missing. Or list the red ones separately.

If you have something that blows up before you can visit that page, bad luck, but I think most modules aren't going to do that. Looking at core, I fail to see a single example that will prevent you from visiting the status page. Also, those things are checked in update.php as well, so it's quite possible that you do a module update which has a new extension dependency that you're missing. And update.php blowing up already because of it at that step is *very* unlikely.

Also, I'd suggest to update the existing examples in core which require php extensions, shouldn't be that many and should show how much copy & paste code we can avoid with this.

I'd like to have an overview displayed on the site status page as well. Perhaps if not within the score of this issue but can be (relatively) easily implemented, how about a follow-up issue then?

A side note that cURL now needs to be a system requirement, and not just an optional requirement for certain modules. It's used to request localization files from localize.d.org during the install process. #1991298: Fatal error when installing Drupal on servers that don't have the cURL extension

Hmm that's a good point, I don't think this was accounted for with the drupal_http_request()/guzzle change since most of the other calls are very optional.

For untranslated installs we don't need to download translations and it seems a bit rigid to add a system requirement for that (or for something needed in the installer).

Also wondering if we should consider a fallback just for the installer to use something dumb.

The real function being used is install_retrieve_file() which is an API function that any install profile can use. It's not just limited to translations.

Sorry, meant to link to #1991298: Fatal error when installing Drupal on servers that don't have the cURL extension in #61. I truly believe that it is in our best interest for our end users and module developers, that I should be able to rely on being able to use Guzzle without needing to add a requirement to my module. It's provided by core, why do I have to add a dependency in my module?

referencing follow up issue in summary.

attempting a reroll.

uploaded the wrong patch.

+++ b/core/modules/system/tests/modules/php_extension_requirement1_test/php_extension_requirement1_test.info
@@ -0,0 +1,7 @@
+php extensions[] = foobar

Do we have any other yaml keys with spaces in them? That seems like a great confusion source...

Otherwise this seems reasonable.

I searched the other info files using the regex ^[.*]\s[.*]: but couldn't come up with any other keys having a space.
hence I renamed the key to php_extensions

also, When I examined the test report, I couldn't find the message corresponding to the test. would appreciate some help on that.

uploading the updated patch.

Drupal extensions can specify PHP extensions in their composer.json file. #2494073: Prevent modules which have unmet Composer dependencies from being installed is trying to solve some of that, but it looks like the most elegant solution in that issue is to mark Drupal extensions as requiring installation through Composer, rather than solving the dependency tree.

Thus it looks like Composer might solve the problem here.