Prevent modules which have unmet Composer dependencies from being installed

If we go with a single composer.json and vendor/ directory for core and contrib, it's quite easy. We ship with a hash of the composer.lock file for that specific core tagged version. Before doing certain actions, we rehash it and see if it's changed. If it has, we assume the site owner has added a composer lib, run composer update, or otherwise opted into a Composer World(tm). We therefore disallow composer-unsafe-actions(tm), whatever those happen to be. Instead, we show appropriate instructional text.

If we use a separate vendor/ directory for core and contrib, it's probably much the same except that we have to check for a top-level composer.json/composer.lock independently of core, so the hash logic is not the same.

For hook_requirements, core can implement the following logic when checking installability (pseudo-code):

foreach ($module) {
  if (file_exists($module_dir . '/composer.json')) {
    if (/* composer.json has a requirement that doesn't begin with 'drupal/' */) {
      if (/* root level composer.lock file does not indicate that dependency is installed */) {
        // mark module as uninstallable with instructions to run 'composer require drupal/$module'
      }
    }
  }
}

Well then let's focus that title a bit, shall we... :-)

...following up on that, we agreed this is actually probably major because the workaround is for each individual module or package to implement its own hook_requirements() logic duplicating the composer dependency check. Ugly, but it would allow developers to work around this until it is fixed.

Attached patch contains a module with a composer.json file that requires scalopus/empty. This is an empty repository so it won't add any actual code to drupal.

I've changed ModulesListForm::buildModuleList to include a check for a composer.json file in the installing module's directory and made sure the module can't be installed for now. Also added a test that checks this behavior.

To do:

- Answer the question posed by @Crell in #2 on wether we should use one /vendor directory for core and contrib or use one for each.
- Adapt the code in ModulesListForm::buildModuleList to check for the actual contents of the composer.lock file.

+ drupal_set_message(t('Module !module can not be installed before composer dependencies are installed. You should run `composer install` from the module directory to install dependencies.', ['!module' => $module]));

Except this is completely false.
And the actual command to run depends on various factors, including whether the root composer.json is used.
I think the best we can do is link to a d.o page.

Changed the message in the patch.
Added the remaining todo's to the IS.

@Xano, you're right that we should also test that a module can be installed if it's composer dependencies are installed. I don't think we can write this test before we've solved the question wether we should have one or more vendor directories.

Attached patch fixes #12. There now is some code that actually checks for composer requirements.
I'm not sure this is the best way to do this because it doesn't check for version requirements, that should still be improved.

There is now also a test for a module that can be installed, so #10.2 is now also fixed.

#10.1 still needs to be done, I added this to the IS's remaining tasks.

#14:

1. We introduce a new hook_requirements_alter to make sure this is on API level,
2. We could do this with a path in settings.php but if we're going to suggest that people use an application-specific composer file than we should use that one. Fixed that in the patch.
3. We could use https://github.com/composer/composer, the \Composer\Package\Version\VersionParser class looks like it should have methods to do this.

The current patch will break the test because the implementation is not correct yet.

So this makes everything green again, this is good. All we have to do now is do the version checking with composer/composer.

Fixes #18

Attached patch includes the semver package, that should only be in #2575469: Require the composer/semver library to do version checking.. We currently still have failures in the tests because of the alter hook.

Thanks @pjonckiere, attached patch resolves your remarks.

Locally the HookRequirementsTest works again. Hoping the rest is green as well.

can't merge a NULL value and an array, should fix at least Drupal\minimal\Tests\MinimalTest

In #35 \Drupal\system\Tests\Update\UpdateScriptTest failed, this is now green locally. This shouldn't break anything else either so hopefully all is green now.

1. +++ b/core/modules/system/system.module
   @@ -482,6 +483,84 @@ function system_updater_info() {
   +    if (file_exists('core/vendor/composer/installed.json')) {
   

   This hardcodes our vendor directory. This will cause problems if user want to move their vendor dir somewhere else.

2. +++ b/core/modules/system/system.module
   @@ -482,6 +483,84 @@ function system_updater_info() {
   +        'description' => t('Not all the modules have their Composer dependencies installed yet. Please find more information on <a href="@handbook">this handbook page</a>.',
   

   Maybe scope creep, but whats about themes and install profiles? Maybe we can implement this in a generic way with ExtensionDiscovery?

3. +++ b/core/modules/system/tests/modules/composer_installable/composer.json
   @@ -0,0 +1,6 @@
   +{
   

   Nitpick, we use 2 spaces in composer.json

4. +++ b/core/modules/system/tests/modules/composer_uninstallable/composer.json
   @@ -0,0 +1,6 @@
   +{
   

   Codestyle, 2 spaces

Fixes #41.1, .3 and .4.

I feel like .2 is scope creep.

Removed the hook_requirements_alter.

Introduced a new Exception: ExtensionComposerRequirementsException.

mark_modules_with_unmet-2494073-56.patch is supposed to fail because the required composer/semver isn't in yet. That's why the mark_modules_with_unmet-2494073-56-include-package.patch does work.

Fixed some small nitpicks, same argument as in #60 still applies: only the -include-package patch will pass.

I applied the smaller patch in #61 and updated the package to the correct location. The failures in #61 were expected because it did not actually include the package. Let's see how this one fares.

@borisson_, a suggestion: You can use the extension .txt if you don't want the patch without the package to be tested. Alternatively, you can suffix "-do-not-test" in the filename as well, e.g. mark_modules_with_unmet-2494073-61-do-not-test.patch.

@hussainweb, the library should be added in #2575469: Require the composer/semver library to do version checking., that's why it was added as 2 seperate patches every time.

The other issue is RTBC and accepted by framework manager. We can get back to this once #2575469: Require the composer/semver library to do version checking. gets in.

The other issue is RTBC and accepted by framework manager. We can get back to this once #2575469: Require the composer/semver library to do version checking. gets in.

#2575469: Require the composer/semver library to do version checking. is in. I am uploading the small patch in #61 again, which still applies.

See also: #2536576: Add composer_dependency module, have it help users figure out how to install Composer dependencies

If we don't get this in now, I think we should remove core's dependency on composer/semver as well; because that code was only intended to be used by this patch and is otherwise dead. (Reverting #2575469: Require the composer/semver library to do version checking.).

Using Semver here to define constraints: #2579663: Can't use 'composer install' with missing composer.lock and vendor folder

Added a testable class called Drupal\Core\Composer\ExtensionComposerDependencies, with tests, which you can use to query whether the Composer dependencies are met. It's a best-faith effort to verify Composer dependencies without just including composer/composer in the repo.

This simplifies drupal_check_composer_requirements() by making it non-existent.

This patch doesn't pass tests because it (accurately) throws ExtensionComposerRequirementsException which isn't handled anywhere during form building for the module list.

Anyone else is welcome to figure out how to build the form with this in mind.

Fixed errors in #77, some improvements.

RTBC - looks great to me!

CI error: https://dispatcher.drupalci.org/job/default/39506/console

Setting back to RTBC.

@Xano #85 :Thanks. :-)

#85.1: Research tells me that Composer doesn't store the dev status of a root package, so we can't determine it. So now that part of the code only checks for requires and not requires-dev.

#85.4: Thank @borrison. :-)

#85.6, 7: It's the same pattern as UnmetDependenciesException, except with even simpler logic. Check out ModulesListForm::submitForm() to see the similarities. Also, having the translatable string in the exception allows other code to use the message.

This patch addresses all these things, except ExtensionComposerRequirementsException still generates a translated string.

There are numerous documentation problems with the latest patch...

1. +++ b/core/lib/Drupal/Core/Composer/ExtensionComposerDependencies.php
   @@ -0,0 +1,157 @@
   +   * Determine whether Composer-based dependencies are still required.
   

   Determine -> Determines

2. +++ b/core/lib/Drupal/Core/Composer/ExtensionComposerDependencies.php
   @@ -0,0 +1,157 @@
   +        'description' => t('Not all the modules have their Composer dependencies installed yet. Please find more information on <a href="@handbook">this handbook page</a>.', [
   

   Please make the link text in this string accessible. Link text like "this handbook page" is not correct.

   And don't use the word "please" in UI text, and we should NEVER call the online documentation the "handbook".

   Also aren't we using :url not @url in t() for URLs?

   Anyway I suggest the wording of the second sentence of this t() should be:

   For more information, see the (link start)online documentation on Composer(link end)

   One more thing: we usually avoid linking to node IDs. Do we need an alias for that page? If so, what should it be?

3. +++ b/core/lib/Drupal/Core/Composer/ExtensionComposerDependencies.php
   @@ -0,0 +1,157 @@
   +   * Determine whether the Composer-based dependencies of an extension are met.
   

   Determine => Determines

4. +++ b/core/lib/Drupal/Core/Composer/ExtensionComposerDependencies.php
   @@ -0,0 +1,157 @@
   +   * Parse an installed.json file into the useful parts.
   

   Parse => Parses

5. +++ b/core/lib/Drupal/Core/Composer/ExtensionComposerDependencies.php
   @@ -0,0 +1,157 @@
   +   *   JSON contents of installed.json file.
   

   of => of an

6. +++ b/core/lib/Drupal/Core/Composer/ExtensionComposerDependencies.php
   @@ -0,0 +1,157 @@
   +   * @return array
   

   seems to be string[] rather than generic array?

7. +++ b/core/lib/Drupal/Core/Composer/ExtensionComposerDependencies.php
   @@ -0,0 +1,157 @@
   +   *   Array with package names as the keys and normalized version as the
   

   version -> versions

8. +++ b/core/lib/Drupal/Core/Extension/ExtensionComposerRequirementsException.php
   @@ -0,0 +1,50 @@
   +   * The machine name of the module with unmet dependencies.
   ...
   +  protected $moduleName;
   

   Is this just for modules? The class header docs say "extension".

   Let's get the member variable name here all docs headers on the same page. Either always use extension or always use module. Not a mix.

9. +++ b/core/lib/Drupal/Core/Extension/ExtensionComposerRequirementsException.php
   @@ -0,0 +1,50 @@
   +    return $string_translation->translate("Module @module has unmet Composer-based dependencies.", ['@module' => $this->moduleName]);
   

   Question: does calling $string_translation->translate() get a string into the POT database? I don't think it does?

10. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
    @@ -98,9 +101,16 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
    +      $composer_dependencies = new ExtensionComposerDependencies();
    

    Is there a reason this isn't a service?

11. +++ b/core/modules/system/src/Tests/Module/HookRequirementsTest.php
    @@ -28,4 +30,49 @@ function testHookRequirementsFailure() {
    +  /**
    +   * A module with uninstalled Composer dependencies is not installable.
    +   */
    +  function testComposerDependenciesFailure() {
    

    This docs header doesn't conform to our normal docs standards for a function/method.

12. +++ b/core/modules/system/src/Tests/Module/HookRequirementsTest.php
    @@ -28,4 +30,49 @@ function testHookRequirementsFailure() {
    +  /**
    +   * A module with installed Composer dependencies is installable.
    +   */
    +  function testComposerDependenciesSuccess() {
    

    Same here.

    Both of these should start with something like:

    Tests that...

13. +++ b/core/tests/Drupal/Tests/Core/Composer/ExtensionComposerDependenciesTest.php
    @@ -0,0 +1,124 @@
    +/**
    + * @coversDefaultClass \Drupal\Core\Composer\ExtensionComposerDependencies
    + *
    + * @group Composer
    + */
    +class ExtensionComposerDependenciesTest extends UnitTestCase {
    

    Don't we require a summary line for all class docs headers, or is there an (unwritten in standards) exception for test classes?

14. +++ b/core/tests/Drupal/Tests/Core/Composer/ExtensionComposerDependenciesTest.php
    @@ -0,0 +1,124 @@
    +  /**
    +   * @return array
    +   *   - Expected parsed installed.json data.
    +   *   - Contents of installed.json file.
    +   */
    +  public function provideParseInstalled() {
    

    this docs header has no summary line.

15. +++ b/core/tests/Drupal/Tests/Core/Composer/ExtensionComposerDependenciesTest.php
    @@ -0,0 +1,124 @@
    +  /**
    +   * @covers ::parseInstalledPackages
    +   *
    +   * @dataProvider provideParseInstalled
    +   */
    +  public function testParseInstalledPackages($expected, $installed) {
    

    no summary line

16. +++ b/core/tests/Drupal/Tests/Core/Composer/ExtensionComposerDependenciesTest.php
    @@ -0,0 +1,124 @@
    +  /**
    +   * @return array
    +   *   - Boolean showing whether the dependencies are met by the installed
    +   *     packages.
    +   *   - Object of dependencies parsed from JSON.
    +   *   - Array of installed dependencies, with package name as key and version
    +   *     as value.
    +   */
    +  public function provideExtensionDependencies() {
    

    no summary line

    I've skipped pointing out the rest of this class lacks good docs for the methods.

Nice, thanks for the review. :-)

Is there a reason this isn't a service?

Good point. I think we only ever need it when there aren't a lot of dependencies available, such as install time. Also it's not a good candidate for replacement with an alternate service. Someone with more service-fu can address this.

One more thing: we usually avoid linking to node IDs. Do we need an alias for that page? If so, what should it be?

Heh. The node with the ID is this issue. :-)

Clearly we need a better doc page. We currently don't have a defined behavior for contrib-with-composer, so it's hard to tell people what to do.

I've left that part (#89.2) as-is, and addressed the rest of the concerns. So it still needs work.

As far as the PHPUnit summary lines... You can see that it's mostly redundant. The annotation tells all the stories. There are a lot of tests with no summary lines for this reason.

+++ b/core/lib/Drupal/Core/Composer/ExtensionComposerDependencies.php
@@ -0,0 +1,157 @@
+   */
+  public function extensionComposerRequirements($drupal_root, Extension $extension) {
+    $requirements = [];
+    if (!$this->dependenciesAreMet($drupal_root, $extension)) {
...
+   */
+  public function dependenciesAreMet($drupal_root, Extension $extension) {
+    // Does the extension have a composer.json file?
+    $extension_composer_json = $drupal_root . '/' . $extension->getPath() . '/composer.json';
+    if (is_readable($extension_composer_json)) {

I'm wondering whether the drupal root should be passed in as constructor parameter?

This patch is great and a whole lot better than how I left it. Great work @Xano & @Mile23.

I did however find a couple of small issues that I think can be resolved. But I only feel strongly about the documentation remarks I made, if you disagree with the other remarks they can be left as-is.

1. +++ b/core/lib/Drupal/Core/Composer/ExtensionDependencyChecker.php
   @@ -0,0 +1,110 @@
   +   * @var string[]
   +   *   Keys are package names and values are package versions. Example:
   +   *   ['crell/api-problem' => '1.7.0'.\].
   

   There's a syntax error here. The .\ should be removed.

2. +++ b/core/lib/Drupal/Core/Composer/ExtensionDependencyChecker.php
   @@ -0,0 +1,110 @@
   +    if (is_readable($composer_file_path)) {
   

   Can we change this? By reverting this check, we can make sure the rest of the code isn't as deeply nested.

   if (!is_readable($composer_file_path)) {
     // Modules that don't have a composer file don't have external dependencies, which means they are met.
     return TRUE; 
   }
   
   $composer_dependencies = ...
   

3. +++ b/core/lib/Drupal/Core/Composer/ExtensionDependencyChecker.php
   @@ -0,0 +1,110 @@
   +      // Gather all the requirements.
   +      $require = [];
   

   Can we change this variable name to $requirements? That makes more sense I think.

4. +++ b/core/lib/Drupal/Core/Composer/ExtensionDependencyChecker.php
   @@ -0,0 +1,110 @@
   +   *   Keys are package names and values are package versions. Example:
   +   *   ['crell/api-problem' => '1.7.0'.\].
   

   See earlier comment.

5. +++ b/core/lib/Drupal/Core/Composer/ExtensionDependencyChecker.php
   @@ -0,0 +1,110 @@
   +    // Find the installed packages once, and cache the data.
   +    if (empty($this->composerInstalled)) {
   

   We can also change this check to reduce the nesting.

   if (!empty($this->composerInstalled) {
     return $this->composerInstalled;
   }
   ...
   

I noticed one small typo, otherwise I feel this is RTBC.

+++ b/core/lib/Drupal/Core/Composer/ExtensionDependencyChecker.php
@@ -56,30 +54,35 @@ public function __construct($drupal_root) {
+    // If the extension has on Composer file, it has no unmet dependencies.

s/on/no/

This patch changed significantly since the RTBC in #80 and all the work I did on the patch was before that. Because of that, I feel confident setting this to RTBC.

What happens to a site which has a module installed where the composer dependencies are not satisfied? Do we need to think about an upgrade path for this?

I think the docs and UI text strings need a few small fixes in this patch as well:

1. +++ b/core/lib/Drupal/Core/Composer/ExtensionDependencyChecker.php
   @@ -0,0 +1,122 @@
   + * Performs a naive check for Composer-based module dependencies.
   

   Just a comment: a class that is used to deny installation of a module, which says it is "naive" right in the doc block, is a bit scary! :)

2. +++ b/core/lib/Drupal/Core/Composer/ExtensionDependencyChecker.php
   @@ -0,0 +1,122 @@
   +   * @see self::getInstalledPackages()
   

   @see should be at the end of doc blocks, and @see should always have the namespace/class in the line not self::

3. +++ b/core/lib/Drupal/Core/Composer/ExtensionDependencyChecker.php
   @@ -0,0 +1,122 @@
   +   *   Either NULL when the installed packages have not been computed yet, or an
   +   *   array of which keys are package names and values are package versions.
   

   This is a bit garbled. I started ready "... or an array of which keys are package names", and then realized it meant "an array whose keys are package names and whose values are" when I got to "values".

   Also the example... how is crell/api-problem a package name? maybe this needs more explanation?

4. +++ b/core/lib/Drupal/Core/Composer/ExtensionDependencyChecker.php
   @@ -0,0 +1,122 @@
   +   * @param \Drupal\Core\Extension\Extension $extension
   

   Should this be an interface?

5. +++ b/core/lib/Drupal/Core/Composer/ExtensionDependencyChecker.php
   @@ -0,0 +1,122 @@
   +   *   Keys are package names and values are package versions. Example:
   +   *   ['crell/api-problem' => '1.7.0'].
   

   Again I am not sure what "package names" are?

6. +++ b/core/lib/Drupal/Core/Composer/ExtensionDependencyRequirements.php
   @@ -0,0 +1,82 @@
   +        'description' => $this->t('Some modules have unmet Composer dependencies. Read the <a href="@documentation">documentation</a> on how to install them.', [
   

   This is not great link text for accessibility. We generally want the text in a link to tell you where the link goes. This one "documentation" doesn't really do that.

7. +++ b/core/lib/Drupal/Core/Composer/ExtensionDependencyRequirements.php
   @@ -0,0 +1,82 @@
   +        'description' => $this->t('All Composer dependencies have been installed.'),
   

   Hm. Are the composer dependencies just other modules that need to be installed, or can they be other things? maybe just say "have been met", as elsewhere in this patch?

8. +++ b/core/lib/Drupal/Core/Extension/ExtensionComposerRequirementsException.php
   @@ -0,0 +1,50 @@
   +    return $string_translation->translate("Extension @extension has unmet Composer dependencies.", ['@extension' => $this->extensionName]);
   

   Um... does the POTX extractor pick up ->translate() calls? I thought you had to use t()?

9. +++ b/core/lib/Drupal/Core/Extension/ModuleInstallerInterface.php
   @@ -42,6 +42,9 @@
   +   *   Thrown when Composer-based dependencies are not present.
   

   present -> met

10. +++ b/core/modules/system/src/Tests/Module/HookRequirementsTest.php
    @@ -28,4 +30,49 @@ function testHookRequirementsFailure() {
    +   * Test that a module with uninstalled dependencies is not installable.
    

    Test -> Tests

11. +++ b/core/modules/system/src/Tests/Module/HookRequirementsTest.php
    @@ -28,4 +30,49 @@ function testHookRequirementsFailure() {
    +   * Test that a module with installed dependencies is installable.
    

    Test -> Tests

Regarding @see and references to classes/methods in docs, see:
https://www.drupal.org/node/1354#classes

"Immediately after an @tag (@param, @return, @ver, etc.), class and interface names must always include the fully-qualified namespace."

So. I guess we have not explictly said you cannot use self:: but we normally in @see do put in the FQNS/class::method().

Regarding my other comments, I agree we shouldn't document Composer but just saying "composer package name" would have at least alerted me that it was Composer jargon and not something specific to this function? And do we have a link somewhere to the composer docs you mentioned that explain what the package name is, maybe in the docs header for the class (not saying it should be on each method but a reference link would be helpful if we have a class in Drupal namespace that is dealing with this stuff)?

Yes, we can use @return static and @return $this. That is not the same as @see self::methodName(). It is a special case for @return, as described at
https://www.drupal.org/node/1354#types

Anyway. Whatever. api.drupal.org can actually handle @see self::methodName(). If IDEs can also handle it, that is OK. I'm not going to insist.

Just a comment: a class that is used to deny installation of a module, which says it is "naive" right in the doc block, is a bit scary! :)

Well it *is* naive. If it weren't, we'd require composer/composer and then we'd use composer's code to do the check. Totally OK with changing that docblock though. :-)

+++ b/core/lib/Drupal/Core/Composer/ExtensionDependencyChecker.php
@@ -0,0 +1,122 @@
+   * @see self::getInstalledPackages()

Return type can be static or $this without the FQNS for @return, but @see needs FQNS: https://www.drupal.org/coding-standards/docs#types and https://www.drupal.org/coding-standards/docs#see

I think #109 is valid, too: If we have contrib_module v.1.0 installed, and the update to v.2.0 now adds a composer.json file with unmet dependencies, then we should be blocked from doing an update. Maybe in update_check_requirements()?

VERY glad @alexpott clarified that. Sounds great.

I added a non-existent requirement to a contrib module and went to update.php on my dev site. It showed me this:

The documentation link sent me here: https://www.drupal.org/node/2627292

As @jhogdon pointed out, we should have a readable path for that instead of a node id, but that can be a follow-up.

RTBC from me, even though I wrote a little bit of it.

I aliased the page https://www.drupal.org/documentation/install/composer-dependencies if someone wants to update the patch.

I'm happy to change it. What would you suggest? I just named it similar to other pages in the section.

+1

FILE: ...ore/lib/Drupal/Core/Composer/ExtensionDependencyRequirements.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 3 WARNINGS AFFECTING 3 LINES
----------------------------------------------------------------------
 10 | WARNING | [x] Unused use statement
 11 | WARNING | [x] Unused use statement
 67 | WARNING | [x] A comma should follow the last multiline array
    |         |     item. Found:
    |         |     'https://www.drupal.org/documentation/install/composer-dependencies'
----------------------------------------------------------------------
PHPCBF CAN FIX THE 3 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------


FILE: ...al/core/modules/system/src/Tests/Module/HookRequirementsTest.php
----------------------------------------------------------------------
FOUND 4 ERRORS AFFECTING 4 LINES
----------------------------------------------------------------------
 37 | ERROR | [ ] Visibility must be declared on method
    |       |     "testComposerDependenciesFailure"
 67 | ERROR | [ ] Visibility must be declared on method
    |       |     "testComposerDependenciesSuccess"
 78 | ERROR | [x] The closing brace for the class must have an empty
    |       |     line before it
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------


FILE: ...ts/Drupal/Tests/Core/Composer/ExtensionDependencyCheckerTest.php
----------------------------------------------------------------------
FOUND 8 ERRORS AFFECTING 8 LINES
----------------------------------------------------------------------
 40 | ERROR | If the line declaring an array spans longer than 80
    |       | characters, each element should be broken into its own
    |       | line
 41 | ERROR | If the line declaring an array spans longer than 80
    |       | characters, each element should be broken into its own
    |       | line
 45 | ERROR | If the line declaring an array spans longer than 80
    |       | characters, each element should be broken into its own
    |       | line
 47 | ERROR | If the line declaring an array spans longer than 80
    |       | characters, each element should be broken into its own
    |       | line
 48 | ERROR | If the line declaring an array spans longer than 80
    |       | characters, each element should be broken into its own
    |       | line
 52 | ERROR | If the line declaring an array spans longer than 80
    |       | characters, each element should be broken into its own
    |       | line
 53 | ERROR | If the line declaring an array spans longer than 80
    |       | characters, each element should be broken into its own
    |       | line
 57 | ERROR | Missing short description in doc comment
----------------------------------------------------------------------


FILE: ...upal/Tests/Core/Composer/ExtensionDependencyRequirementsTest.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 42 | ERROR | Missing short description in doc comment
----------------------------------------------------------------------

Some code style issues that can be discovered using coder...

We shouldn't care about platform packages at all.

Quoting composer_manager:

/**
   * Removes platform packages from the requirements.
   *
   * Platform packages include 'php' and its various extensions ('ext-curl',
   * 'ext-intl', etc). Drupal modules have their own methods for raising the PHP
   * requirement ('php' key in $extension.info.yml) or requiring additional
   * PHP extensions (hook_requirements()).
   *
   * @param array $requirements
   *   The requirements.
   *
   * @return array
   *   The filtered requirements array.
   */
  protected function filterPlatformPackages($requirements) {
    foreach ($requirements as $package => $constraint) {
      if (strpos($package, '/') === FALSE) {
        unset($requirements[$package]);
      }
    }

    return $requirements;
  }

We don't want to include composer/composer unless we're willing to take on full responsibility.

For this issue, we only want to tell the user to do something outside of Drupal.

It's right to ignore platform requirements here, because if the platform requirements are unmet then the user couldn't install them through Composer anyway, so we should have uninstalled packages at the point we're checking for them.

(I think composer_manager *should* care about platform requirements, because it's taking on the responsibility of managing dependencies, but that's out of scope here.)

So here's a patch.

I agree with the logic in #129 and #130, back to RTBC it is.

This has been changed to 8.1.x in #88, but I think we should get it into 8.0.x, because it's a bug.

We've been discussing external code support improvements for search api solr and this issue is currently a blocker for us.

1. +++ b/core/lib/Drupal/Core/Composer/ExtensionDependencyRequirements.php
   @@ -0,0 +1,80 @@
   +        'description' => $this->t('Some modules have unmet Composer dependencies. Read the <a href="@documentation">documentation on drupal.org</a> on how to install them.', [
   +          '@documentation' => 'https://www.drupal.org/documentation/install/composer-dependencies',
   +        ]),
   

   I think this could be way more helpful. How we're not saying what module and what dependency? Also I think it probably should be one requirement fail per module.

   Also this would help what happens when you install a module missing a dependency as this fires before the module install code. At the moment when you install a module missing a dependency this is the message you see.

2. +++ b/core/modules/system/src/Form/ModulesListForm.php
   @@ -471,6 +472,13 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
   +      catch (ExtensionComposerRequirementsException $e) {
   +        drupal_set_message(
   +          $e->getTranslatedMessage($this->getStringTranslation()),
   +          'error'
   +        );
   +        return;
   +      }
   

   How can this exception catching ever be hit at runtime (and tested)? Requirements are tested before we get here.

@Xano well we still have the problem of installing several modules at once and not telling the user which one failed.

OK. 8.0.x it is, attempting to at least tell which module is failing the requirements test.

OK, we have helpful information about module names and their dependencies.

I may be being too much of a pedant here, but these aren't really Composer dependencies, are they? They're just PHP library dependencies that happen to be managed via Composer. (e.g. we wouldn't call other Drupal modules "Drush dependencies" or "Info file dependencies")

Thanks for the reviews.

I addressed everything in #140, #142 except a few things:

#140.3: Changed to getUnmetDependencies() because no one cares if it's calculated. :-)

#140.5: Re: t(). The string is entirely calculated. I'm not sure any translator is going to be translating composer package names.

Re: Human-readable names: We're working with extensions, so we'd have to check if it's a module before gleaning the human-readable name that way. Also, we have to be able to do this at install time, and it looks like system_get_info() needs a container (and thus a kernel). Maybe after #2208429: Extension System, Part III: ExtensionList, ModuleExtensionList and ProfileExtensionList

We really should come up with a way to encapsulate all the info file info into a model class like Extension. Like just read the info file if someone needs the human-readable name and there are no services. This patch should also do the same thing for composer.json files, adding it as a method on Extension rather than computing it during the check. Someone say yes and I'll add it.

Ewps. Forgot that test.

1. +++ b/core/tests/Drupal/Tests/Core/Composer/ExtensionDependencyCheckerTest.php
   @@ -0,0 +1,116 @@
   +      // No Composer dependencies.
   +      [TRUE, NULL, NULL],
   +      // No Composer dependencies, without any installed packages.
   +      [TRUE, NULL, '[]'],
   +      // No Composer dependencies, with an empty extension Composer file.
   +      [TRUE, '{}', '[]'],
   +      // One met dependency using different version constraints.
   +      [
   +        TRUE,
   +        '{"require": {"vendor/package":"1.0.0"}}',
   +        '[{"name":"vendor/package","version_normalized":"1.0.0"}]'
   +      ],
   +      [
   +        TRUE,
   +        '{"require": {"vendor/package":"~1.0"}}',
   +        '[{"name":"vendor/package","version_normalized":"1.0.0"}]'
   +      ],
   +      // One unmet dependency, without any installed packages.
   +      [FALSE, '{"require": {"vendor/not-installed":"1.0.0"}}', '[]'],
   +      // One unmet dependency, with another package installed.
   +      [
   +        FALSE,
   +        '{"require": {"vendor/not-installed":"1.0.0"}}',
   +        '[{"name":"vendor/package","version_normalized":"1.0.0"}]'
   +      ],
   +      // One unmet dependency using different version constraints.
   +      [
   +        FALSE,
   +        '{"require": {"vendor/package":"1.1.1"}}',
   +        '[{"name":"vendor/package","version_normalized":"1.0.0"}]'
   +      ],
   +      [
   +        FALSE,
   +        '{"require": {"vendor/package":"~1.1"}}',
   +        '[{"name":"vendor/package","version_normalized":"1.0.0"}]'
   +      ],
   +      // One unmet require-dev dependency, which is ignored, because it is not
   +      // required for normal site operation.
   +      [TRUE, '{"require-dev": {"vendor/package":"~1.1"}}', NULL],
   +      [
   +        TRUE,
   +        '{"require-dev": {"vendor/package":"1.1.1"}}',
   +        '[{"name":"vendor/package","version_normalized":"1.0.0"}]'
   +      ],
   +      [
   +        TRUE,
   +        '{"require-dev": {"vendor/package":"~1.1"}}',
   +        '[{"name":"vendor/package","version_normalized":"1.0.0"}]'
   +      ],
   +      // Platform dependencies are ignored.
   +      [
   +        TRUE,
   +        '{"require": {"php":">10.0"}}',
   +        '[{"name":"php","version_normalized":"1.0.0"}]'
   +      ],
   +    ];
   

   What about using keys for the data provider for better debugability?

2. +++ b/core/tests/Drupal/Tests/Core/Composer/ExtensionDependencyRequirementsTest.php
   @@ -0,0 +1,77 @@
   +    $dependency_checker = $this->getMockBuilder(ExtensionDependencyChecker::class)
   +      ->disableOriginalConstructor()
   +      ->getMock();
   +    $dependency_checker->expects($this->once())
   +      ->method('getUnmetDependencies')
   +      ->willReturn($unmet_dependencies);
   +
   +    $string_translation = $this->getMock(TranslationInterface::class);
   +
   +    $extension = $this->getMockBuilder(Extension::class)
   +      ->disableOriginalConstructor()
   +      ->getMock();
   

   Is there a reason we don't use prophecy? Prophecy is IMHO way less verbose and makes it easier to understand the test

There will come a time (er, well, now) when maintainers of modules will express their module dependencies in composer as well (http://cgit.drupalcode.org/pathauto/tree/composer.json), and this will soon be fully supported via the composer façade. Not sure if this is in or out of scope for this issue, but we're essentially providing two mechanisms for expressing dependencies - .info.yml and the composer.json. If there is a discrepancy between the two (in this case the versions are specific in the composer.json, but ambiguous in the info.yml file (http://cgit.drupalcode.org/pathauto/tree/pathauto.info.yml), then that should probably be handled as well.

composer.json only tells you what files should be in /vendor (or in /modules or whatever). We can query installed.json and figure out if that dependency is met, and prevent installing modules this way.

.info.yml only tells you which extensions should be enabled before a given extension can be enabled. We already have this check, and we check if the composer requirements are met before we check whether dependencies are enabled.

If a contrib module declares its dependency on a module in composer.json then that dependency will be unmet if it's not in installed.json, even if the module has been downloaded and placed in /modules. Using the pathauto link above as an example, the worst case scenario here is that someone already installed ctools without Composer, and then tried to install pathauto. They'd be prevented from installing pathauto because installed.json wouldn't reflect that ctools exists. Then they'd do composer install and potentially end up with two ctools in the file system, maybe even different versions.

There are a few ways to solve this for realsies:

1) Have Drupal poke around in installed.json when it installs modules. This is an unsustainable solution for a lot of reasons.

2) Have a way to install extensions under /vendor and then always do that. This would be a big change.

3) Require a Drupal-specific dependency management Composer plugin. This plugin could take on the responsibility of bridging the info.yml/json gap, but it would be a much less sustainable solution than #2. It could determine whether a given extension is in the filesystem and then add it to installed.json. This is similar to the embedded composer solution so many batted around before.

That's all out of scope here, of course, where we just want to tell the user to run composer if they need to. Implementing any of those other solutions would fix the problem outlined here.

#150 and #151 are out of scope and should be discussed somewhere else, this issue long enough as is. Let's keep it on topic.

The patch in #149 addressed all feedback, so that's great.

I have a few minor docs things that should be changed, but otherwise this has my vote of approval.

1. +++ b/core/lib/Drupal/Core/Composer/ExtensionDependencyChecker.php
   @@ -0,0 +1,144 @@
   +/**
   + * @file
   + * Contains \Drupal\Core\Composer\ExtensionComposerDependencies.
   + */
   

   Should be removed.

2. +++ b/core/lib/Drupal/Core/Composer/ExtensionDependencyRequirements.php
   @@ -0,0 +1,98 @@
   +   * @return array[]
   +   *   Requirements array, suitable for hook_requirements().
   

   Maybe we should add an @see to hook_requirements() so we link to the allowed keys for this array?

Just a couple of small bits ...

1. +++ b/core/includes/install.inc
   @@ -982,9 +983,22 @@ function drupal_requirements_severity(&$requirements) {
      $requirements = \Drupal::moduleHandler()->invoke($module, 'requirements', array('install'));
   -  if (is_array($requirements) && drupal_requirements_severity($requirements) == REQUIREMENT_ERROR) {
   +  // Make sure the requirements are an array, even if the hook invocation
   +  // returns NULL.
   +  if (!is_array($requirements)) {
   +    $requirements = [];
   +  }
   

   You know I would have just used $foo ?: []; and call it a day.

2. +++ b/core/includes/install.inc
   @@ -982,9 +983,22 @@ function drupal_requirements_severity(&$requirements) {
   +  $dependency_requirements = \Drupal::service('composer.extension_dependency_requirements');
   

   Can we put a inline /** @var */ just to be sure, and make it easier for people?

3. +++ b/core/lib/Drupal/Core/Composer/ExtensionDependencyChecker.php
   @@ -0,0 +1,144 @@
   +/**
   + * @file
   + * Contains \Drupal\Core\Composer\ExtensionComposerDependencies.
   + */
   

   @file is now dropped

4. +++ b/core/lib/Drupal/Core/Composer/ExtensionDependencyChecker.php
   @@ -0,0 +1,144 @@
   +   * @param string $drupal_root
   +   *   The path to the Drupal root directory.
   ...
   +  public function __construct($drupal_root) {
   +    $this->drupalRoot = $drupal_root;
   +  }
   

   In other places we called that app root. Feel free to change if you want

5. +++ b/core/lib/Drupal/Core/Composer/ExtensionDependencyRequirements.php
   @@ -0,0 +1,98 @@
   +        'description' => $this->t('The following extensions have unmet Composer-based dependencies: @extensions. Read the <a href="@documentation">documentation on drupal.org</a> on how to install them.', [
   ...
   +          '@documentation' => 'https://www.drupal.org/documentation/install/composer-dependencies',
   

   Note: we have the ":" placeholder for URLs these days

I'm tagging this as DX, since otherwise, many contrib modules will have to start implementing this on their own (for instance #2687197: Verify the composer managed dependencies using hook_requirements()).

+++ b/core/includes/install.inc
@@ -984,9 +985,23 @@ function drupal_requirements_severity(&$requirements) {
+  // Make sure the requirements are an array, even if the hook invocation
+  // returns NULL, because the module does not implement hook_requirements().
+  if (!is_array($requirements)) {
+    $requirements = [];
+  }

I think @dawehner was suggesting in #153 that this can be removed and simplified to

$requirements = \Drupal::moduleHandler()->invoke(...) ?: [];

RTBC + 1

+++ b/autoload.php
@@ -11,4 +11,11 @@
+if (!defined('DRUPAL_COMPOSER_VENDOR_DIRECTORY')) {
+  define('DRUPAL_COMPOSER_VENDOR_DIRECTORY', 'vendor');
+}

Maybe we want to define this as absolute path as well?

Using __DIR__ . '/vendor'?

+1 on #175.

+++ b/autoload.php
@@ -11,4 +11,11 @@
+/**
+ * Defines the path to the Composer vendor directory.
+ */
+if (!defined('DRUPAL_COMPOSER_VENDOR_DIRECTORY')) {
+  define('DRUPAL_COMPOSER_VENDOR_DIRECTORY', 'vendor');
+}
+
 return require __DIR__ . '/vendor/autoload.php';

Use the constant here instead of rebuilding the path without it. That way other systems can just set the constant.

We'll write a Composer script that stores a list of installed packages during composer install.

Why? (Fergawdsakes...)

Composer does this already. It's in a file called ./vendor/composer/installed.json.

See ExtensionDependencyChecker->getInstalledPackages() in the patch.

It's the basis of the solution to this issue.

Composer does this already. It's in a file called ./vendor/composer/installed.json.

See ExtensionDependencyChecker->getInstalledPackages() in the patch.

It's the basis of the solution to this issue.

Composer does this, but installed.json doesn't have a schema so we're not sure if we can depend on that file always having the same structure.

@borrison_ OK, that makes a little bit of sense, but here we have a fork in the road.

On one side, we have the current patch, which trusts installed.json, which means that if it stops working we have to patch it and make it better. One premise of this approach is that we then don't have to require Composer itself as a dependency, because no one really wants to require Composer.

On the other side, we have a proposal to write a Composer plugin which duplicates the behavior of Composer. This is based on the idea that at some point in the future, the most widely-used package manager in the PHP universe will change the way installed.json files work. It also means we get to maintain a set of code which doesn't do anything we couldn't accomplish by simply requiring Composer as a dependency and then asking it what it has installed at runtime. This is a use-case that Composer wasn't designed for. You'll recall that the problem we're trying to solve here is whether Composer has been used to install something, which might not even be a requirement for a given site.

I think the former is better, because it's smaller, less of a hassle, doesn't re-implement an external library, and accomplishes one goal effectively.

I can understand the concern with there not being a schema, and installed.json isn't static, and has changed over time (as a look at the Blame for ArrayDumper, the part of composer that translates the Package interface into json: https://github.com/composer/composer/blame/346133d2a112dbc52163eceeee678...). If something were to change with the schema that was incompatible with our expectations, that would break *existing* drupal installs, and yes, we could patch, but every site out there would be unable to leverage this feature until they do.

On the other hand, our expectations are modest - we're not dependent upon the entire comprehensive schema of installed.json. It looks like here:

+++ b/core/lib/Drupal/Core/Composer/ExtensionDependencyChecker.php
@@ -0,0 +1,140 @@
+    // Parse the list once and then cache it.
+    $json = file_get_contents($installed_json_path, FALSE);
+    $this->installedPackages = [];
+    foreach (json_decode($json) as $package) {
+      if (isset($package->name) && isset($package->version_normalized)) {
+        $this->installedPackages[$package->name] = $package->version_normalized;
+      }
+    }
+
+    return $this->installedPackages;

we're only reliant on two things: That installed.json is an array at the top level of packages, and that packages have a version_normalized key.

I think that the likelihood that those two things would change are very low, and thus its a tolerable risk to assume we can rely on installed.json not changing out from underneath us.

If we do decide that the risk is too great, we do not actually need #2474007: Add a “General” project type to Drupal.org to take place (especially since thats not actually going to happen anytime soonish). Since it would be a package, it could be placed on github, and included in composer.json like all the other packages we currently require. If it needs to be more official than that, it can live in the drupal/ namespace on github.