LegacyProject composer templates wrongly reference 8.x + fix test coverage

Initial patch.

I get a lot of errors trying to chmod and cleanup the workspace directory. Let's see if the testbot fares better.

I'm pretty sure my local fails were/are related to the problem in #3082866: composer complains "Skipped installation of bin bin/composer for package composer/composer: file not found in package". So we really should work on getting that issue fixed.

Somehow the testbot filesystem seems much more forgiving of that dangling alias, however.

Anyway, this patch converts the test to use a dataProvider, so we can better identify which test is failing. Also I've modified drupalci.yml to get some quicker test results for the build test. We'll need to revert that change before committing.

Ready for prime time. Reverted drupalci.yml, removed errant error_log().

I reverted drupalci.yml but neglected to remove the part that runs the script that is, you know... the point of this issue.

Re #8: The testbot seems to not care that there is a dangling composer alias, so I don't think #3082866: composer complains "Skipped installation of bin bin/composer for package composer/composer: file not found in package" is a blocker. Just something that would be better to have sooner, rather than later.

#10 oh yikes, I totally missed that. Interesting that it passes anyway.

This patch adds a composer create-project test as well, and adds @group #slow. This way you can call phpunit --exclude-group #slow if you want to skip the big builds.

I think the create-project test is the actual test here. Opinions?

It still fails locally based on #3082866: composer complains "Skipped installation of bin bin/composer for package composer/composer: file not found in package" but I wager the testbot won't fail.

It takes ~6 minutes to run locally.

A few things here.

We did a sort of emergency script-ectomy of the shell script here, because it was being disruptive: #3087219: Remove broken test for composer templates

That will necessitate a re-roll here.

We also discussed the fails in #13.

+++ b/core/tests/Drupal/BuildTests/Composer/Plugin/Scaffold/ComposerProjectTemplatesTest.php
@@ -0,0 +1,86 @@
+    $this->executeCommand('composer --working-dir="' . $working_path . '" config repositories.scaffold path ' . $scaffold_dir);
...
+    $this->executeCommand('composer --working-dir="' . $working_path . '" config repositories.hardening path ' . $hardening_dir);

The testbot does the right thing and symlinks these packages during the install test.

Locally, I found that it checking out the packages which is why we have this assertion:

+++ b/core/tests/Drupal/BuildTests/Composer/Plugin/Scaffold/ComposerProjectTemplatesTest.php
@@ -0,0 +1,86 @@
+    $this->assertRegExp('/Installing drupal\/core-composer-scaffold .*: Cloning .* from cache/m', $process->getErrorOutput(), 'Composer Scaffold plugin was not cloned from path repo.');

That assertion is incorrect, and should instead reflect the symlinking in this line:

 Extracting archive  - Installing drupal/core-composer-scaffold (8.8.x-dev): Symlinking from /tmp/build_workspace_5831d5c9cfa8ccda0ddf7a7adea7e248Ke5Wtg/composer/Plugin/Scaffold\n

The changes in #3087219: Remove broken test for composer templates came about because while these tests use a path repo for in-core dependencies, we can’t use a path repo for the drupal/core-recommended metapackage. If there is a change to the core repo’s composer.lock file that hasn’t yet been used to generate a new drupal/core-recommended package, we have to assume that the test will fail. (Because it did in this case.)

This led to a failing HEAD, which was a big problem because today is the day before alpha for Drupal 8.8.x :-)

So to mitigate this, we discussed replacing a dependency on drupal/core-recommended with drupal/core. This will mitigate the lag in generating drupal/core-recommended, but at the same time makes a test that doesn’t reflect a complete build of the template package.

Until this conundrum is solved, we’ll pursue ‘good enough’ with this change.

And of course we still have a fail related to #3082866: composer complains "Skipped installation of bin bin/composer for package composer/composer: file not found in package"

This is me experimenting trying to get local to do the right thing with symlinks. I never succeeded, but I'm curious what the testbot will do.

Modified drupalci.yml to only run build tests.

It seems our luck has changed and we're postponed here on #3082866: composer complains "Skipped installation of bin bin/composer for package composer/composer: file not found in package"

Also we def want this in 8.8.x because it's our only test of the composer templates, and they were disabled after breaking HEAD: #3087219: Remove broken test for composer templates

I experimented with writing this patch with #3087626-4: Convert drupal/core-recommended & c. into a subtree split added, so I can confirm that we're still blocked on that.

But the other thing I discovered is that the install test is pretty fragile once we try to include all the special cases needed to make it work here. To the point that it's not really a test any more. For instance: drupal/recommended-project requires drupal/core-recommended ^8.8, which we have to change to self.version in order to perform an install using the local codebase.

After talking to @greg.1.anderson about it in Slack, we decided the create-project is the real test here, and that we can remove testTemplateInstalls().

#3087626: Convert drupal/core-recommended & c. into a subtree split is in, so this can move forward.

It turns out that if you specify a packages.json file on the command line with composer create-project, that packages.json file only applies towards the discovery of the project template you want to create, and not its dependencies.

So here we add path repos to the project templates, because we have to. Otherwise Composer will seek the dependencies from packagist and/or Drupal facade.

Moved the test to the Templates namespace since it's not a test of the scaffold plugin.

Also targeting to 8.9.x, but we'll need to backport this to 8.8.x.

One of the saddest things in the world is that it works, you submit the patch, you go back to fix a comment you forgot to delete, and then it doesn't work any more.

  Extracting archive  - Installing drupal/core-composer-scaffold (8.9.x-dev dca4b12): Cloning dca4b123a638d78bf77719632993e920de6cc426 from cache\n

That's supposed to be a symlink.

Anyway, I'm currently trying to make this test prove that it used symlinks when it was supposed to.

Any other suggestions for assertions about the validity of the installed code are welcome.

I worked as hard as I could to not make any specific version requirements necessary for this test, but we do have to specify them, and in fact it's a necessary test.

As an example... In 9.0.x, drupal/recommended-project currently says it needs drupal/core-recommended ^8.8. Since it also has minimum stability as dev, and also prefers stable, it will use 8.8.0-alpha1 for drupal/core-recommended.

So therefore this test already shows us that our templates need fixing, which I've done here with updating the minimum versions in the templates.

Here are patches for 8.9.x and 9.0.x.

The differences between 8.9.x and 9.0.x are in the minimum version specification for the templates, the packages.json fixture, and a single variable in the test so we only have to maintain it in one place there.

We'll reroll for 8.8.x later, because this should be in 8.8.x.

Updated to use \Drupal\Composer\Composer::drupalVersionBranch() cuz that's the perfect thing.

Included here are patches for 8.8.x, 8.9.x, and 9.0.x. Committers can use whichever ones they please, and ask for more as needed. :-) This issue is still targeted for 8.9.x, however.

Interdiff reflects a bad rebase that was fixed along with the change.

Reroll after #3087482: Add 'next steps' info to Composer installation output

So we're blocked here until we can put the composer.lock into the template projects, then.

Uhm, nope. :-)

Which leads us to a seemingly-eternal question...

Installs: drupal/core-composer-scaffold:8.8.x-dev, […] drupal/core-vendor-hardening:8.8.0-alpha1, […] drupal/core-project-message:8.8.0-alpha1, drupal/core-recommended:8.8.0-alpha1

Why does drupal/core-composer-scaffold resolve to 8.8.x-dev but the others do not?

drupal/core-composer-scaffold has a branch-alias setting, but adding a similar branch alias to the other packages does not solve the problem.

The answer seems to be: The patch in #43 does not account for the project message plugin as a path repo. Therefore Composer searches the world for a version and discovers 8.8.0-alpha1, and then that apparently opens the floodgates and all bets are off when it comes to the rest of our carefully-curated path repos. Whether this counts as an upstream bug or not probably depends on your point of view.

So that leaves us with a choice for this test:
1. Hard code all our expectations for where plugins and metapackages should be, so that they can be converted to path repos.
2. Dynamically generate all the path repos by scanning the file system for the various packages.

This patch does the latter, and the reason is this: As we add metapackages or plugins and/or modify the templates, we should gracefully make sure that the patched additions are available to this build test. Using hard-coded paths means that we could get false positives because no one would remember to update this test.

This patch also adds a test which reconciles the template dataprovider against a scan of the composer/Template directory to fail and tell us that we need to update this test for templates when we add them.

This patch fixes the bad rebase for the 8.8.x patch from #49.

Hiding files.

#1: "Which, to me means that as soon as we have an 8.8.0 stable release, the 8.9.x-dev branch is going to start failing because it will find a stable version." I don't think that's the case since there's a path repo, but I'm totally willing to be wrong. If the build tests were in 8.7.x we could test that now.

#2: We should add drupal/core as one of the path repos.

#3: We add all the possible path repos because the test doesn't know which ones are required by the current template. This way it works for whichever template you supply in the @dataprovider.

#5: I think you're right that this test is the round-trip for the metapackages: You do a dev on core, you update composer.lock, it generates new metapackages, that's all in the patch, and here we apply those metapackages back in a build process. Theoretically this should never fail because you were able to generate a composer.lock file in the first place, but if it does, we want to know.

The other thing is that we've already used the tests to find a 'bug' in the template files in #33. I put bug in quotes because if you're making a new project, you'd probably get what you wanted, but per-branch it breaks our expectation.

The cherry on top would be to perform an install and visit the front page, but this thing already takes two and a half minutes. Also, i'm not sure the quickstart feature knows what to do with the recommended install layout. That'd make a good follow-up.

This patch adds drupal/core as a path repo, with added assertions to match.

There’s also a FAIL patch which illustrates some ways to fail this test by making a bad project template. One has minimum_stability at stable, and the other specifies a scaffold file that doesn’t exist.

Yes, if we turn off packagist (and/or drupal composer facade) then we can only get cached discoveries. Also, we'd have to turn it back on in tearDown() because otherwise the next test will break.

I was trying to figure out how to make a fake D8.8 release and stuff, but the drush/drush experiment in #61 is the right idea. However it doesn't actually pass, since the test doesn't make path repos for the Template directory.

Here's a patch that demonstrates this technique that should pass DrupalCI by moving the fake drush/drush into composer/Plugin/.

#62 is the interdiff. :-)

Edit: The fail just tells us that the fake drush/drush composer.json file doesn't validate, so yay, we demonstrated that path repos work.

Will re-upload #55 after #3090906: self.version in metapackages considered harmful, which is a bugfix and might make a reroll happen here.

Yes, each branch needs its own patch, because they have to target their own core version per #33.

Leaving the backport tag until we get a commit to 8.8.x. I'm not entirely sure which branch has the code freeze at this point...

And here's the follow-up: #3096044: Ensure quickstart feature works with drupal/core-recommended

Yes, if you put ^8.9 in a template project, it's just documentation.

Well, except it's not, actually. Because, as I pointed out in #33, for the 9.0.x branch Composer can resolve drupal/core-recommended to 8.8.0-alpha1 because it's more stable than 9.0.x-dev, which undoes the purpose of the test. See #10.

@alexpott mentions that if we get rid of minimum-stability: dev then that's not the case. But we haven't done that, and I don't see an issue to do that. Also, I don't think that we should, because then we can't require any dependencies at @dev stability. If you look at 9.0.x you see that we're using a beta version of symfony because we have to (for now).

And even if we get rid of minimum-stability: dev, then we'd still be allowing drupal/core-recommended:8.8.0 within the 9.0.x branch, which doesn't make much sense at all, especially in light of this issue: #3090905: composer create project drupal/core-recommended:version should get the exact version specified For instance, if you are running PHP 7.0, and you say composer create-project drupal/recommended-project myproject ~9 then you'll end up with the 9.0.x version of the template, and either 8.8.x or 8.9.x version of core, with no indication of this mismatch. What should happen is that Composer should say, no, you can't install Drupal 9 on PHP 7.0.

I can understand the desire to not have to update the templates with appropriate minor versions with each release in this test, but we have to do that for the packages.json file even if we don't do it for the templates. Otherwise we're not testing whether we can build with the patched codebase. Also, the test won't have false positives if we overlook that update. It will fail because we do this:

+++ b/core/tests/Drupal/BuildTests/Composer/Template/ComposerProjectTemplatesTest.php
@@ -0,0 +1,169 @@
+    // Get the Drupal core version branch. For instance, this should be
+    // 8.9.x-dev for the 8.9.x branch. If the test fails because this branch
+    // version is incorrect, you should update the version specified in
+    // core/tests/Drupal/BuildTests/Composer/fixtures/packages.json to reflect
+    // the target branch of Drupal core.
+    $core_version = Composer::drupalVersionBranch();

The proof is https://www.drupal.org/pift-ci-job/1482558, which is the failed test against 3086644_88x_55.patch in #67, that ran against 8.9.x instead of 8.8.x.

If anyone wants to amend the patch to improve it, that'd be great.

But if we're changing specifications such that we're *not* testing whether we can build against the patched codebase, then someone will have to update the issue summary to reflect this fact.

I think the constraint in the major branch needs to match but I hope we don't need to do minor updates. There's no way that 8.8.0-alpha1 can match ^9

Well, it's the same problem moving forward: 9.0.0-alpha1 can match ^9 when you mean ^9.1 with minimum-stability:dev. This is much less likely than the 8-for-9 mixup, but only by accident.

But it's still a thing. We want this test to fail if that happens, so that release managers can be sure they're getting what they want.

+++ b/core/tests/Drupal/BuildTests/Composer/Template/ComposerProjectTemplatesTest.php
@@ -0,0 +1,211 @@
+    // Get the Drupal core version branch. For instance, this should be
+    // 8.9.x-dev for the 8.9.x branch. If the test fails because this branch
+    // version is incorrect, you should update the version specified in
+    // core/tests/Drupal/BuildTests/Composer/fixtures/packages.json to reflect
+    // the target branch of Drupal core.
+    $core_version = Composer::drupalVersionBranch();
...
+  protected function makeTestPackage($repository_path, $version) {

+1 on this change, except it means the doc block is incorrect now.

Now that we have an official release for 8.8.0 I was able to run this against a tagged release with good effect.

$ git checkout 8.8.0
$ git apply 3086644-2-75.8.x.x.patch
# Modify \Drupal::VERSION to be '8.8.x-dev'
$ ./vendor/bin/phpunit -c core/ --testsuite build --filter ComposerProjectTemplatesTest --stop-on-fail

This fails (as it should) because drupal/core-recommended still requires drupal/core 8.8.0, so the build log does not reflect 8.8.x-dev. Like this:

 Extracting archive  - Installing drupal/core (8.8.0): Loading from cache
 Extracting archive  - Installing drupal/core-recommended (8.8.x-dev)

# which does not contain...

Installing drupal/core (8.8.x-dev): Symlinking from

Without modifying \Drupal::VERSION we get a pass. So yay!

This patch updates the docblock mentioned in #76.

Patch still applies. Re-testing.

The point of this test is to account for a build from patched core, and we can patch the dependencies of any of our plugins.

For instance if the patch adds a new Composer plugin and adds a dependency to it from a template, we'd want that to be symlinked in this build. The only way to find out about it is either to dynamically generate it, or remember to amend the dataprovider here.

If we neglect to update the dataprovider, then we run into the world of false positives, where for instance the patch changes a template to require one of our existing plugins, but erroneously requires a tagged release. Then we don't know that the patched plugin is not being used.

If there wasn't a risk of false-positive, then it could be reasonable to just let the test fail and cause people to grumble as they update the dataprovider in the next patch. But they might never find out.

To be clear, the only bit of the calculated code I am proposing moving to the data provider is the "require" line.

Yes, that's the part where we gather the expectations from the part of the codebase that could have been patched. That's the part I'm defending. :-)

If someone makes a patch that adds a requirement to the Composer Template project, they'll get a false positive until they update the test, but that is as it should be, IMO.

No, they'll never update the test because it's a false positive. They won't know they need to update it, because it passes.

OK, so the concern is that there's a false positive if there is some sort of error about the template requirements, not necessarily that the expectations are or aren't hard-coded.

We already have some assertions about the existence of the composer.json file, which helps in that regard.

Adding hard-coded expectations is definitely desirable, but see #70.

So adding hard-coded expectations and then checking up on them means we're not only testing the code but also testing the test. We do that a little bit here with testVerifyTemplateTestProviderIsAccurate() which checks if there are new templates we haven't accounted for here, because it seems likely we'd do that at some point.

I think in the case of $require, however, if we trust a separate process that checks the hard-coded test data, then we should cut out the middle man and have the trusted checking data be the test data. We can put a boundary check on it, such as asserting that $require is not empty because it's vanishingly unlikely that we'd have a template with no requirements.

Here's a patch which does just that.

Now that Drupal 9 requires PHP 7.3+, I added that test to the 9.0.x patch in #84.

This should have been committed before 8.8.1, because basically the raison d'etre of the composer initiative is untested. That's despite having all these subtree splits in the repo specifically so that we can test them patched in DrupalCI, and despite having invented a new testing framework to accomplish it.

It should be a hard blocker for 9.0.x-alpha1.

The 8.9.x/8.8.x patch from #84 still applies, but here it is again.