Merge Component composer.json files to account for them during build

Now that #2352091: Create (and maintain) a subtree split of Drupal core and #2513388: Create (and maintain) a subtree split of each Drupal Component are essentially fixed, these bugs in the composer.json are going to be more prominent and should be fixed soon.

Let's fix it so stuff is working without changing the replace-es. We should have a separate issue for that. I'd wager it's not going to be as easy as it could be.

Needs work because it doesn't apply to 8.3.x due to composer.lock having changed. I'll forgo the 'needs reroll' tag because it really just needs to be re-applied with --rej and then have the lock file rebuilt.

+++ b/core/composer.json
@@ -172,6 +172,37 @@
+    "extra": {
+        "merge-plugin": {
+            "require": [
+                "core/lib/Drupal/Component/Annotation/composer.json",
+                "core/lib/Drupal/Component/Assertion/composer.json",
+                "core/lib/Drupal/Component/Bridge/composer.json",

So the idea here is that we merge these composer.json files so that they'll be exercised, and any dependencies they have are taken into account, as well as any errors that would break things.

We want to keep the replace section because we don't want to require and pull the components into vendor/. Yet. :-) That's definitely a follow-up.

The patches in #24 apply and allow for an install, and also survive a composer update.

Everything looks good, except the IS mentions a change record. I'm not sure one is needed, but it might be helpful.

I'm coming from #2873101: Component version constraints are out-dated

The component split has occurred, but core isn't using it. Try going to an empty directory and typing composer require drupal/core-uuid and be amazed. :-)

There are problems with the declarations of dependencies in some of the component composer.json files. We can't at this point use some components because they have these problems. For instance, we'd like to use the plugin component in the testbot instead of having our own copy of the code, but we can't because of these problems: #2643110: Have drupalci_testbot require drupal/core-plugin in composer.json

We also need to update the integration test, because that's good to have.

By way of preventing regressions, it's nice to merge the component composer.json files. Even if we never directly require the components and only replace them in core/composer.json, then we're still getting the benefit of having Composer try to account for their dependencies.

And... We want these to be merged from core/composer.json (and not ./composer.json) because they're dependencies of drupal/core, not drupal/drupal. I'd rather not have the recursion, but we're only doing the recursion on core/composer.json anyway, and we're in control of the merges in that file.

If we change to requiring the components, it will happen in core/composer.json. And when that happens, even if someone's installed ./composer.json still has the recursion set to true due to being outdated or whatever, there won't be any merges in core/composer.json to recurse through.

Components should not be tied to the requirements of drupal/core, unless there is an actual hard requirement within the component. This is because the component might be consumed by another project and they shouldn't be forced to upgrade from, for instance, symfony 2.8 to 3.2 just because we did, if the component doesn't actually need it.

So:

• A patch to both 8.3.x and 8.4.x should fix obvious errors in components' composer.json files, and it should fix the omission from ComposerIntegrationTest.
• The patch should also add the same version constraints to both 8.4.x and 8.3.x branches for components, which allows both branches to be built with composer. This would be an open-ended >= type constraint on the lowest version in core.
• So that we can test whether this is working, we should allow for a recursive merge of components' composer.json files from core/composer.json. This will exercise the version constraints, but will not require them as dependencies since they are in the replace section.
• Ideally, there would be a core branch test which performs composer update and then runs the tests. This would tell us if our component requirement constraints need work.

Unfortunately, there's no way to exercise these merged component composer.json files other than to just say composer update, which then updates all the dependencies. For instance, composer/installers is updated to 1.3.0 in the lock file for both patches. The testbot might tell us that this isn't a problem, but it might be inconsistent enough to make a maintainer unhappy.

In order to get around this, we might verify that the merge works and then go back to the old lock file, but that patch is not presented here. Maintainers could come up with a better idea, too. :-)

So here are two patches. The only difference between them is their updated composer.lock files.

Here are the same patches where I verified that the merges work with composer update, and then did the following:

$ git checkout [dev] composer.lock
$ composer update --lock
# verify with:
$ composer install

Updating title and added change record.

See #36. The components don't have a hard dependency on any version other than being newer than the one we use in core for 8.3.x. Also, if we declare a version dependency of, say, ~2.8 in 8.3.x, and ~3.2 in 8.4.x, then we impose that raise in version constraint on any project using the component, even though the component itself doesn't need the newer version.

So, if we have a better way to express 'any version higher than 2.8.*' which satisfies the needs of both drupal/core and any other project that might use the components, then I'm all for it.

OK, added <4.0.0 to all those symfony components.

The interdiffs for the different branches are identical, and the patches only differ on the composer.lock content hash. But they're both here so they have their own test run.

I can't repro the fail of Drupal\node\Tests\Views\FrontPageTest locally, so I'm setting to NR and re-running the test. Also it's in the list of random fails: #2829040-35: [meta] Known intermittent, random, and environment-specific test failures

You're right... We should be as wide in version constraints as we reasonably can.

I was using the version constraints that core has. Since we don't have tests of the core components themselves at minimum version constraints, it's kind of hard to account for changes to the components since core was using 2.7 versions of symfony components.

Here's a version with 2.7 restored.

Drupal 8.4 just switched to Symfony 3.2: #2712647: Update Symfony components to ~3.2 and #2871253: Update Symfony components to 3.2.8.

@mpdonadio: Yes, but the components should cast a wider net, since they're supposed to be consumed by other projects. That's why the upper constraints on them for symfony components is <4.0.0.

#56++ ... My bad.

Trying to figure out how to test some of these things led me here: #2876408: Update wikimedia/composer-merge-plugin to ~1.4 in composer.json to match composer.lock

I made a travis-ci test of the components, which currently fails because of this issue. https://github.com/paul-m/drupal_component_tester

Travis results here: https://travis-ci.org/paul-m/drupal_component_tester

A review here would be great. :-)

Patch in #54 keeps the current versions, just adds <4.0.0 for symfony components. This is in both 8.4 and 8.3, because the requirements for the components themselves probably haven't changed. We'll test this assumption in follow-ups.

We want to have version requirements that are general enough for the outside world, specific enough for core, and somehow testable.

We can't test until the dependencies are at least minimally correct and resolvable, so we add reasonable best-guess version boundaries now. Then we get more specific in #2876669: Fix dependency version requirement declarations in components and children.

How does that sound?

There's a chance that the latter will have composer suddenly start merging stuff it didn't merge before, right?

It will only merge what we tell it to merge. Since we're merging the component composer files, the recurse will only go as far as those components require. It might be that some component dependency also has wikimedia merge directives, in which case we might need another strategy, but so far that's not the case.

Well, if you apply the patch and run composer update -vv you'll see that currently we don't have that problem. There's really no way around assuming it won't be a problem, other than to not exercise the composer.json files for the components.

No, no, I mean packages *added* to the template composer.json, and dependencies of those packages.

wikimedia will only start merging the files we tell it to merge, under extra:merge-plugin:require. The only way for packages required by drupal/drupal to start recursively merging is if we also add its composer.json to extra:merge-plugin:require.

The only worrisome part is dependencies of core/composer.json, but for that we have this:

+++ b/core/composer.json
@@ -150,6 +150,37 @@
+            "recurse": false,

Also: DRUPAL_ROOT/composer.json isn't a template. It's the drupal/drupal package. It gets used every time a tarball is built, for instance.

We all look forward to the day when we can be free of this bailing-wire-and-spit type of dependency management, but that day is not near. :-)

Rerolled for composer.lock content-hash after #2876408: Update wikimedia/composer-merge-plugin to ~1.4 in composer.json to match composer.lock

Reroll of 8.4.x patch.

8.3.x patch in #75 still applies and passes.

Do we still need #2876669: Fix dependency version requirement declarations in components after this? We need a new 8.5.x patch.

This issue is to get the component composer.json files into shape so that we can use them.

We can then figure out more specific requirements in other issues. And since the composer.json files will actually work, we can write automated tests to help us figure that out.

Patch for 8.5.x.

The only difference is the lock file's content-hash. Either patch will apply to either branch using git apply --rej. Create a new lock file with composer update --lock.

#81 makes sense to me so RTBC

Would love to be able to require core components in the testbot before 8.5.0... #2643110: Have drupalci_testbot require drupal/core-plugin in composer.json

The plan seems to be that we should stop using the wikimedia merge plugin: #2912387: Stop using wikimedia/composer-merge-plugin

Therefore we'll eventually need to figure out another way to account for the Components. We might end up requiring them into core's composer.json.

I won't change RTBC here in case this becomes a temporary measure to get things working.

Needs some work since #2876725: Fix trivial core component composer bugs before 8.4.0

Re-rolled.

Whoops, the patch in #89 was against 8.5.x. This is against 8.4.x.

Patch in #91 no longer applies. Re-rolled.

Almost RTBC except it needs an 8.5.x patch so we can do both at the same time. The only difference is the lock file content hash, which you can make like this:

$ git apply git apply --rej 2867960-93.patch
$ rm composer.lock.rej
$ composer update --lock

I was able to perform composer update and composer update --prefer-lowest and then run all the unit tests, for both 8.4.x and 8.5.x.

The 84x patch is just the one from #93renamed for clarity.

Ran ComposerIntegrationTest locally and both passed.

Same deal as #94: I was able to perform composer update and composer update --prefer-lowest and then run all the unit tests, for both 8.4.x and 8.5.x.

Test failed due to #2919773: Fail to download drupal/coder because github.com/klausi/coder has gone missing

Restarting test.

Do we need a change record because people are managing their own root composer.json files? I.e. are people going to have to change the recurse value in their own root composer.json files? I also wonder what happens with composer drupal installer? Or do we just not care because the core is coming anyway and this buys us testing on DrupalCI but makes no difference for anyone else atm?

Well there is a CR https://www.drupal.org/node/2875519 - but it is not telling a site what are the actual impacts of the change for them and what if anything they need to do.

Also the CR talks about things that haven't happened yet - ie. moving components to vendor which is odd because it means that people reading it are getting information that is not helping them deal with the actual change.

Or do we just not care because the core is coming anyway and this buys us testing on DrupalCI but makes no difference for anyone else atm?

That's pretty much it. Drupal core doesn't even officially support having a different composer.json file. However, other projects could do the same trick for their testing.

The only concern is that if you required, say, drupal/address into drupal/drupal in the past, and maybe drupal/address' requirements are in conflict with a component or two. How do you find out? You have to merge the components. But if you don't add the merge to your stale composer.json file then you never find out. We'd expect this to be as much a problem as it is right now (not a huge one).

However... If you follow the update.txt instructions you'll do the right thing in that case and type composer require drupal/address into the new drupal/drupal. Then your stale composer.json file won't be stale any more, and you'll find out that there's a conflict (if there is).

Hmm... Looking for that issue about updating UPDATE.TXT to make this clearer, and can't find it.

Updated CR to point out that you can make this change in your bespoke composer.json file if you want, with instructions.

Restarting tests.

Tests passed so setting back to RTBC, assuming testbot failure.

@xjm and I discussed this and decided to postpone it on #2874909: Update Symfony components to 3.3.* as that is Critical.

If the two won't collide, please advise.

The changes to the component composer.json files have a very wide allowance for symfony versions, safe to say >=2.8 <4.0. That should include 3.3.

Part of the point of merging the dependencies here is that we can then test which constraints are right or wrong and generally make them more maintainable.

We should probably concern ourselves with the 3.3 upgrade first, but as part of that we might end up changing the components and forgetting to change their constraints to ^3.3. Which we wouldn't do if we fixed this issue first.

#2874909: Update Symfony components to 3.3.* was replaced by #2927746: Update Symfony components to 3.4.*, which is in, so I'm unpostponing here.

Also, the testbot now depends on this: #2643110: Have drupalci_testbot require drupal/core-plugin in composer.json

The patches from #96 no longer apply so I rerolled.

This will probably need another re-roll after #2940367: Update Symfony components to 3.4.4 (or vice-versa).

It'd be great to get this in 8.5.x so we can start testing the component constraints before the 8.6.0 release. It's only disruptive to our test results if we're doing the wrong thing, which is what we want.

We already have a follow-up #2876669: Fix dependency version requirement declarations in components ready to finetune version requirements so setting it to RTBC.

Committed c70c4f4 and pushed to 8.5.x. Thanks!
Committed 65b232a and pushed to 8.6.x. Thanks!

Whilst 8.5.x and 8.6.x are relatively aligned I think we should do this. It helps keeps our component composer.json a bit more honest.