Stop using wikimedia/composer-merge-plugin

Woot!

Step 1: #2578485: Composer::preAutoloadDump fails with no specified classmap

Step 2: Remove the merge plugin.

Step 3: Account for Component composer.json files some other way than #2867960: Merge Component composer.json files to account for them during build

Step 4: Realize we don't have a good way to dev, because composer install will overwrite changes.

Bonus points: #2385395: Make Drupal core folder agnostic and allow it to be placed in vendor/drupal/core

From working with people on Windows and even some environments with unusual production constraints (never followed up on that, but it happens), symlinks are really an issue, so anything we do should also work without symlinks, typically using just copy/move operations instead.

Now this appeared.

The plan with the composer initiative is that we use the current codebase for core dev, so we'll likely keep the merge plugin for the drupal/drupal composer.json file.

Then when you actually build a Drupal installation, you'll probably use at least drupal/drupal-project-legacy (see #2982680-2: Add composer-ready project templates to Drupal core) which will not use the merge plugin.

And of course other projects like drupal-composer/drupal-project have already omitted the merge plugin.

I like the merge plugin, I really do, but I am starting to see it as a technical dept because it seems quite unmaintained and I am unable to reach its maintainer it for a year. It does not support proper highest-lowest testing at this moment. https://github.com/wikimedia/composer-merge-plugin/pull/169

Working on removing usages of the merge plugin for Components here: #2943842: Allow component namespaces to be autoloaded independently

Here's a patch that removes the wikimedia merge plugin and obviates the need for #2943842: Allow component namespaces to be autoloaded independently. This technique came from experiments in #3067645: Add core scaffold assets to drupal/core's composer.json extra field that built on the patch from #2943842.

Things of note:

- The wikimedia plugin is removed, and drupal/core is installed as a path repository instead.
- The Drupal Components in druapl/core are no longer maintained as separate Composer projects. They are instead replaced in drupal/core. A psr-4 autoload entry has been added to drupal/core so that the classes for these components can still be found. These two changes require that the dependencies for each component be copied into the require section of drupal/core. This is in alignment with how Symfony manages their monorepo. Note that the Drupal Components are still split out into separate projects by the subtree split tool in drupal.org infrastructure, but they are not used by drupal/core.
- The Drupal Scaffold component is included also as a path repository in drupal/core. It is not replaced. When using the drupal/drupal repository, the Scaffold component in place. When using something like drupal-composer/drupal-scaffold, though, then the subtree split version is installed to the vendor directory, and that copy is used. The copy in drupal/core is unused in this scenario. It might make sense to move the Composer plugins (Scaffold and Vendor cleanup, in progress) outside of drupal/core.
- The type of drupal/core was changed from drupal-core to drupal-core-type. Changing the type was necessary because composer/installers has a default rule to relocate drupal-core to the core directory. If drupal/core is already being installed as a path repository whose source is at core, then Composer complains that the project cannot be copied over itself.
- The dev dependencies from drupal/core have been moved to the root composer.json, because require-dev is only parsed from that location now that the merge plugin is no longer used.

Tests pass, except for the nightwatchjs thing at the end, which also showed up in #3067645: Add core scaffold assets to drupal/core's composer.json extra field. Not sure if this is caused by this patch, or something unrelated. Seems related to the errors above about the bash commandline being too long for some of the setup / cleanup operations for the tests.

Here's a better way to manage the path repository for drupal/core without having to change the type from drupal-core.

Wow, this is super cool! What a coincidence, I was also experimenting with the monorepo vs. Drupal topic this week.

These two changes require that the dependencies for each component be copied into the require section of drupal/core. This is in alignment with how Symfony manages their monorepo.

One of the biggest challenges of Composer monorepos is keeping the commonly used dependencies on the same version. Is there a plan to add some kind of automated validation/script to Drupal that could ensure this?

https://github.com/Symplify/MonorepoBuilder this is a tool by Tomas Votruba that could help to solve this issue. He also wrote quite some blog posts about the monorepo topic: https://www.tomasvotruba.cz/clusters/#monorepo-from-zero-to-hero

In the case of projects that are using drupal/core, the other components are require'd using self.version. I believe that in this scenario, there will not be any problem keeping the versions in alignment. Versioning mis-alignment may happen for projects that are using components split out from drupal/core without using drupal/core itself. For projects that are lightweight Drupal sites, this could be solved by making a new metapackage that is a subset of drupal/core and tagged with aligned versions, so that self.version can continue to be used. For projects that are not Drupal sites (re-using Drupal components in other contexts), I do not think that version mis-alignment will often be a large problem, as our current components are fairly isolated. I have not considered the use-case of re-using the 'core' subprojects in a non-Drupal context; perhaps this is not a use-case.

We already have a repo splitter running on drupal.org. The tools you reference might be useful in the future if there is some reason to change the existing infrastructure. I don't think we have a strong need to do that at this point.

Oh, and as for the non-Drupal dependencies, yes, we are planning on making something akin to webflo/drupal-core-strict for Drupal core.

Rolling back a couple of dependencies that were updated by the previous patch, but did not need to be. Hoping this will improve the test results.

Patches on unrelated issues are still passing, so the problem here with nightwatch seems to be caused by the changes here. I am a loss as to why that should be, though.

OK, I have a theory. In tests/Drupal/Nightwatch/nightwatch.conf.js, we search for files matching the path pattern '**/tests/**/Nightwatch/**/*.js'. In drupal/core, we have the following matches:

./core/tests/Drupal/Nightwatch/globals.js
./core/tests/Drupal/Nightwatch/Tests/exampleTest.js
./core/tests/Drupal/Nightwatch/Tests/loginTest.js
./core/tests/Drupal/Nightwatch/Tests/statesTest.js
./core/tests/Drupal/Nightwatch/nightwatch.conf.js
./core/tests/Drupal/Nightwatch/Commands/drupalLogin.js
./core/tests/Drupal/Nightwatch/Commands/drupalUserIsLoggedIn.js
./core/tests/Drupal/Nightwatch/Commands/drupalCreateRole.js
./core/tests/Drupal/Nightwatch/Commands/drupalLogAndEnd.js
./core/tests/Drupal/Nightwatch/Commands/drupalInstall.js
./core/tests/Drupal/Nightwatch/Commands/drupalUninstall.js
./core/tests/Drupal/Nightwatch/Commands/drupalLoginAsAdmin.js
./core/tests/Drupal/Nightwatch/Commands/drupalLogout.js
./core/tests/Drupal/Nightwatch/Commands/drupalRelativeURL.js
./core/tests/Drupal/Nightwatch/Commands/drupalCreateUser.js
./core/tests/Drupal/Nightwatch/Pages/TestPage.js

Presumably we search in directories beyond `core` because we want to allow contrib modules to add Nightwatch tests.

This patch introduces a vestigial copy of drupal/core in the vendor directory (symlinked to /core). This is normal behavior for path repositories. It appears that this test suite is finding both copies of the core directory, and is therefore adding the Drupal/Nightwatch/Commands twice.

Would it be reasonable to exclude `vendor` from the Nightwatch search path? We do not want to run Nightwatch tests on non-modules / other vendor content that happen to have Nightwatch tests, do we?

Updated patch to ignore 'vendor' directory in Nightwatch.

- Remove unneeded polyfills.
- Move Components that were 'required' back to 'replace'. Only the Scaffold component must be required; the others should be replaced.

Not so much a review as maybe an explainer.

1. +++ b/composer.json
   @@ -5,10 +5,23 @@
   +        "drupal/core": "self.version"
   
   @@ -59,6 +64,16 @@
   +        {
   +            "type": "path",
   +            "url": "core",
   +            "options": { "symlink": true }
   +        },
   

   This is OK because drupal/drupal will be the dev-only package for core.

2. +++ b/composer.json
   @@ -5,10 +5,23 @@
   +    "require-dev": {
   +        "behat/mink": "1.7.x-dev",
   +        "behat/mink-goutte-driver": "^1.2",
   +        "behat/mink-selenium2-driver": "1.3.x-dev",
   +        "composer/composer": "^1.8",
   +        "drupal/coder": "^8.3.2",
   +        "jcalderonzumba/gastonjs": "^1.0.2",
   

   These would be more properly called the dev dependencies of drupal/core. But we can't leave them there because dev dependencies are only added for the root project, so we put them here.

   The plan is to use this set of dev dependencies to generate a metapackage that people can use in their own projects to enable them to run tests, similar if not identical to https://github.com/webflo/drupal-core-require-dev

My first once over.

+++ b/composer.json
@@ -24,16 +37,8 @@
         "installer-paths": {
-            "core": ["type:drupal-core"],
+            "vendor/drupal/core": ["type:drupal-core"],

+++ b/composer.lock
@@ -5770,26 +5962,12 @@
-    "platform": {
-        "ext-date": "*",
-        "ext-dom": "*",
-        "ext-filter": "*",
-        "ext-gd": "*",
-        "ext-hash": "*",
-        "ext-json": "*",
-        "ext-pcre": "*",
-        "ext-pdo": "*",
-        "ext-session": "*",
-        "ext-simplexml": "*",
-        "ext-spl": "*",
-        "ext-tokenizer": "*",
-        "ext-xml": "*",
-        "php": ">=7.0.8"
-    },

+++ b/core/composer.json
@@ -51,46 +52,7 @@
-    "require-dev": {
...
-        "drupal/action": "self.version",
-        "drupal/aggregator": "self.version",
-        "drupal/automated_cron": "self.version",
-        "drupal/bartik": "self.version",
-        "drupal/ban": "self.version",
-        "drupal/basic_auth": "self.version",
-        "drupal/big_pipe": "self.version",
-        "drupal/block": "self.version",
-        "drupal/block_content": "self.version",
-        "drupal/block_place": "self.version",
-        "drupal/book": "self.version",
-        "drupal/breakpoint": "self.version",
-        "drupal/ckeditor": "self.version",
-        "drupal/classy": "self.version",
-        "drupal/color": "self.version",
-        "drupal/comment": "self.version",
-        "drupal/config": "self.version",
-        "drupal/config_environment": "self.version",
-        "drupal/config_translation": "self.version",
-        "drupal/contact": "self.version",
-        "drupal/content_moderation": "self.version",
-        "drupal/content_translation": "self.version",
-        "drupal/contextual": "self.version",
         "drupal/core-annotation": "self.version",
         "drupal/core-assertion": "self.version",
         "drupal/core-bridge": "self.version",

@@ -110,11 +72,33 @@
         "drupal/core-proxy-builder": "self.version",
         "drupal/core-render": "self.version",
         "drupal/core-serialization": "self.version",
-        "drupal/core-composer-scaffold": "self.version",
         "drupal/core-transliteration": "self.version",
         "drupal/core-utility": "self.version",
         "drupal/core-uuid": "self.version",
         "drupal/core-version": "self.version",
+        "drupal/action": "self.version",
+        "drupal/aggregator": "self.version",
+        "drupal/automated_cron": "self.version",
+        "drupal/bartik": "self.version",
+        "drupal/ban": "self.version",
+        "drupal/basic_auth": "self.version",
+        "drupal/big_pipe": "self.version",
+        "drupal/block": "self.version",
+        "drupal/block_content": "self.version",
+        "drupal/block_place": "self.version",
+        "drupal/book": "self.version",
+        "drupal/breakpoint": "self.version",
+        "drupal/ckeditor": "self.version",
+        "drupal/classy": "self.version",
+        "drupal/color": "self.version",
+        "drupal/comment": "self.version",
+        "drupal/config": "self.version",
+        "drupal/config_environment": "self.version",
+        "drupal/config_translation": "self.version",
+        "drupal/contact": "self.version",
+        "drupal/content_moderation": "self.version",
+        "drupal/content_translation": "self.version",
+        "drupal/contextual": "self.version",

+++ b/core/lib/Drupal/Component/Diff/composer.json
@@ -5,8 +5,7 @@
-    "php": ">=7.0.8",
-    "symfony/polyfill-mbstring": "~1.0"
+    "php": ">=7.0.8"

+++ b/core/lib/Drupal/Component/Utility/composer.json
@@ -6,10 +6,8 @@
-    "paragonie/random_compat": "^1.0|^2.0",
     "drupal/core-render": "^8.2",
-    "symfony/polyfill-iconv": "~1.0",
-    "symfony/polyfill-mbstring": "~1.0"
+    "symfony/polyfill-iconv": "~1.0"

+++ b/core/tests/Drupal/Nightwatch/nightwatch.conf.js
@@ -10,13 +10,14 @@ const collectedFolders = {
+const defaultIgnore = ['vendor/**'];
 
 glob
   .sync('**/tests/**/Nightwatch/**/*.js', {
     cwd: path.resolve(process.cwd(), `../${searchDirectory}`),
     ignore: process.env.DRUPAL_NIGHTWATCH_IGNORE_DIRECTORIES
-      ? process.env.DRUPAL_NIGHTWATCH_IGNORE_DIRECTORIES.split(',')
-      : [],
+      ? process.env.DRUPAL_NIGHTWATCH_IGNORE_DIRECTORIES.split(',').concat(defaultIgnore)
+      : defaultIgnore,

Re-reading some comments on here (specifically #4): how well do path repositories work with 'symlink: true' and windows? I guess we may have to figure out how to test this there. Unless, we do not support core development on windows, for example.

Nevermind. Composer docs answer this for me: from https://getcomposer.org/doc/05-repositories.md#path

Note: On Windows, directory symlinks are implemented using NTFS junctions because they can be created by non-admin users. Mirroring will always be used on versions below Windows 7 or if proc_open has been disabled.

+ "vendor/drupal/core": ["type:drupal-core"],
This is the only change Im wondering about/don't understand. What does this do?

Composer/installers has a default rule to relocate drupal-core to the core directory. If we do not override the default rule and put drupal/core back in vendor, then Composer will try to copy /core over /core, and will reject the notion out of hand (failed operation). Since drupal/core is a path repository, we end up with an unused symlink to /core in vendor/drupal/core.

These look like they still disappeared from the lock file. I think they should still be in it. Do we just need a lock update again?

The platform reqs are in the "require" section of drupal/core, which is where they have always been. I think this is where they belong, and they are correctly copied into the require section of composer.lock. The "platform" section of composer.lock disappears with this patch, but I think that the fact that it was there at all was a wikimedia bug. The platform section means "pretend I have this, whether I do or not". Require means "I need this". We want the later, not the former.

Looks like we reordered the components to be before the modules out of alphabetical order.

Re-alphabetized the require section (except for "bartik" and "minimal", which were out-of-order, so I left them out of order).

These don't really block this patch, so they should probably just go into a separate issue. They're trivial enough, but risk is risk.

Sure, I put those dependencies back in their respective components.

how well do path repositories work with 'symlink: true' and windows?

Despite your other comments, I think things are probably better for Windows if we let Composer do its default behavior, which is to favor symlinks.

This, on the other hand, does block us, so, not sure if we want that to be a separate issue or not.

I don't think that this should be a separate issue, because the problem fixed here is not evidenced until the core symlink in vendor is placed by this patch.

Does core-composer-scaffold have to be path repo'd and require'd from drupal/drupal because its a composer plugin, or because of the nature of what it does?

In other words, will the vendor cleanup plugin also require that same treatment?

Reviewed the latest, this is hawt. Im very happy that this works and that we can eliminate this merge plugin. This will be an unexpected performance boost for a lot of folks. The merge plugin was *s*l*o*w*.

Some questions regarding this:

1. What impact will this have on existing core checkouts if this gets committed? Is it just 'git pull;composer install' to get the latest?
2. Do any core comitters have scripts or tools that do things like update composer.lock files etc that might need warning/changes etc?
3. Will it affect drupal infra packaging - wait, I know this one. It will not. This will have no impact.
4. Do any tools like drush or drupal console rely on or expect or need the current layout for any reason?

The Scaffold component must be required, not replaced, because it is a Composer plugin. The Vendor Cleanup plugin will require the same treatment.

Just `git pull; composer install` will be sufficient to get existing git users caught up. (If not, then the test runner wouldn't be able to set up the SUT). Also, `git pm-update` will be sufficient to get existing tarball users updated (once there is a tagged release).

Once this is RTBC'ed, we should ensure that it gets a lot of exposure among core committers to find out if there are any scripts or other processes that might be affected by this change.

I am not aware of any Drush or Drupal Console changes that will be needed as a result of this patch.

The component that builds drupal-core-require-devwill need to be updated to account for the new location where the 'require-dev' components have been moved to. See webflo/package-generator-drupal/#2.

That's all that I am aware of at this time.

What's the workflow for working with contributed modules against a git checkout of core and contributed modules? Today, I use the merge plugin to include the composer.json from any contributed modules that have them.

@greg.1.anderson Re: #16 - #16

Oh, and as for the non-Drupal dependencies, yes, we are planning on making something akin to webflo/drupal-core-strict for Drupal core.

Not sure what are the plans regarding this, but I am leaving a reference here to our other conversion on Github :)
https://github.com/drupal-composer/drupal-project/issues/477#issuecommen...

Also, what I actually wanted to ask when I reacted:

One of the biggest challenges of Composer monorepos is keeping the commonly used dependencies on the same version.

to your comment about:

These two changes require that the dependencies for each component be copied into the require section of drupal/core.

is what is a plan for keeping subpackage's 3rd-party library version requirements in sync with what has been merged (copied) to the parent package's composer.json now? Because subpackages in the monorepo should still define their dependencies and if a dependency version need to be changed or a new dependency needs to be added to a subpackage then the parent package's composer.json should also reflect that change and it should ensure that two packages that requires the same 3rd party library also requires the same minimum/maximum version from it.

This version mismatch problem is what https://www.drupal.org/project/drupal/issues/2876669 and it child issues tries to eliminate.

is what is a plan for keeping subpackage's 3rd-party library version requirements in sync with what has been merged (copied) to the parent package's composer.json now?

My comment that you quoted is out of date. Only the Composer plugins (Scaffold and Vendor Cleanup) need to be `require`d. The current version of this patch `replace`s all of the other components.

What's the workflow for working with contributed modules against a git checkout of core and contributed modules?

With this patch, you could do it the Composer way: require your contrib module (into the root composer.json) as dev-master. This will cause Composer to check out the contrib module from git. You may then edit it in place, check out a different branch, and push changes you make back up to the original repository.

If you really wanted to, you could of course re-add the wikimedia merge plugin and use it in your projects just for contrib module development. The only reason to do so, though, would be if you were really used to the workflow. I think that the Composer way is generally superior.

Finally, it has been our assumption that usually folks will be doing either core development or contrib development, but not both at the same time. When doing contrib development, it will generally be more convenient to use one of the template projects to get started. The only real difference between using a template project vs. doing core and contrib development at the same time, though, is that you own the root composer.json when using a template project. If you have required a contrib module into drupal/drupal's composer.json file, you need to be careful not to include your diffs in any patch you make. That's left as an exercise for the reader, though.

ensure that two packages that requires the same 3rd party library also requires the same minimum/maximum version from it.

Thats not entirely true. The components that exist inside of drupal are intended to be used independent of drupal entirely, and as such can have much more relaxed requirements than what drupal needs. We *do* need to ensure that the components have proper dependency expressions, and we currently have an issue where a component could be updated to a new dependency in core, but fail to update that in the components composer.json.

The solution to that problem is to allow for the components to be tested in isolation. (https://www.drupal.org/project/drupal/issues/2943856) That way if core changes an underlying assumption, it will have to change it in the component in order to get the tests to pass, and thus we dont run into any issues where there is a version incompatiblity when running core, or running a component separately.

I believe we've tested, scoured, and polished this patch into a pleasant state thats ready for committer review.

Thanks for the details. +1 from me, as I don't think the below assumption actually matters any more, but my thoughts in case it does.

Finally, it has been our assumption that usually folks will be doing either core development or contrib development, but not both at the same time.

This assumption doesn't hold for the teams I'm on. Typically our client sites are on a stable version of Drupal, and we'll be have a dev release of core in parallel (even if it's just the stable release branch) for the contrib side of things. The idea is that we get testing against a tag "for free" when we pull the contrib updates into the site. If we have a project with a longer timeline that goes past the next stable release, we'll often start with the latest dev branch, as we may know we need or want new features. There's also the case where a contrib or custom module requires a patch to core, though over time D8 has made that less and less of an issue.

Oh, and to clarify, I have this a static review and the patch looks good too.

Perhaps you might be confusing "core development" with "having a dev version of core" in #38.

Core development is making changes to core and submitting patches to drupal itself. If you're building a client site, thats unlikely to be either core development or contrib development.

Having a -dev version of core will still be doable, its just that you wont use 'git clone' to get it, you'd use `composer create-project drupal/legacy-core:8.7.x-dev` which would build, scaffold, and give you an 8.7.x-dev version of drupal. If you wanted the git repos for those you can use --prefer-source.

My 5c - if core will got split on dev/no-dev (maybe tests will be removed) it could have bigger impact on how we apply patches to client projects - most of patches has tests and they will not apply on "no-dev" version of core.
I think this is main issue with this all changes - totally not clear how to maintain 2 (or more) patchsets

@andypost if you use `composer create-project --prefer-source drupal/legacy-core:8.8.x-dev` as @mixologic suggests, then you should get the same project layout as the git clone + composer install version, and you will also have a composer.json that you can add your modules to without losing the test directories.

I guess it is still a question whether the tests should be removed in --prefer-dist mode. Since tarballs today include the test folders, I would presume that we would continue to add them to dist downloads. If we do that, then the file layout should remain the same, and patches should continue to apply (provided that they do not change the root composer.json file et. al., which should be rare).

"no-dev" does not mean "no tests" - however there is discussion about removing tests over in #3067979: Exclude test files from release packages

The current cweagans/composer-patches works for drupal/core because everything in core ends up under the /core folder, and thus the patches still apply.

In the future where the components no longer live inside of /core, and end up in vendor, a patch that touches those in the core repo wouldn't apply to a client site. We'd have to figure out something else to make that work.

Rewrote issue summery using standard issue template.

Changing parent issue. I think it's safe to say this issue is part of the composer initiative now, and does support the goal of eventually building the tarball project.

Adding needs CR tag because this is a bit of a re-arrange, and we should note the changes there, including how various dev workflows might operate.

Leaving RTBC because the sooner the better. :-)

We still need a change record here. So marking for needs work for that.

Are we concerned about impact on existing sites? There is at least one interesting thing that happens if you apply this patch. Run composer install and revert the patch and run composer install - you lose your core directory!!!!

@alexpott Thank you for finding the issue with the removal of the core directory. Applying and un-applying this patch is probably going to become a fairly common operation, as that will effectively happen any time someone switches from the 8.8.x branch back to the 8.7.x branch (or earlier). That will likely happen now and again after this is committed when core developers backport patches. Site developers may have occasion to want to run tests on newer / older versions of Drupal core occasionally as well.

I looked into this a little bit and determined that the reason it happens is that the path repository for the 'core' directory is written into vendor/composer/installed.json. When this patch is reverted, there is no longer a definition for the path repository. As a result, Composer thinks that the core directory was installed from a package and deletes it. The workaround is to remove the vendor directory after revering this but before running composer install.

I'll write up a changelog entry and include an explanation of this.

The draft CR looks fabulous, and articulates everything I think we're anticipating.

Just been thinking about the impact of this on custom modules and their composer dependencies. At the moment this change will be breaking the advice given on https://www.drupal.org/docs/develop/using-composer/managing-dependencies... - not sure what we should do. I think this situation needs to be covered in the change record and might require some discussion on the issue.

@alexpott That is a good point. We cannot rewrite the managing dependencies documentation right now, because work that is blocked by this issue is still needed before contrib modules can update their method of including Composer dependencies.

I suggest:

• Change this issue to (still) stop using wikimedia/composer-merge-plugin, but continue to 'require' it in composer.json
• Later, in follow-on work, provide composer-managed project templates for Drupal
• Recommend use of composer-managed project templates and provide conversion tool
• Deprecate wikimedia/composer-merge-plugin at that time
• Remove wikimedia/composer-merge-plugin in Drupal 9

I discussed this with @mixologic, and I no longer agree with my recommendation in #50. The managing dependencies document already recommends modifying the root composer.json file. It is therefore reasonable for us to:

• Remove wikimedia/composer-merge-plugin from the drupal/drupal composer.json (as this patch does)
• Rewrite the managing dependencies document to recommend the use of path repositories instead of the merge plugin
• Update the change record for this issue to recommend that sites utilizing the composer-merge-plugin for other purposes to either switch to using path repositories (recommended), or ensure that the wikimedia/composer-merge-plugin remains in their modified root composer.json file after they resolve the conflicts caused by this patch + them hacking core

I have updated the managing dependencies in a custom project document and the change record to advise users of the wikimedia composer merge plugin to use Composer path repositories instead.

Once these changes are reviewed, we can move this issue back to RTBC.

The primary concern is that by removing the wikimedia/composer-merge-plugin will remove it for people who are relying on it and they will no longer get their dependencies from their custom modules.

The problem here is that in order for the merge plugin to work, it *must* be in your 'root' composer.json. The only users that would have it in their root composer.json are ones that started with the highly discouraged composer create-project drupal/drupal, or users who have added it to their root manually, and have some other setup.

The reason that we discourage using drupal/drupal as a starting point is that as soon as you start adding any other modules/dependencies, your composer.json and your composer.lock changes, and then if you try to upgrade to a newer version of drupal/drupal, you enter into a whole new kind of merging hell with trying to sort out what is an upstream change to your composer.lock, and what is something your site currently relys on.

This nightmare is mitigated by altering a few lines in your composer.json to change the replace of drupal/core to a require, and removing the wikimedia merge lines from your root composer.json, and then upgrading core using composer by doing `composer update drupal/core --with-ependencies`.

So, given all of that, I do not believe that there will be a real impact in removing this from core's root composer.json. The scenario to be affected by this are:

1. The user started with drupal/drupal, against all the recommendations in the docs.
2. The user has never suffered through a drupal/drupal upgrade and converted their site to be a typical project where they do not conflict with the root composer.json/.lock in drupal/drupal.
3. The user has added a custom module, with 3rd party composer dependencies, and are using the wikimedia merge plugin to include those (instead of requiring them at the root level or using a path repository or using a vcs repository, which are all the recommended ways in all composer documentation)
4. The user then upgrades to 8.8.0.

So, even if we change nothing, step 4 above is already broken for the user, as they will have to resolve the conflicts in their root composer.json where they added info for their custom modules, and core's new composer.json does not contain that info. If we remove the wikimedia merge plugin, they will hopefully notice its removal, and keep it during their tangled mess of a merge resolution.

I don't have any real data that I can lean on to know how many users are currently stewing in that terrible composer soup, but my gut says "a small number approaching zero".

If the user somehow manages to get through merging in the upstream changes from drupal 8.8.0, and fail to preserve their wikimedia merge functionality that they were relying on, they can immediately reverse that change by executing `composer require wikimedia/composer-merge-plugin` from the root of their project.

So yeah, despite the fact that there was bad advice being given in the community maintained docs, I'm still very much +1 on removing this entirely from core.

Tagging needs framework manager review as per @alexpott's recommendation, and back to RTBC.

I picked the wrong management officials.

@Mixologic and @greg.1.anderson thank you for thinking through the concerns I've raised. I'm +1 on this change.

+++ b/composer.json
@@ -24,16 +37,8 @@
-            "core": ["type:drupal-core"],
+            "vendor/drupal/core": ["type:drupal-core"],

so does this move core outside the webroot?

How does that work with .css and .js files when aggregation is off?

It does not move core outside of webroot. It makes a symlink to the existing /core directory in vendor. See #28 for deeper explains.

@larowlan: The `core` directory remains in its current location. The problem that we need to work around in this patch is that the Composer Installers project has a rule that relocates type drupal-core to the /core directory. See #28. The result of this directive in composer.json is to reset the rule in Composer Installer back to the default (what it would be if there was no rule in Composer Installers for the type drupal-core), which, in conjunction with the `path` repository that we set up here will result in a symlink in vendor/drupal/core being created, pointing to /core. The symlink is unused.

Note also that in follow-on work, the Composer in Core Initiative will be making project templates for Drupal (see #2982680: Add composer-ready project templates to Drupal core) that will be used to generate the Drupal tarball downloads. These template projects will include drupal/core as an ordinary Composer project. It will be relocated to the /core directory. The symlink and the path repository will only be for folks who install Drupal from the .git repository, i.e. core developers. Those who install via download (drush dl) or Composer won't get the symlink in vendor/drupal/core.

Can we do an upstream change in any way?

@mile23 If we fixed composer/installers, the sites based on drupal-composer/drupal-installer would be fine, as those sites define the relocation for drupal-core to go to web/core. We might break existing sites if they do not have a relocated document root, and do not define an explicit relocation for drupal-core. There might not be any sites at all in this category, but technically this is a breaking change, and composer/installers covers a lot of different project types. We could try to fix the upstream, but I wouldn't want to block this issue on that attempt.

+++ b/composer.lock
@@ -674,6 +674,244 @@
+            "version": "8.8.x-dev",

Will this need updating every minor release? I guess that happens anyway though implicitly when we update other things.

With the symlink, it would be good to have a follow-up issue for core and related upstream issue for composer/installers so we can track things, I can see people getting confused by it and there's no way to document things in composer.json.

The downgrade issue is going to hit people doing core development but I doubt really anyone else, so seems an OK price to pay to improve composer for everyone else. Same thing with the symlink - it's an oddity but if it only affects core developers and unblocks work on improving everything else it's worth the trade-off. The one thing here that would be disruptive as such is for custom projects using the composer merge plugin but I don't see a way around that and this also pre-supposes a decent amount of composer knowledge on that project.

Untagging for release manager review but I think we need the follow-up and upstream issue before commit.

Im not sure there's much to do upstream in composer/installers. They have a facility to disable custom installers entirely, and all of the actual installation is achieved in composer itself. We could perhaps open an issue with composer/composer such that it checks if an installer is trying to copy over an existing path repository, that it noop's and assumes that the contents of the path on disk are what you want.

What would happen if we simply removed the symlink after composer install? is composer doing anything like writing /vendor/drupal/core into any autoloader paths?

@catch: Thank you very much for reviewing. Just to be clear, though, an upstream issue in composer/installers will allow us to get rid of the line in composer.json specifying where the symlink to /core should be placed; however, removing this line does not get rid of the symlink. The symlink is always placed by the composer path repository feature. Removing the line in the composer.json section for composer/installers will revert the location of the symlink to its default value. We don't have a way to get rid of it.

I'll go ahead and make the upstream and follow-up issues; I just wanted to make sure you were clear on the implications.

Oh wait, never mind about #64. I had an idea of how we can fix the problem in Composer and leave composer/installers alone.

I didn't notice #63 until just now. I submitted composer/composer#8254, "Allow path repositories to be relocated to their source location" to request the adjustment suggested there.

I can make a follow-on issue on drupal.org for that. I also wonder if #2701253: [meta] Make it possible to put modules into the vendor directory would also be a suitable follow-on issue, as the drupal/core relocation will not be necessary once that work is done.

I tried manually removing the symlink in vendor/drupal/core. The result was:

$ composer info drupal/core

  [InvalidArgumentException]     
  Package drupal/core not found  

I don't recommend this workaround.

The composer.lock entry for drupal/core will be "version": "8.8.x-dev" (or 8.9.x-dev in the future, etc.) when doing core development with a path repository (or when installing a dev version of drupal/core in a template project).

Most users will utilize a template project to install drupal/core ^8.8, in which case the composer.lock entry will be the specific version installed, e.g. 8.8.0 or whatever is currently appropriate.

I created a follow-on issue to remove the relocation of drupal/core to vendor/drupal/core once it is no longer necessary: #3071703: Remove composer/installer rule for drupal/core in root composer.json file

Discussed this with @Mixologic and @greg.1.anderson on slack.

I asked for a summary of the impact to each of these three groups:

- those using drupal-composer/drupal-project
- those using tarballs
- those using composer with drupal/drupal

There is no impact for the first two groups. The third group is who we're trying to help here as things like https://groups.drupal.org/node/516571 illustrate their issues. #2958021: Proposal: Composer Support in Core initiative shows the plan we're addressing.

So the only impact is on those using wikimedia/composer-merge-plugin for contrib modules. The change record highlights how those folks should update their existing code, which boils down to composer require wikimedia/composer-merge-plugin

Adding issue credits

This fails JS linting

Checking changed files...

/Volumes/files/code/git/core/drupal/core/tests/Drupal/Nightwatch/nightwatch.conf.js
  19:76  error  Replace `defaultIgnore` with `⏎··········defaultIgnore,⏎········`  prettier/prettier

✖ 1 problem (1 error, 0 warnings)
  1 error, 0 warnings potentially fixable with the `--fix` option.

Also, we need documentation updates on https://www.drupal.org/docs/develop/using-composer/managing-dependencies...

@larowlan re #73, I already updated those documents. I put the new preferred way (path repositories) at the top of the page. I kept the existing documentation about the Composer Merge plugin, in case someone was using it, but moved it to the bottom and marked it as deprecated.

The existing state of that page should be good for folks to rely on before this patch is committed, and after.

Still need to fix the js lint issue, though.

Here's an update to fix the eslint issue.

Added NDU tag to note that we should update the documentation when this is committed.

I'm not sure if this is appropriate at all, but this should satisfy eslint. It seems it may be suspect, though; there is nothing in the 'dependency' section at all right now; everything is a devDependency. nightwatch is a dev-only component. Not sure why eslint is fussy about glob, but does not care about any of the other dev dependencies.

If this is no good, we can go with #75.

#75 is the one we want here.

The eslint plugin is configured to specifically exclude the file in question here:
https://git.drupalcode.org/project/drupal/blob/8.8.x/core/.eslintignore#L7

Which is why it made it through without detecting any issues, but we're supposed to run yarn run prettier (https://git.drupalcode.org/project/drupal/blob/8.8.x/core/package.json#L21) to fix them, so we should probably open up an issue with the JS folks to find out if we should actually be ignoring those files or not.

It's a drag to get all the way to the finish line, only to stumble on an undocumented procedures that the CI system is designed to inform us about.

Committed 5cd26f2 and pushed to 8.8.

Published the change record

Woohoo! :-)

Still needs docs updates and that JS followup from #78.

Seems like drupal/core inside vendor directory confuses Drupal finder. On my git based Drupal 8 installation Drush cannot boostrap Drupal anymore. Also drush site-install fails to install a site created with drupal-composer/drupal-project.

It looks strange - when I add -o to composer install it throws lots of warnings for core

andypost@andy-HP:~/www/core8$ composer install
Loading composer repositories with package information
Installing dependencies (including require-dev) from lock file
Nothing to install or update
Package phpunit/phpunit-mock-objects is abandoned, you should avoid using it. No replacement was suggested.
Generating autoload files
> Drupal\Core\Composer\Composer::preAutoloadDump
> Drupal\Core\Composer\Composer::ensureHtaccess
    Skipped installation of bin bin/composer for package composer/composer: file not found in package
Nothing scaffolded because no packages are allowed in the top-level composer.json file.
andypost@andy-HP:~/www/core8$ composer install -o
Loading composer repositories with package information
Installing dependencies (including require-dev) from lock file
Nothing to install or update
Package phpunit/phpunit-mock-objects is abandoned, you should avoid using it. No replacement was suggested.
Generating optimized autoload files
> Drupal\Core\Composer\Composer::preAutoloadDump
Warning: Ambiguous class resolution, "Drupal\Core\Composer\Composer" was found in both "$baseDir . '/core/lib/Drupal/Core/Composer/Composer.php" and "/home/andypost/www/core8/vendor/drupal/core/lib/Drupal/Core/Composer/Composer.php", the first will be used.
Warning: Ambiguous class resolution, "Drupal\Component\Scaffold\GenerateAutoloadReferenceFile" was found in both "/home/andypost/www/core8/vendor/drupal/core-composer-scaffold/GenerateAutoloadReferenceFile.php" and "/home/andypost/www/core8/vendor/drupal/core/lib/Drupal/Component/Scaffold/GenerateAutoloadReferenceFile.php", the first will be used.
Warning: Ambiguous class resolution, "Drupal\Component\Scaffold\Operations\ScaffoldFileCollection" was found in both "/home/andypost/www/core8/vendor/drupal/core-composer-scaffold/Operations/ScaffoldFileCollection.php" and "/home/andypost/www/core8/vendor/drupal/core/lib/Drupal/Component/Scaffold/Operations/ScaffoldFileCollection.php", the first will be used.
Warning: Ambiguous class resolution, "Drupal\Component\Scaffold\Operations\ReplaceOp" was found in both "/home/andypost/www/core8/vendor/drupal/core-composer-scaffold/Operations/ReplaceOp.php" and "/home/andypost/www/core8/vendor/drupal/core/lib/Drupal/Component/Scaffold/Operations/ReplaceOp.php", the first will be used.
Warning: Ambiguous class resolution, "Drupal\Component\Scaffold\Operations\OperationFactory" was found in both "/home/andypost/www/core8/vendor/drupal/core-composer-scaffold/Operations/OperationFactory.php" and "/home/andypost/www/core8/vendor/drupal/core/lib/Drupal/Component/Scaffold/Operations/OperationFactory.php", the first will be used.
Warning: Ambiguous class resolution, "Drupal\Component\Scaffold\Operations\ConjunctionOp" was found in both "/home/andypost/www/core8/vendor/drupal/core-composer-scaffold/Operations/ConjunctionOp.php" and "/home/andypost/www/core8/vendor/drupal/core/lib/Drupal/Component/Scaffold/Operations/ConjunctionOp.php", the first will be used.
Warning: Ambiguous class resolution, "Drupal\Component\Scaffold\Operations\OperationInterface" was found in both "/home/andypost/www/core8/vendor/drupal/core-composer-scaffold/Operations/OperationInterface.php" and "/home/andypost/www/core8/vendor/drupal/core/lib/Drupal/Component/Scaffold/Operations/OperationInterface.php", the first will be used.
Warning: Ambiguous class resolution, "Drupal\Component\Scaffold\Operations\OperationData" was found in both "/home/andypost/www/core8/vendor/drupal/core-composer-scaffold/Operations/OperationData.php" and "/home/andypost/www/core8/vendor/drupal/core/lib/Drupal/Component/Scaffold/Operations/OperationData.php", the first will be used.
Warning: Ambiguous class resolution, "Drupal\Component\Scaffold\Operations\ScaffoldResult" was found in both "/home/andypost/www/core8/vendor/drupal/core-composer-scaffold/Operations/ScaffoldResult.php" and "/home/andypost/www/core8/vendor/drupal/core/lib/Drupal/Component/Scaffold/Operations/ScaffoldResult.php", the first will be used.
Warning: Ambiguous class resolution, "Drupal\Component\Scaffold\Operations\SkipOp" was found in both "/home/andypost/www/core8/vendor/drupal/core-composer-scaffold/Operations/SkipOp.php" and "/home/andypost/www/core8/vendor/drupal/core/lib/Drupal/Component/Scaffold/Operations/SkipOp.php", the first will be used.
Warning: Ambiguous class resolution, "Drupal\Component\Scaffold\Operations\AppendOp" was found in both "/home/andypost/www/core8/vendor/drupal/core-composer-scaffold/Operations/AppendOp.php" and "/home/andypost/www/core8/vendor/drupal/core/lib/Drupal/Component/Scaffold/Operations/AppendOp.php", the first will be used.
Warning: Ambiguous class resolution, "Drupal\Component\Scaffold\Operations\ConjoinableInterface" was found in both "/home/andypost/www/core8/vendor/drupal/core-composer-scaffold/Operations/ConjoinableInterface.php" and "/home/andypost/www/core8/vendor/drupal/core/lib/Drupal/Component/Scaffold/Operations/ConjoinableInterface.php", the first will be used.
Warning: Ambiguous class resolution, "Drupal\Component\Scaffold\ScaffoldFilePath" was found in both "/home/andypost/www/core8/vendor/drupal/core-composer-scaffold/ScaffoldFilePath.php" and "/home/andypost/www/core8/vendor/drupal/core/lib/Drupal/Component/Scaffold/ScaffoldFilePath.php", the first will be used.
Warning: Ambiguous class resolution, "Drupal\Component\Scaffold\Plugin" was found in both "/home/andypost/www/core8/vendor/drupal/core-composer-scaffold/Plugin.php" and "/home/andypost/www/core8/vendor/drupal/core/lib/Drupal/Component/Scaffold/Plugin.php", the first will be used.
Warning: Ambiguous class resolution, "Drupal\Component\Scaffold\CommandProvider" was found in both "/home/andypost/www/core8/vendor/drupal/core-composer-scaffold/CommandProvider.php" and "/home/andypost/www/core8/vendor/drupal/core/lib/Drupal/Component/Scaffold/CommandProvider.php", the first will be used.
Warning: Ambiguous class resolution, "Drupal\Component\Scaffold\ComposerScaffoldCommand" was found in both "/home/andypost/www/core8/vendor/drupal/core-composer-scaffold/ComposerScaffoldCommand.php" and "/home/andypost/www/core8/vendor/drupal/core/lib/Drupal/Component/Scaffold/ComposerScaffoldCommand.php", the first will be used.
Warning: Ambiguous class resolution, "Drupal\Component\Scaffold\ScaffoldFileInfo" was found in both "/home/andypost/www/core8/vendor/drupal/core-composer-scaffold/ScaffoldFileInfo.php" and "/home/andypost/www/core8/vendor/drupal/core/lib/Drupal/Component/Scaffold/ScaffoldFileInfo.php", the first will be used.
Warning: Ambiguous class resolution, "Drupal\Component\Scaffold\ScaffoldOptions" was found in both "/home/andypost/www/core8/vendor/drupal/core-composer-scaffold/ScaffoldOptions.php" and "/home/andypost/www/core8/vendor/drupal/core/lib/Drupal/Component/Scaffold/ScaffoldOptions.php", the first will be used.
Warning: Ambiguous class resolution, "Drupal\Component\Scaffold\AllowedPackages" was found in both "/home/andypost/www/core8/vendor/drupal/core-composer-scaffold/AllowedPackages.php" and "/home/andypost/www/core8/vendor/drupal/core/lib/Drupal/Component/Scaffold/AllowedPackages.php", the first will be used.
Warning: Ambiguous class resolution, "Drupal\Component\Scaffold\PostPackageEventListenerInterface" was found in both "/home/andypost/www/core8/vendor/drupal/core-composer-scaffold/PostPackageEventListenerInterface.php" and "/home/andypost/www/core8/vendor/drupal/core/lib/Drupal/Component/Scaffold/PostPackageEventListenerInterface.php", the first will be used.
Warning: Ambiguous class resolution, "Drupal\Component\Scaffold\Handler" was found in both "/home/andypost/www/core8/vendor/drupal/core-composer-scaffold/Handler.php" and "/home/andypost/www/core8/vendor/drupal/core/lib/Drupal/Component/Scaffold/Handler.php", the first will be used.
Warning: Ambiguous class resolution, "Drupal\Component\Scaffold\Interpolator" was found in both "/home/andypost/www/core8/vendor/drupal/core-composer-scaffold/Interpolator.php" and "/home/andypost/www/core8/vendor/drupal/core/lib/Drupal/Component/Scaffold/Interpolator.php", the first will be used.
Warning: Ambiguous class resolution, "Drupal\Component\Scaffold\ManageGitIgnore" was found in both "/home/andypost/www/core8/vendor/drupal/core-composer-scaffold/ManageGitIgnore.php" and "/home/andypost/www/core8/vendor/drupal/core/lib/Drupal/Component/Scaffold/ManageGitIgnore.php", the first will be used.
Warning: Ambiguous class resolution, "Drupal\Component\Scaffold\Git" was found in both "/home/andypost/www/core8/vendor/drupal/core-composer-scaffold/Git.php" and "/home/andypost/www/core8/vendor/drupal/core/lib/Drupal/Component/Scaffold/Git.php", the first will be used.
Warning: Ambiguous class resolution, "Drupal\Component\Scaffold\ManageOptions" was found in both "/home/andypost/www/core8/vendor/drupal/core-composer-scaffold/ManageOptions.php" and "/home/andypost/www/core8/vendor/drupal/core/lib/Drupal/Component/Scaffold/ManageOptions.php", the first will be used.
> Drupal\Core\Composer\Composer::ensureHtaccess
    Skipped installation of bin bin/composer for package composer/composer: file not found in package
Nothing scaffolded because no packages are allowed in the top-level composer.json file.

@andypost I've just been following this issue but ^ seems troubling. I have to carve out time to test manually.

Seems like drupal/core inside vendor directory confuses Drupal finder. On my git based Drupal 8 installation Drush cannot boostrap Drupal anymore.

This is unexpected; Drush should not try to iterate through vendor during bootstrap. I haven't looked into this yet. We should fix Drush.

Also drush site-install fails to install a site created with drupal-composer/drupal-project.

In theory, sites created with drupal-composer/drupal-project should be unchanged with this patch. I just created a new drupal-project site and was able to install it just fine. What problem did you encounter?

when I add -o to composer install it throws lots of warnings for core

This is annoying, but at least the warnings are all referring to the same file, so it is not indicative of any operational problem. Perhaps the warnings regarding the Composer components can be fixed by removing the autoload directive in the root composer.json file. I haven't tested to see if this is in fact redundant.

I'm pretty sure that the scaffold warnings can be removed by relocating the Composer plugins out of Components, and into some other directory that is not autoloaded. Maybe we should have Core, Components and Plugins. I'll investigate. This issue is minor, and I think easy to fix.

What problem did you encounter?

Never mind, it was irrelevant to this issue.

edit: nevermind. :-)

Removing the redundant autoload section from the root level composer.json file fixed the warning Ambiguous class resolution, "Drupal\Core\Composer\Composer" , and relocating the Scaffold plugin fixed the warnings about the scaffold plugin source classes.

I also tracked down the cause of #82. Drupal Finder is making certain assumptions about the contents of the composer.json for drupal/drupal. We broke those assumptions with this pass (necessarily). I'll submit a PR to drupal/finder to fix.

... aaaand let's even test that patch.

So let's say we move all composer plugins to core/lib/Drupal/Plugin/YourPluginNameHere.

This means we should move the tests to core/tests/Drupal/Tests/Plugin/YourPluginNameHere.

However this means that run-tests.sh won't run our tests because it's broken: #2878269: Modify TestDiscovery so the testbot runs all the tests

And yup, since we don't do PSR-4 for Drupal\Core\Plugin\ that means all the tests fail anyway, which is why: #2943856: [meta] Reorganize Components so they are testable in isolation

Either we can refactor our testing system (which I'd love to have happen), or revert this issue and get back to making the legacy tarball product.

Yeah, the Scaffold tests here, besides being in the wrong location, are also broken due to the fact that the scaffold files are no longer autoloaded. We should fix things up such that the scaffold files are autoloaded only in the tests. (Note that autoload-dev is not what we're looking for; we want to Scaffold files available in the scaffold tests, but not in other instances where drupal/drupal was installed with dev components.)

I guess we should work on getting #2878269: Modify TestDiscovery so the testbot runs all the tests committed and then circle back here, relocate the tests and figure out an autoloading strategy? We could do #2943856: [meta] Reorganize Components so they are testable in isolation or some subset of it for just plugins, or something temporary / hacky.

'Temporary/hacky' is really 'permanent/bad,' which is why we're staring at this.

Let's revert and make drupal/drupal work with the new plugins and also with the merge. The legacy tarball product won't have the merge, and then we can loop back here with all the stuff we learned.

I don't really want to revert. The thing we're fixing here is just a warning that does not affect the operation of Drupal. I think we can live with the warnings from composer install -o until we can fix the scaffold tests.

A temporary workaround for #82 till Drupal Finder is fixed.

"require": {
    "webflo/drupal-finder": "dev-find-drupal-drupal-root as 1.1.0"
},
"repositories": [
    {
        "type": "vcs",
        "url": "https://github.com/greg-1-anderson/drupal-finder"
    }
]

Another possible approach is patching Drupal Finder with Composer patches.

drupal-finder 1.2.0 contains the fix. https://github.com/webflo/drupal-finder/releases/tag/1.2.0

I had to revert this to get drush 9.7.1 working again in a lando setup.

c.f. drush-ops/drush#4142

First of all, I want to thank you @greg.1.anderson, @Mile23, and @Mixologic for all the work on this initiative.

As a git user, my whole core development workflow is broken on the 8.8.x branch, I'm not sure which core folder to hack. I'm also seeing #83.

This issue does not aim go make tarball downloads of Drupal "Composer ready", but it unblocks the way for that to happen in follow-up work.

This means git users are in the same boat. I think those follow up should be committed at the same time. We also need a separate detailed change record to explain the core development process after all the composer workflow changes.

The file system layout is exactly the same with and without this patch, with the only exception being that there is a new symbolic link created from /vendor/drupal/core to /core.

This statement is not true at all. After these steps, I have two core folder /vendor/drupal/core and /core:

• git clone --depth 1 --branch 8.8.x https://git.drupalcode.org/project/drupal.git
• cd drupal/
• composer install

I also tried following steps:

• git clone --depth 1 --branch 8.8.x https://git.drupalcode.org/project/drupal.git
• cd drupal/
• git fetch-patch https://www.drupal.org/files/issues/2019-08-01/2912387-88.patch
• composer install

I get the following error:

ading composer repositories with package information
Installing dependencies (including require-dev) from lock file
Package operations: 102 installs, 0 updates, 0 removals
  - Installing composer/installers (v1.6.0): Loading from cache
Class Drupal\Core\Composer\Composer is not autoloadable, can not call post-package-install script

                                                                                                           
  [RuntimeException]                                                                                       
  Source path "core/lib/Drupal/Component/Scaffold" is not found for package drupal/core-composer-scaffold  
                                                                                                           

install [--prefer-source] [--prefer-dist] [--dry-run] [--dev] [--no-dev] [--no-custom-installers] [--no-autoloader] [--no-scripts] [--no-progress] [--no-suggest] [-v|vv|vvv|--verbose] [-o|--optimize-autoloader] [-a|--classmap-authoritative] [--apcu-autoloader] [--ignore-platform-reqs] [--] [<packages>]...

Last but not the least, I don't agree with the assessment in #62.

The one thing here that would be disruptive as such is for custom projects using the composer merge plugin but I don't see a way around that and this also pre-supposes a decent amount of composer knowledge on that project.

These changes should be considered a BC break and should not be done in a minor version. Yes, I understand we are not changing any API here but we are changing the whole workflow for people without any prior announcement. We are not pointing them to any documentation, at least not in this issue AFAICS, to fix their setup issues. The changes introduced here and #2982684: Add a composer scaffolding plugin to core are going to break all the projects using custom composer workflow not just merge plugin users which in terms means breaking CI/CD pipelines for all of those projects.

The argument this also pre-supposes a decent amount of composer knowledge on that proje is also not valid. This means that upgrade from 8.7.x to 8.8.x is not going to be straight forward and it will take time to understand and accommodate these changes into the projects.

Given all of the above can I please request core committer to discuss this decision once more? So that we can take proper non-BC break steps to implement all these changes. That's why I'm changing this to a critical bug for now so that we can revert this and reassess the situation.

This statement is not true at all. After these steps, I have two core folder /vendor/drupal/core and /core:

From the docs on path repos:

The local package will be symlinked if possible, in which case the output in the console will read Symlinking from ../../packages/my-package. If symlinking is not possible the package will be copied. In that case, the console will output Mirrored from ../../packages/my-package.

Do you see 'Mirrored from' in your Composer output when you do the install? Are you using Windows?

Anyway: If we're not reverting this, whatever else we do, we also need to add options:symlink: true to all the path repos. (We should add that to the patch even if we are reverting it.)

I'm marginally in favor of reverting, since the real goal is to try to make the legacy tarball product (see #94), but if there's a solution let's do it.

We can get rid of vendor/drupal core with #3071703: Remove composer/installer rule for drupal/core in root composer.json file.

Are you using Windows?

No, I'm on linux.

I've reverted this for now. At least for #88 and #90. Haven't had a chance to properly assess jibran's comments (and will be afk most of next week).

After pulling the reverting change, I had trouble with composer install.

But this made my troubles go away:

• rm -rf vendor/
• composer install

FTR: The patch being reverted is #77.

CR back to draft.

Thanks, @catch.

Re #105: #90 was fixed.

Disappointed about the revert, but it's probably for the best, since Drush 9 is still broken with this applied. Let's use this opportunity to resolve as many issues as possible. We need to be able to make incremental progress forward; it won't work to make one giant patch. The rerolls would be a nightmare, and the review would be impossible.

I've reverted this for now. At least for #88 and #90.

#88 and #90 caused warnings only. We didn't notice these until after the commit, but there was no actual problem here beyond the autoloader complaining that some of the same classes appeared more than once. Composer was already eliminating one of the duplicates, and they were the same files, so no harm done. We'll work on clearing up the warnings for the re-commit. It's good to hear that #90 is fixed now; this will make getting rid of the warnings easier.

Anyway: If we're not reverting this, whatever else we do, we also need to add options:symlink: true to all the path repos. (We should add that to the patch even if we are reverting it.)

I had it like this in an earlier version of the patch, but it seems that the advice from the documentation is to leave this option undefined for the best (default) behavior for each platform.

The changes introduced here and #2982684: Add a composer scaffolding plugin to core are going to break all the projects using custom composer workflow

That should not be the case. #2982684 only added the scaffold plugin; it did not use the plugin or change behavior at all. This patch should not affect custom composer workflow users, as those projects use drupal/core as a subtree split, which is not affected by this patch.

I do hear your concern about the possibility that future patches, e.g. #3067645: Add core scaffold assets to drupal/core's composer.json extra field, might break drupal-composer/drupal-project. We can consider our action in that issue more carefully. We should not hold up preliminary issues such as this one with future work, though.

The argument this also pre-supposes a decent amount of composer knowledge on that proje is also not valid.

That statement was only intended to refer to users who were directly utilizing the Composer Merge Plugin to manage the dependencies of their modules. These users should also switch to path repositories, as explained in the updated documentation.

This means that upgrade from 8.7.x to 8.8.x is not going to be straight forward and it will take time to understand and accommodate these changes into the projects.

There should not be any difference at all for tarball (drush pm-update) users. As I mentioned above, we hear your concern about drupal-composer/drupal-project users, and will consider BC for these users in the follow-on work. We hope that most of these users will use the to-be-provided conversion tool to update to the "Drupal 8.8.x way", which will be faster and more effective than the current template. You are right that it would be better if this were an optional step that could happen after the initial conversion, though.

Source path "core/lib/Drupal/Component/Scaffold" is not found for package drupal/core-composer-scaffold

This can happen if you run `composer install` after changing your codebase to include this patch iff you do not remove your vendor directory. We're going to have to have some small road-bumps like this to get rid of the Composer Merge Plugin. We can do a better job at documenting things like this in the Change Record et. al.; I don't think we should wait until Drupal 9 to drop the use of the Composer Merge plugin, though, as doing that isn't simply dropping a dependency. If desired, we could keep wikimedia/composer-merge-plugin in the composer.json unused in the reroll of this patch, and remove just that part with Drupal 9. That will have some benefit for people who are still using it instead of a path repository.

@greg.1.anderson thank you for addressing my concerns. I also had a lengthy chat with @Mixologic and we tried to fix the duplicate core folder issue which we were able to fix after updating composer to the latest version.

I agree with @Mixologic that it is impossible to understand before committing these kinds of changes that what is broken.

we hear your concern about drupal-composer/drupal-project users, and will consider BC for these users in the follow-on work.

Thanks, for understanding that I'll try to review, test the new patch(s), and help with docs review as well.

Let's fix #83 and get this in.

If we require Composer 1.9 as the minimum version for core development, then the vendor/drupal/core symlink goes away and the warnings in #83 goes away and Drush starts working.

If we need to allow earlier versions of Composer, then the warnings in #83 are a lot harder to remove, and a new (not-yet-available) version of Drush 9 (or Drush 8) is required.

We can't require Composer 1.9 until the testbots have upgraded. Here's a newer patch that relocates the Scaffold plugin, which eliminates most of the warnings in #83. One remains, which will go away when the installer-paths entry is fixed (Composer 1.9 required).

Reminder: delete your vendor directory after applying (or removing) this patch, before running composer install.

+++ b/composer.json
@@ -5,10 +5,23 @@
+        "composer/composer": "^1.8",

How about 1.9? It's just dev for running tests, but it's the only place where we document the minimum.

@mile23 I am planning on making that change once we require Composer 1.9.

Also, #112 is green, but it doesn't seem that the Scaffold plugin tests ran. Care to look at that?

The test discovery code looks like this should run. I don't see any evidence of it running in the test output, but then again, I can't see any evidence of Core or Component tests running either, so maybe I am just bad at comprehending the test output. :p

Here's a clear way to find out: insert a test that always fails in the Scaffold plugin tests. Inelegant an inefficient, but here we go.

I see the new tests in the log from #112... https://dispatcher.drupalci.org/job/drupal_patches/3891/consoleFull

Yay! Thanks for #116, @mile23. #115 also proves that #112 is good.

The next patch will only take a couple of minutes to roll, but I don't have time to do it right now. Once the test bots are upgraded to 1.9 we'll be ready to rock.

OK, I am still not sure if we should be relocating the Scaffold plugin here or somewhere else (e.g. #3067645), but in any event, here is a re-roll of #112 that moves the Scaffold plugin out of core.

If we do not relocate the Scaffold plugin here, the alternative would be to remove it from drupal/core and instead require it from drupal/drupal (for testing).

Still holding off on the "next patch" mentioned in #118 until the test bots are upgraded to Composer 1.9.

Hm forgot to add the patch and interdiff for #119.

To focus on BC, we will switch our strategy here.

- The Scaffold Plugin will remain in the repository so that it will continue to be subtree-split into a separate project. We will probably move it to a different location as follow-on work.
- We will no longer `require` the scaffold plugin from drupal/core. The template projects will need to require it explicitly.
- We will no longer include the path repository for the Scaffold plugin from drupal/drupal.
- When we add the scaffold assets to drupal/core, we will also keep them in their original location. This will be done as follow-on work.
- We will stop using the Merge Plugin in this patch, but we will keep it in the composer.json file, to be removed in Drupal 9 per the deprecation policy.

These changes require Composer 1.9, because we are not creating the symlink at vendor/drupal/core any longer. Once the tests pass here, we will need to update the issue summery and change record.

Patch in #121 works fine with composer 1.9.

+1 to the strategy in #121. Let's update the IS with it.

Updated issue summary and change record. The documentation Managing dependencies for a custom project did not need any changes; the instructions there are still applicable under the new strategy.

Requiring Composer 1.9.0 for Drupal core should not be a huge ask, since it is easy to upgrade via `composer self-update`. The larger issue is whether it might cause confusion if folks do not read the change record. This updated patch adds pre-install and pre-update scripts that test to see if the Composer version is at least 1.9.0. If it is not, then the operation aborts with an error message telling the user to run `composer self-update`.

IS looks good thanks for updating it.

The IS looks good for core and custom modules committed to the main project ; however, there is also the case of contrib modules using the merge plugin in "interesting" ways, first of which would be Webform, which has not one, but two composer.json files, one being the normal composer.json file, and the other one being composer.libraries.json, which needs to be brought in using the merge plugin.

It would be nice to include a recommendation on such situations, even if they are rare, especially considering how popular Webform is.

The merge plugin isn’t a problem on its own. As far as I know this would just be a documentation update for Webform.

Re: #127: The docs that would need updating are here: https://www.drupal.org/docs/8/modules/webform/webform-frequently-asked-q...

They'd need to add that you should require wikimedia/merge to your root project.

And that's generally the solution to solving situations like this: Add wikimedia/merge to your root project as needed.

Code review stuff:

1. +++ b/core/lib/Drupal/Core/Composer/Composer.php
   @@ -94,6 +95,17 @@ class Composer {
   +    $composerVersion = \Composer\Composer::getVersion();
   

   Add this class to the use block.

2. +++ b/core/lib/Drupal/Core/Composer/Composer.php
   @@ -94,6 +95,17 @@ class Composer {
   +      throw new \Exception("Drupal core development requires Composer 1.9.0, but Composer $composerVersion is installed. Please run 'composer self-update'.\n";
   

   Needs a close-parenthesis. Also let's throw \RuntimeException, and then make a tiny unit test that runs this method and expects either NULL or \RuntimeException just so we know it's working.

Awesome, looks like #121 passed on re-run with Composer 1.9-enabled testbots. I accidentally submitted garbage in #124, but I'll get that fixed up and address the other comments.

I'm not sure about mocking the static getVersion method. I know there are some libraries that will allow that; is Drupal using any of them already?

Finally, regarding the Webform module, it looks like everything in the existing instructions will still work after this is merged, so I don't think any action is specifically required here. In the fullness of time, Webform and other modules that recommend using the wikimedia merge plugin should perhaps switch to recommending path repositories instead, similar to https://www.drupal.org/docs/develop/using-composer/managing-dependencies.... In Drupal 9, modules can just put their dependencies in their composer.json file.

Fixes #129, adds test.

With this Composer 1.9.0 warning, it looks like this:

$ composer update
> Drupal\Core\Composer\Composer::ensureComposerVersion
Script Drupal\Core\Composer\Composer::ensureComposerVersion handling the pre-update-cmd event terminated with an exception

                                                                               
  [RuntimeException]                                                           
  Drupal core development requires Composer 1.9.0, but Composer 1.8.6 is inst  
  alled. Please run 'composer self-update'.                                    
                                                                               

Note that without this DX enhancement, this patch is basically identical to #13. :-)

Edit: Woops, missed this one:

+++ b/core/lib/Drupal/Core/Composer/Composer.php
@@ -94,6 +95,17 @@ class Composer {
+    $composerVersion = \Composer\Composer::getVersion();

Add to use block.

Yeah we were working around a lot of Composer stuff. Having Composer 1.9.0 allowed us to go back to the way we wanted it to be in the first place.

Two small additions:

- Added the `use ... as ComposerApp` thing.
- Put back the Wikimedia Composer Merge Plugin options in the 'extra' block. Who knows, some contrib module might need those settings, so let's maximize our BC potential. Shouldn't have any effect on core.

All of the tests in #133 passed EXCEPT the one that checks to see if the composer.lock hash is correct, because I forgot to update that.

Allrighty. This is green now.

I think that composer 1.9 is the way forward here, as that extra symlink had all kinds of unanticipated side effects (api.drupal.org ended up with 2 copies of everything, then no copies after the patch was reverted, hilarity ensued).

So, the wikimedia merge is not 'removed' in as much as 'core development no longer relies on it, but its skeleton is still there just in case somebody *really* wants to re-animate that zombie.

I think this is still a good way forward, and unblocks the rest of the initiatives next steps.

Would love to see what happens if we get this in before MWDS so we can hands on troubleshoot to shake out anything else that could potentially cause a core development issue, but I think the composer script that throws the exception is a fabulous solution that immediately allows a dev to progress past that hump.

I did the following:

$ composer selfupdate -r

This rolled me back to Composer 1.8.6.

$ git checkout 8.8.x
$ rm -rf vendor
$ composer install
$ wget https://www.drupal.org/files/issues/2019-08-05/2912387-134.patch
$ git apply 2912387-134.patch
$ composer install

At this point I got the error message that says you need to update your Composer.

$ composer selfupdate
$ composer install
$ ./vendor/bin/phpunit -c core --testsuite unit

Everything worked as expected, so +1.

Committed 344d842 and pushed to 8.8.x. Thanks!

Ok, lets try again - the impact here is much smaller, and the upstream fix is very welcome

Republished the updated change notice.

Should we show the warning on composer install or composer update if the project is using composer-merge-plugin? If yes, then it can be a follow up. Also, do we need a follow child issue of #3032686: Remove references to unused packages in Drupal 9's vendor hardening to remove composer-merge-plugin form composer.json?

The follow-up issues @jibran suggests are both worth adding.

Re #141 Do you mean a warning if they are merging core/composer.json? Simply using the merge plugin for another reason is anyone's prerogative and doesn't present any problems for core.

shake out anything else that could potentially cause a core development issue

So, heres the first one Composer::getVersion() was added to Composer in version 1.8.5 released on April 9, 2019.

Which leads to things like "PHP Fatal error: Uncaught Error: Call to undefined method Composer\Composer::getVersion() in /Users/mike/Development/Drupal/core/lib/Drupal/Core/Composer/Composer.php:104

Looks like we can fall back on \Composer\Composer::VERSION;

https://github.com/composer/composer/commit/427116749558f99e4c04ffa3fe35...

I opened a follow up here: https://www.drupal.org/project/drupal/issues/3073012

#3073012: Detect Composer Version in older versions of composer

Shouldn't #3075785: Update composer/composer to ^1.9.1 be done as a follow-up?

Commented on that issue.

Back in #114 we were planning on proceeding per #146, but moved away from that later. Conversation must have happened in slack, as it's not recorded here.

I think the release note here could use a little work to explain the impacts of the change. Typically we'll also link the change record from the release note for advanced details. (This change broke my script to create a new branch of core and so presumably other usecases might need updating as well.) :)

Update the release notes snippet to reference the change record.

Perfect, thanks @greg.1.anderson!

Small tweaks to the release note markup.

Webform recommends the merge plugin: #3088336: Is composer.libraries.json compatible with the Composer path repository configuration?.

The wikimedia/composer-merge-plugin is deprecated, but folks can still choose to use it. Contrib may recommend something that core deprecates. Clearly, that is not recommended, though. :)

The wikimedia/composer-merge-plugin will be removed in Drupal 9, but even then, folks may choose to add it back in to the root-level composer.json for their specific site.

Apologies... we need to reopen this one more time to warn people about:

You must set COMPOSER_ROOT_VERSION to the branch this patch is intended for, plus -dev. This keeps composer from trying to 'guess' the version from your branchname, which can cause unexpected results and may even end up deleting your local core folder.

Probably merely adding this:

Following this change, core developers creating patches that update core's dependencies must set COMPOSER_ROOT_VERSION explicitly to avoid unexpected results (including possible accidental deletion of the core directory). Read Managing Composer updates for Drupal core for more information.

(If those words are accurate?)

What is the process here? Just edit the existing Change Record?

FYI: I encountered the same when I downloaded the 8.8.0-alpha1 tarball and called composer require drupal/some_random_module.

Maybe the d.o. tarball builder could add a version key to the composer.json file. 🤔

#158 makes a good point. We have been advertising that 8.8.0 is going to have "Composer Ready downloads", but at the moment, the tarballs still contain drupal/drupal instead of drupal/legacy-project. This makes them dangerous to use.

This will clear up once the tarball packager is updated to use drupal/legacy-project.

@greg.1.anderson We need to warn people in both the change record and the release note snippet in the issue summary. I will mention it in the beta1 release notes today and we will also want to mention it in the final 8.8.0 release notes. You can add my paragraph, or improve it if there's chnages needed, update the IS and CR, and then mark the issue back to fixed.
Thanks!

Update release notes snippet to include warning about COMPOSER_ROOT_VERSION.

Marking "fixed" per #160.