Have wikimedia/composer-merge-plugin manage contrib dependencies.

This change slightly depends on how modules are stored

+++ b/composer.json
@@ -23,7 +23,9 @@
-        "core/composer.json"
+        "core/composer.json",
+        "modules/*/composer.json",
+        "sites/*/modules/*/composer.json"

I'm using modules/contrib/*/composer.json and sites/all/modules/contrib/*/composer.json

Would this be autoloading code for modules that are not even installed?

We would have to consider not just sites/*/modules/contrib, but, sites/*/modules/custom, and any arbitrary depth. I guess that could be up to the devs to put in the right paths that need autoloading, but it's another manual step.

If adding /contrib/ in there is a common pattern for another tool, then we can add it here as a pattern.

This has been a best-practice from at least the D6 days. See Commerce Kickstart, Panopoly, and Open Atrium distributions' drush make files for instance.

So +1 for supporting contrib.

I'm not sure we need to specifically take care of '/contrib/'. If we leave it out and scan the whole of modules/ and sites/*/modules, then it means custome modules are handled as well, which seems like the desired behavior ?

This means that custom modules specify their deps in a composer.json of their own, just like contrib modules, and that's IMO the correct practice :
- a module can start as custom and be later contributed
- a contrib module can (even temporarily) get committed in the local site git repo as a "custom fork", for example if too heavily patched, waiting for the patches to get officially committed

Having contrib and custom modules handled the same way is conceptually simpler and ensures you can seamlessly move from one to the other.

@Mile23 : hm, right, the wikimedia plugin doesn't scan subfolders.

- Having to point the exact folder that contains modules ('modules/*/composer.json' or 'modules/contrib/*/composer.json' / 'modules/custom/*/composer.json') is an annoying limitation.
- Along with the fact that it will never catch composer.jsons of submodules.

Our modules discovery system is based on a recursive scan of subfolders. I'd think an auto-discovery of modules composer.jsons should follow the same logic, or we face inconsistencies and weird behaviors.

That might mean extending/forking our own version of the wikimedia plugin, or maybe just adding a preliminary step in a separate plugin of our own that does the scan discovery and dynamically adds the corresponding glob paths at runtime before the wikimedia plugin does its thing ?

Also, re: @Dave Reid #8 :

Would this be autoloading code for modules that are not even installed?

Yes, that's the normal behavior. Regardless of "drupal modules", any time you 'composer require' a package, all its deps are downloaded and added to the autoloader, even if no code in your app actually uses the package yet.
For e.g a Symfony bundle in a Symfony app, the bundle and all its dependencies are added to the autoloader even if the actual code of your app doesnt 'enable' the bundle.

That's kind of tied to an implicit assumption that if you require a package, you intend to use it at some point, so we make it ready for you to use. That is a reasonable assumption most of the times, but our practice of submodules (optional subpackages shipped in a package...) is a bit at odds with that...

Another problem with wildcard approach is a confusion when same module lives in /modules and sites/{somesite}/module - I mean that composer could process inclusion differently from core

when same module lives
in /modules and sites/{somesite}/module

Ouch, that seems difficult to support if those are different versions with different deps. Multisites share one single set of dependencies in one single vendor dir... One site cannot jump in and say "nope I'll use a different version of that package".

I guess we should merge all the same, across all sites, and let composer try to resolve to a satisfying version of each package, and fail as usual if the requirements are conflicting ?

hm, makes sense... looks no other way
so only remaining question about abusing autoloader with disabled modules, I think that performance related

so only remaining question about abusing autoloader with disabled modules, I think that performance related

As explained in #17, I don't think this can be avoided. If a module is present in the codebase, enabled or not, its deps are present in the autoloader... (not the classes in the module's own src/ though, that is still controlled by the module being enabled in the drupal sense)

I don't think we want to track which module requires which package and only dynamically add those to the autoloader only if the module is enabled, that sounds like an awful can of worms...

This issue is truly a terrible idea :)

It basically adds composer_manager to core (between RC3 and RC4?) while also breaking the sites of people who use Composer to install Drupal modules.

composer_manager has to do the following flow:
1) Use ExtensionDiscovery to find modules
2) Checks the discovered modules against Composer's installed.json to exclude any installed via Composer directly
3) Add the discovered modules to the merge list, as well as the replace list, so that they aren't redownloaded because another module specifies them in its requirements.

We'd need to do all three for this issue to actually work. There's no point, since we actually want to tell people to use Composer directly, leaving the composer_manager flow for edge cases (such as testing, local dev, etc).

There's nothing RC eligible about this, the breakage introduced just wasn't forseen.

As I've documented before, there are currently two ways to approach getting a module's dependencies:
1) A module is already present on disk, and now its dependencies need to be brought in with Composer.
This is what composer_manager assumes, and what this patch assumes.

2) The module is downloaded via Composer, and its dependencies are fetched along the way. This includes libraries and other Drupal modules.
This is the "Use Composer as Drush Make" pure approach that requires no additional modules. This is what we've been working towards for the past year.

Core has no business mandating #1. Furthermore, it was never the desired outcome.

Your comments on composer_manager make me believe you last read its source code a year ago. Especially since you are keen to run into bugs we've already fixed there. The logic consists of around 200 lines of code, centered around the PackageManager class. Give it a look.

The technical problems with the patch:
- This module discovery does not match the core discovery.
A module can be found by Drupal, but not by merge plugin based on the specified paths.
Install profiles are excluded, and specific module subdirectories (contrib, custom) are mandated, which is a big change in itself.

- The merged extensions are not added to the "replace" list (the merge plugin doesn't do it).
So if you've downloaded Address, it got merged, then you decided to add Commerce, which has a "drupal/address" requirement, then Address will be redownloaded and replaced.

However, once you start maintaining the replace list, you get the main issue:

If drupal/commerce depends on drupal/entity then Composer will download the Entity module and place it in the appropriate place.
Let's say that on the next Composer run the merge plugin discovers drupal/entity, merges it in, and adds it to the replace list.
This confuses Composer (having the same module in installed.json and in the replace list), and the module gets removed.

The only workaround is to tell modules not to require other modules in the require list.
Or do what composer_manager does and skip adding extensions present in installed.json to the merge list.

So, here are the pros and cons of the current patch:
Pros:
- No need to download composer_manager.

Cons:
- Can't use Composer as a Drush Make alternative anymore. In fact, the whole Drupal Packagist becomes pointless.

rc eligible: This tag is for non-invasive patches which solely make coding standards, testing, and docs improvements.

Regardless of if this is 8.1.x or 8.0.x, it certainly is not 'rc eligible'.
You can try for 'rc target triage' if you think breaking BC is worth it.

@Mile23 thanx for working on that but... this change is pointless.
The main reason for me is abusing autoloader, then this is no-go for shared hostings (limit for opcacher is 100k files and inodes limit) and last
you will need to add own paths anyway if your folder structure of project different.

Also +100500 to what #28 said after "The technical problems with the patch"

+++ b/composer.json
@@ -23,7 +23,13 @@
+        "modules/*/composer.json",
+        "modules/*/contrib/*/composer.json",
+        "modules/*/custom/*/composer.json",
+        "sites/*/modules/*/composer.json",
+        "sites/*/modules/*/contrib/*/composer.json",
+        "sites/*/modules/*/custom/*/composer.json"

@@ -36,7 +42,6 @@
-    "pre-autoload-dump": "Drupal\\Core\\Composer\\Composer::preAutoloadDump",

That's all what the patch does

Not sure if this on topic, or a new issue would be needed but I think another use case for something similar to this (non composer manager related) is to have a composer.local.json file, for example to avoid modifying core's composer.json file and use the local json file instead for the "composer as drush make" approach.

This way, when the core main composer.json file changes, you would not have the file out of sync.

        "merge-plugin": {
            "include": [
                "core/composer.json",
                "composer.local.json"
            ],
            "recurse": false,
            "replace": false,
            "merge-extra": false
        }

@pcambra
The root composer.json is not core's, it's yours, and it's expected to be modified/
Core's is the one in core/composer.json.

Now that #2758737: Add a packages.drupal.org to root composer.json is in, is there any point to this issue?

I really dont think that using wikimedia merge is a good idea for contrib modules. It might have been in an era when people would use tarballs to place modules into folders, but the best practice (probably the only practice) with composer is to composer require a module to get its dependencies.

My suggestion is we close wontfix.

Agreed.