Problem/Motivation

Currently, drupal/core-recommended is a metapackage that is created via a drupalorg infrastructure tool that builds it from the information in each composer.lock file of each version (tag) and branch of the Drupal git repository (drupal/drupal). This is done after a tag or branch is pushed (i.e. on every release, and every time a patch is committed).

In #3086644: LegacyProject composer templates wrongly reference 8.x + fix test coverage, we discovered that if we attempt to do a composer create-project test on the drupal/recommended-project Composer Template Project, that said test will fail for any patch that updates the dependency versions in the drupal/drupal lock file. The reason for the failure is that the create-project test attempts to combine the drupal/core project, which it includes via a path repository so that it includes the changes from the patch being tested, and the drupal/core-recommended project, which has all of the version constraints from the current HEAD of the base branch (e.g. 8.9.x-dev). This will work for any patch where the composer.lock file has the same dependencies as in the HEAD of the dev branch, but fails when these dependencies are updated.

The only way that a create-project test could possibly work would be if the drupal/core-recommended project were updated with the data from the composer.lock file in the current patch. Under the current design and implementation, this presents a chicken-and-egg problem.

Proposed resolution

We can resolve this problem by converting drupal/core-recommended into a subtree split of drupal/drupal. We can update it every time a composer update command runs, and give it a composer.json file that will cause the subtree split tool to update the split project on GitHub at the same time it updates drupal/core and the other split projects. (n.b. drupal/core-recommended is a metapackage, and therefore consists only of a composer.json file.)

With drupal/core-recommended being created as a subtree split, then and composer create-project test that wishes to include drupal/core as a path repository may also include drupal/core-recommended as a path repository at the same time. In this way, these two projects will always have compatible information in their composer.lock / composer.json files, respectively, and the test will work.

We will also port over drupal/dev-dependencies and drupal/pinned-dev-dependencies to use the same technique so that we can be consistent in the way that the metapackages are generated for Drupal 8.8.x and later.

Finally, also for consistency, we will move the Composer hook that checks the Composer version to composer/Composer.php, because this hook is only needed for drupal/drupal; it is not needed by drupal/core users.

Steps to Test

1. Check out a branch of drupal/drupal, e.g. 8.9.x. If this isn't a fresh install, `rm -rf vendor`.
2. Run 'composer install'
3. Pick the most recent patch for the branch, e.g. #104 for 8.9.x, and apply it
4. Make a branch (`git checkout -b 3087626-104`) and then commit the results from (3)
5. Run: `COMPOSER_ROOT_VERSION=8.9.x-dev composer update --lock`
6. You should see an error message similar to the one below:

> Drupal\Composer\Composer::ensureBehatDriverVersions
Script Drupal\Composer\Composer::ensureBehatDriverVersions handling the post-update-cmd event terminated with an exception

[RuntimeException]
Drupal requires behat/mink-selenium2-driver:1.3.x-dev in its composer.json
file, but it is pinned to dev-master in the composer.lock file.
This sometimes happens when Composer becomes confused. To fix:

1. `git checkout -- composer.lock`, or otherwise reset to a known-good lock file.
2. `rm -rf vendor`
3. `composer install`
4. `COMPOSER_ROOT_VERSION=8.9.x-dev composer update ...` (where ... is
the update arguments you wish to run, e.g. --lock).

Follow the instructions and run the same update command again. It should work now.
7. Pick a package and try to update it, e.g. `COMPOSER_ROOT_VERSION=8.9.x-dev composer update doctrine/lexer`
8. Confirm that the package was updated. If it was updated, then you should also see the message:

Updated metapackage file composer/Metapackage/CoreRecommended/composer.json.
If you make a patch, ensure that the files above are included.

9. Run `git diff composer/Metapackage/CoreRecommended/composer.json` and confirm that the package that you updated was changed in the metapackage.
10. Discard your changes (e.g. `git checkout -- .`), because we do not actually want to make any updates at this time.

Optional: try again for 9.0.x. Be sure to update `COMPOSER_ROOT_VERSION=9.0.x` if you do. This branch could be harder to test, though, because the composer.lock is up-to-date, so it might be hard to do step 6. If you test both branches, make sure that you REMOVE YOUR `vendor` DIRECTORY COMPLETELY before switching branches and running `composer install` again.

Remaining tasks

  • Convert the package generator function that creates drupal/core-recommended into a post-update script in drupal/core.
  • Modify the package generator so that it continues to creates tags and branches for Drupal 8.7.x and earlier, but does not create tags or branches for Drupal 8.8.x and later. drupalorg-infrastructure/package-generator#7
  • Update the documentation with instructions on how to deal with Composer updates when making packages. See https://www.drupal.org/node/3089540
  • Sit back and watch the existing subtree split tool automatically keep the 8.8.x versions of drupal/core-recommended, drupal/dev-dependencies and drupal/pinned-dev-dependencies from the files we write into drupal/drupal.

User interface changes

None.

API changes

None.

Data model changes

None.

Release notes snippet

n/a

CommentFileSizeAuthor
#119 9.x-to-8.x-patch-diff.txt4.13 KBjibran
#118 3087626-118-8.8.x.patch50.92 KBjibran
#118 3087626-118-8.9.x.patch50.92 KBjibran
#118 3087626-118-9.0.x.patch51.3 KBjibran
#118 interdiff-D8-3c231b.txt2.86 KBjibran
#118 interdiff-D9-36efde.txt2.24 KBjibran
#105 3087626-105.patch51.3 KBgreg.1.anderson
#105 101-to-105-interdiff.txt590 bytesgreg.1.anderson
#104 3087626-104.patch176.24 KBgreg.1.anderson
#104 102-104-interdiff.txt590 bytesgreg.1.anderson
#102 3087626-102.patch176.21 KBgreg.1.anderson
#102 96-to-102-interdiff.txt2.19 KBgreg.1.anderson
#101 3087626-101.patch51.27 KBgreg.1.anderson
#101 97-to-101-interdiff.txt2.82 KBgreg.1.anderson
#97 3087626-97.patch51.24 KBgreg.1.anderson
#97 94-to-97-interdiff.txt1.1 KBgreg.1.anderson
#96 3087626-96.patch50.85 KBgreg.1.anderson
#96 94-to-96-interdiff.txt5.9 KBgreg.1.anderson
#94 3087626-94.patch50.94 KBgreg.1.anderson
#94 91-to-94-interdiff.txt3.65 KBgreg.1.anderson
#91 3087626-91.patch50.91 KBgreg.1.anderson
#91 90-to-91-interdiff.txt7.45 KBgreg.1.anderson
#90 3087626-90.patch47.98 KBgreg.1.anderson
#90 71-to-90-interdiff.txt261.13 KBgreg.1.anderson
#71 3087626-71.patch265.62 KBgreg.1.anderson
#69 3087626-69.patch266.01 KBgreg.1.anderson
#69 66-to-69-interdiff.txt6.08 KBgreg.1.anderson
#66 3087626-68.patch263.85 KBgreg.1.anderson
#66 62-to-66-interdiff.txt707 bytesgreg.1.anderson
#62 3087626-62.patch263.83 KBMixologic
#62 interdiff_60_62.txt1.66 KBMixologic
#60 3087626-60.patch263.85 KBgreg.1.anderson
#60 58-to-60-interdiff.txt7.95 KBgreg.1.anderson
#58 52-to-58-interdiff.txt8.5 KBgreg.1.anderson
#58 3087626-58.patch363.79 KBgreg.1.anderson
#52 3087626-52.patch262.66 KBgreg.1.anderson
#52 34-to-51-interdiff.txt13.55 KBgreg.1.anderson
#34 3087626-34.patch260.07 KBgreg.1.anderson
#34 27-to-34-interdiff.txt2.6 KBgreg.1.anderson
#27 3087626-27.patch259.85 KBMixologic
#25 3087626-25.patch0 bytesMixologic
#25 interdiff_23_25.txt2.27 KBMixologic
#23 interdiff_21-23.txt5.3 KBMixologic
#23 3087626-23.patch259.9 KBMixologic
#21 interdiff_17-21.txt9.63 KBMixologic
#21 3087626-21.patch259.88 KBMixologic
#17 interdiff_14-17.txt36 KBMixologic
#17 3087626-17.patch265.1 KBMixologic
#14 3087626-14.patch272.2 KBgreg.1.anderson
#14 11-to-14-interdiff.txt8.84 KBgreg.1.anderson
#11 3087626-11.patch274.1 KBgreg.1.anderson
#4 3087626-4.patch8.41 KBgreg.1.anderson
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

greg.1.anderson created an issue. See original summary.

Mile23’s picture

To me this means that we should not have a drupal/core-recommended, but instead have trustworthy dependency constraints in drupal/core.

The problem with that is that we then have to do min/max testing on multiple platforms at some point in the CI/release process.

greg.1.anderson’s picture

There is an existing long discussion of min/max testing vs. drupal/core-recommended in #3076600: Create drupal/core-recommended metapackage. My opinion has not changed, but if we are going to resume that conversation, we should do so on that issue, or make a follow-up issue, and reserve this thread for considerations on how drupal/core-recommended is built.

greg.1.anderson’s picture

Status: Active » Needs review
FileSize
8.41 KB

Here is a patch that converts the method of creating drupal/core-recommended to a post-update hook which is subsequently subtree-split out.

Mile23’s picture

Status: Needs review » Needs work

OK, so this patch updates composer/Metapackage/CoreRecommended/composer.json after updating anything with Composer.

This means that if you're a core developer, you might well see a changed file after composer update, and that you should commit it for your patch.

I'd suggest that we also trigger this on project install as well, because it's possible to update, reset a few composer.json and lock files here and there, and then install, leaving the metapackage out of date.

I'd also suggest running composer update --lock so there's a start state for the metapackge, and adding that to the patch here.

greg.1.anderson’s picture

Adding a message whenever the metapackage changes is a good idea.

I'm not sure that the use-case of updating after composer install is necessary. If the user modifies composer.json, then they should run composer update --lock, which will update the metapackage.

Also, the metapackage does not need a composer.lock file. It is never installed as a top-level project, so the composer.lock file is always ignored. Metapackages also can only have one file, their composer.json. Finally, if we did make a lock file for drupal/core-recommended, it would always have the same info as the composer.json file, which pins each dependency to a specific version.

Mixologic’s picture

One thing that I wondered but never said out loud with the other metapackages, is whether or not they needed to include a README.md, or whether they are _required_ to have a LICENSE.txt. The former seems optional, and maybe gets in the way of a scaffolded readme, but I dont think you can have a subtree split, without a LICENSE.txt

greg.1.anderson’s picture

You can write files like README.txt and LICENSE.txt into a metapackage, but when you `composer require` the metapackage you do not get any of these files; you only get the composer.json.

If we must have files, we can do the same thing we do with the legacy-scaffold-assets project: call it a metapackage informally, but in fact register it as a "project" instead of as a "metapackage".

Mile23’s picture

I'm not sure that the use-case of updating after composer install is necessary. If the user modifies composer.json, then they should run composer update --lock, which will update the metapackage.

  • Modify composer.json
  • Composer update
  • Work on stuff
  • Realize you didn't need to update anything
  • Revert composer.json
  • Revert composer.lock
  • Composer install
  • Now your metapackage is wrong.
  • Make a patch anyway.
  • Tests fail? Dunno? Maybe?

Also, the metapackage does not need a composer.lock file. It is never installed as a top-level project, so the composer.lock file is always ignored. Metapackages also can only have one file, their composer.json. Finally, if we did make a lock file for drupal/core-recommended, it would always have the same info as the composer.json file, which pins each dependency to a specific version.

  • Apply the patch here.
  • Say composer update --lock in the repo.
  • A metapackage is generated in code.
  • Commit that.
  • Generate a new patch.
  • Now the patch has both the updater and the metapackage.
greg.1.anderson’s picture

I'm ambivalent about #9. On the one hand, if you revert your composer.lock file, you should revert your metapackages. On the other hand, folks don't know to do that yet, and it's not a lot of overhead to regenerate the metapackage on every composer install. It just seems unnecessary to do it then, as the metapackage only changes on composer install in edge cases.

greg.1.anderson’s picture

Title: Convert drupal/core-recommended into a subtree split » Convert drupal/core-recommended & c. into a subtree split
Issue summary: View changes
FileSize
274.1 KB

Updated patch - not running on any additional events at the moment, but it does print a warning whenever a metapackage changes.

Updated the code by porting from the existing Package Generator. No interdiff, because the implementation is completely different.

greg.1.anderson’s picture

Status: Needs work » Needs review

Let's run the tests too.

I will also mention that the test fixtures in #11 contain a full composer.lock from Drupal 8.8.x. If this is viewed as being too much to tuck away in an obscure test directory, we could always pare it down to contain just a few dependencies, and update the expected results in the tests to match.

Status: Needs review » Needs work

The last submitted patch, 11: 3087626-11.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

greg.1.anderson’s picture

Remove some dead code, fix some code style problems, and fix a couple of broken tests.

greg.1.anderson’s picture

Status: Needs work » Needs review

... and run the tests.

greg.1.anderson’s picture

Issue summary: View changes
Mixologic’s picture

Okay, I reviewed, screenshared with @greg.1.anderson and shuffled some things around.

Removed the defaults from the code - they now read in the existing composer.json's - This is under the assumption that 'codebase wide' composer.json scans happen and we dont really want folks to have to remember where to change additional things, because its in php instead of in json on the filesystem.

Also, I moved the lookup table for the pinned dev dependencies to be more like it is for the regular dependencies -> a couple of if statements with a TODO to remove later once upstream gives us something to move to.

Also found/removed several instances of dead code, as well as some places where earlier branch compatibilty stuff was coming in from the upstream package generator.

Minor docblock fixes too.

Mixologic’s picture

I had one question: wasnt sure about why we have drupal.core as a requirement of pinned dependencies.

greg.1.anderson’s picture

drupal/core is a requirement of pinned dependencies because if you care to strongly constrain your dependency versions, it's important that they are strongly constrained to something specific. It wouldn't make much sense, for example, if you constrained your drupal/core to ~8.7.0 and then your "pinned" dev dependencies floated up to ^8.8.

I think that only the tarball generator will use pinned dependencies, for the most part.

Mixologic’s picture

Ah, sure. that makes sense.

+++ b/composer/Metapackage/PinnedDevDependencies/composer.json
@@ -7,7 +7,6 @@
-        "drupal/core": "self.version",

Oops. I had mistakenly taken these out, then put them back in on second thought and missed this one.

Also, Im going to trim the fixutres - they dont need any of their requires in there, plus that'll make for a better test by proving theyre different.

Mixologic’s picture

AIght. added that pinned dev dependencies thing back in.

Made a couple other small changes: The 'no newline at end of file ' was probably going to make many other patch submitters annoyed, so I altered the trims to not check for newlines,

I stripped out some of the unnecessary stuff from the fixtures, and adjusted the output message to include the full relative path of 'core/Metapackage'.

Status: Needs review » Needs work

The last submitted patch, 21: 3087626-21.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Mixologic’s picture

Status: Needs work » Needs review
FileSize
259.9 KB
5.3 KB

coding standards and test fixture fixes to fix fixture test.

greg.1.anderson’s picture

Status: Needs review » Needs work

I made this review on #21 w/out reading #23 (cross-post):

  1. +++ b/composer/Metapackage/Generator/Builder/DrupalCoreRecommendedBuilder.php
    @@ -0,0 +1,49 @@
    +    unset($composer['require']);
    

    I'd just:
    ```
    $composer['require'] = [
    'drupal/core' => 'self.version',
    ];
    ```

  2. +++ b/composer/Metapackage/Generator/Builder/DrupalDevDependenciesBuilder.php
    @@ -0,0 +1,44 @@
    +    unset($composer['require']);
    

    No need to unset here, we replace on the next line.

  3. +++ b/composer/Metapackage/Generator/Builder/DrupalPinnedDevDependenciesBuilder.php
    @@ -0,0 +1,51 @@
    +    unset($composer['require']);
    
Mixologic’s picture

Yeah, I just blindly migrated the clearing out to the other two classes. Thats a better strategy.

Mixologic’s picture

Status: Needs work » Needs review
Mixologic’s picture

The interdiff in #25 is still relevant

greg.1.anderson’s picture

I am +1 on #27, but cannot RTBC a patch I started.

n.b. Still only runs on post-update-cmd. If folks have trouble after reverting composer.lock, they can always run composer update --lock. That's my preference, anyway.

jibran’s picture

This looks ready. Just a couple of minor points.

  1. +++ b/composer/Metapackage/Generator/Builder/DrupalCoreRecommendedBuilder.php
    @@ -0,0 +1,47 @@
    +      // If there is no 'source' record, then this is a path repository or something.
    
    +++ b/composer/Metapackage/Generator/Builder/DrupalPinnedDevDependenciesBuilder.php
    @@ -0,0 +1,48 @@
    +        // If the require-dev is bringing in a dev version of behat/mink, convert
    

    > 80 char. We need a follow-up to add composer dir to core/phpcs.xml.dist

  2. +++ b/composer/Metapackage/Generator/Builder/DrupalPinnedDevDependenciesBuilder.php
    @@ -0,0 +1,48 @@
    +          $composer['require']['behat/mink'] = '1.8.0 | 1.7.1.1 | 1.7.x-dev';
    ...
    +          $composer['require']['behat/mink-selenium2-driver'] = '1.4.0 | 1.3.1.1 | 1.3.x-dev';
    

    Pinning a version is the same thing as mentioning commit SHA so why can't we leave these a commit SHA?

  3. +++ b/core/tests/Drupal/Tests/Composer/Metapackage/fixtures/Metapackage/PinnedDevDependencies/composer.json
    index 906b45d4e5..fccfd7bec0 100644
    --- a/core/tests/Drupal/Tests/Core/Composer/ComposerTest.php
    
    --- a/core/tests/Drupal/Tests/Core/Composer/ComposerTest.php
    +++ b/core/tests/Drupal/Tests/Core/Composer/ComposerTest.php
    

    Let's move the test to Drupal\Test\Composer dir and namespace as well.

jibran’s picture

I tired the composer script with #3088369: Update Drupal 9 to Symfony 4.4-dev works fine.

Ignore 2 in #29 it shows up as dev-master so changing this is fine.

Mile23’s picture

Status: Needs review » Needs work

Doing some manual testing....

composer require crell/api-problem

Yields a change to the lock file and thus drupal/core-recommended, with a nice round-trip on remove that ends up with a reverted drupal/core-recommended.

composer require --dev crell/api-problem

Again changes dev dependencies and pinned dev dependencies, with a successful round trip on remove.

So we try this:

git checkout -b 3087626
composer update drupal/core drupal/core-vendor-hardening

This updates the lock file:

diff --git a/composer.lock b/composer.lock
index a3eef1b952..f3eca39380 100644
--- a/composer.lock
+++ b/composer.lock
@@ -649,7 +649,7 @@
         },
         {
             "name": "drupal/core",
-            "version": "8.8.x-dev",
+            "version": "3087626.x-dev",
             "dist": {
                 "type": "path",
                 "url": "core",
@@ -876,7 +876,7 @@
         },
         {
             "name": "drupal/core-vendor-hardening",
-            "version": "8.8.x-dev",
+            "version": "3087626.x-dev",
             "dist": {

So neither of these should make it into the metapackages, which they don't. However, now the lock file still contains the branch name.

We fix that:

COMPOSER_ROOT_VERSION=8.8.x-dev composer update drupal/core drupal/core-vendor-hardening

And all is well, our lock file is not touched.

Now I edit core/composer.json to require crell/api-problem @stable and run the same command:

COMPOSER_ROOT_VERSION=8.8.x-dev composer update drupal/core drupal/core-vendor-hardening

This adds the dependency and updates core-recommended.

Every time the metapackages are updated, I see this (with appropriate files listed):

> Drupal\Composer\Composer::generateMetapackages
Updated metapackage file composer/Metapackage/CoreRecommended/composer.json.
If you make a patch, ensure that the files above are included.

Being able to tell people to run composer update --lock if they revert their lockfile is adequate, but not perfect.

I'd RTBC except for #29. Also: I couldn't find where we exclude composer/ in phpcs.xml.dist.

jibran’s picture

We have

<?xml version="1.0" encoding="UTF-8"?>
<ruleset name="drupal_core">
  <description>Default PHP CodeSniffer configuration for Drupal core.</description>
  <file>.</file>

which means only core is checked.

Mile23’s picture

Derp. In a directory.

greg.1.anderson’s picture

Status: Needs work » Needs review
FileSize
2.6 KB
260.07 KB

#27.1: Wrapped. I'll write up a follow-up issue. Do we want to just include ../composer as an additional path to sniff? This is a bit odd (just like the tests in core that test code in composer), but duplicating the phpcs configuration seems like it would be even worse.

#27.2: Version constraints such as dev-master#sha may only be placed in root-level composer.json files. If this form is used anywhere else, the #sha part is ignored, and the version is interpreted as simply dev-master.

#27.3: Moved.

greg.1.anderson’s picture

jibran’s picture

Status: Needs review » Reviewed & tested by the community

Thanks, for addressing the feedback.

Mixologic’s picture

Status: Reviewed & tested by the community » Needs work

So I just realized that if we switch to this/backport it to 8.8.x, then the 8.9.x version of core is going to get the wikimedia/composer-merge-plugin back.

The external package generator removed that, so, I think we should make sure it doesn't make its way back into the tarballs.

greg.1.anderson’s picture

Status: Needs work » Needs review
+++ b/composer/Metapackage/Generator/Builder/DrupalCoreRecommendedBuilder.php
@@ -0,0 +1,48 @@
+    $remove_list = ['drupal/core', 'wikimedia/composer-merge-plugin'];

wikimedia/composer-merge-plugin is being removed here.

We want this removed in both the 8.8.x and 8.9.x versions of drupal/core-recommended.

We want to add wikimedia/composer-merge-plugin back in to the legacy tarball, but not the recommended tarball.

We will remove wikimedia/composer-merge-plugin completely in 9.x.

Mixologic’s picture

Status: Needs review » Reviewed & tested by the community

Doh. I was going off of my memory of having worked on the patch and not remembering that was there. My bad.

xjm’s picture

So this seems like a lot of overhead to maintain. Every time we update a dependency version, we have to change all of these things in all of these places?

jibran’s picture

@xjm They will be changed automatically with the post-install and post-update scripts.

@greg.1.anderson @Mixologic this reminds me, do we need post install script to compliment "post-update-cmd": "Drupal\\Composer\\Composer::generateMetapackages"?

Mixologic’s picture

It only needs to run when composer.lock changes, so post-install isnt necessary.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

I think we need a test to ensure that root composer.lock file is correct and the meta packages have been updated appropriately otherwise the committers will struggle to keep all this in sync.

Mixologic’s picture

Every time you update a dependency, a message will tell you they've been updated, and that you should add them to your commit.

They're essentially far less fiddly composer.lock files, and may help us eventually dispose of having a lockfile in git, and all the conflicts and rerolls that come along with it.

Okay, so a test somewhat similar to core/tests/Drupal/Tests/ComposerIntegrationTest.php->testComposerLockHash() ?

Thats a great idea. We should put the "content-hash": "8ba406bd7f3522d51f0e55fe33e51aff", from the composer.lock into the generated composer.json files and we can make the test compare them.

greg.1.anderson’s picture

I'm +1 on #43, but I don't think we should copy the content hash. The test can just run the generator and see if the metapackages match.

Mile23’s picture

This whole thing is supposed to get us around a corner: #3086644: LegacyProject composer templates wrongly reference 8.x + fix test coverage

We can't yet add a build test of composer create-project drupal/legacy-project because that project template uses drupal/core-recommended instead of drupal/core. drupal/core-recommended currently exists outside of whatever patch you might have made to the dependencies, since it's generated after commits. Therefore we might have a false pass/false fail on a patch that changes the dependencies, or we won't know if there is a dependency problem within drupal/legacy-project until after the commit happens. This applies to the drupal/recommended-project template as well.

The solution here is that instead of having it generated after commit, we generate it on Composer update, to be included in the patch. That way the metapackages are updated per patch. The downside is that there might be a couple extra files to deal with which are generated automatically.

See #31 for a walkthrough.

Note that this is all predicated on the need for a drupal/core-recommended so that we can lock down on a known-safe version of a given dependency.

A good test to add to this patch would be some of the commands from #31.

Mixologic’s picture

Issue summary: View changes

I assume #46 is in response to #40

But a clarifier:

The intention of the test in #44 is not to detect dependency problems. It's to detect a failure to add the correct metapackages to any submitted patch.

greg.1.anderson’s picture

I am wary about running `composer update` tests on the root of the system-under-test, and I'm not sure that the test would be valid on a copy unless we copied a lot of the source tree. We could perhaps do the latter type of testing in the Build Tests, and a create-project tests in the follow-on issue (#3086644: LegacyProject composer templates wrongly reference 8.x + fix test coverage).

Here, we should just verify that the metapackages are up-to-date with tests such as described in #43 - #45. If we do that, then we should be covered well enough. If `composer update` isn't working correctly, then the bad results will go into the patch, and we'll catch it in testing.

Mile23’s picture

I'm not sure that the test would be valid on a copy unless we copied a lot of the source tree. We could perhaps do the latter type of testing in the Build Tests

That's what I mean. I was thinking a test like this:

  • Copy source.
  • Remove composer.json from metapackages.
  • composer config repositories.empty path core/tests/Drupal/Tests/Composer/Plugin/Scaffold/fixtures/empty-fixture
  • composer require fixtures/empty-fixture
  • Check that new composer.json was generated per metapackage.
  • Maybe composer validate.

But this isn't a good test, because it doesn't demonstrate that we've added a dependency, since it skips path repos.

However..... Performing this test manually yields metapackage composer.json files without name, type, description, license, or conflict keys. So the generator should probably always generate those keys instead of relying on their being in place in the file.

So we go ahead and test the internet a little bit:

  • Copy source.
  • Remove composer.json from metapackages.
  • composer require something/something
  • Check that new composer.json was generated per metapackage.
  • Check that our package was added.
  • Maybe composer validate.
  • Repeat for composer require --dev.

This has the same missing keys problem as before, but also demonstrates that we get the desired outcome.

I'd write these tests, but then I can't review them. :-)

greg.1.anderson’s picture

So the generator should probably always generate those keys instead of relying on their being in place in the file.

I wrote it that way for testability, but that version was backed out in #27 because it reduced the size of the code.

I think we should do things like #49 as follow-on work when we do the Build Tests. I'll write the tests for #43 right now. We need to ro-sham-bo and decide of the initial metadata for the metapackages should be in code, or in-place in the metapackage target location.

Mixologic’s picture

Ro-sham-bo says "yes, put the metadata generation back into code" My initial objections were worries about cross codebase edits of composer.json files being able to hit the metadata packages.

Except we dont want that, and with a test like #43 suggests, we'll end up with some fails if somebody does do that, so that will protect us.

I believe the slack plan is:
re -add the metapackage generation back in.
have a test that does comparisons between whats in the metapackages vs whats in composer.lock to ensure they're equivalent.

greg.1.anderson’s picture

Status: Needs work » Needs review
FileSize
13.55 KB
262.66 KB

Here's an updated patch that puts the metapackage metadata back into code, and provides a test to confirm that the metapackages have been updated along with the most recent update to the composer.lock file.

Mixologic’s picture

Status: Needs review » Reviewed & tested by the community
  1. +++ b/core/tests/Drupal/Tests/Composer/Metapackage/MatapackageUpdateTest.php
    @@ -0,0 +1,85 @@
    +   * Note that this is not a test of code correctness, but rather it merely
    +   * confirms if the package builder was used on the most recent set of
    +   * metapackages.
    

    Solid ++!

  2. +++ b/core/tests/Drupal/Tests/Composer/Metapackage/MatapackageUpdateTest.php
    @@ -0,0 +1,85 @@
    +    $repositoryRoot = dirname(dirname(dirname(dirname(dirname(dirname(__DIR__))))));
    

    hadn't seen this before. I was used to $root = realpath(__DIR__ . '/../../../../'); kind of stuff, but then I went searching core and it seems like theres a lot of dirname russian dolls, so, +1

  3. +++ b/core/tests/Drupal/Tests/Composer/Metapackage/MatapackageUpdateTest.php
    @@ -0,0 +1,85 @@
    +To fix, run:
    +
    +    COMPOSER_ROOT_VERSION=$version composer update --lock
    +
    +__EOT__;
    +    $this->assertEquals($generatedJson, $loadedJson, $message);
    

    nice. Me gusta

jibran’s picture

+++ b/composer/Metapackage/Generator/Builder/DrupalDevDependenciesBuilder.php
@@ -0,0 +1,62 @@
+    if (isset($composer['require']['behat/mink']) && ($composer['require']['behat/mink'] == '1.7.x-dev')) {
...
+    if (isset($composer['require']['behat/mink-selenium2-driver']) && ($composer['require']['behat/mink-selenium2-driver'] == '1.3.x-dev')) {

+++ b/composer/Metapackage/Generator/Builder/DrupalPinnedDevDependenciesBuilder.php
@@ -0,0 +1,69 @@
+        if (($package['name'] == 'behat/mink') && ($package['version'] == 'dev-master')) {
...
+        if (($package['name'] == 'behat/mink-selenium2-driver') && ($package['version'] == 'dev-master')) {

Let's create a white list for exceptions so that we can keep track and add tests to make sure we don't forget to update these generators when we add/remove and exception. Thoughts?

greg.1.anderson’s picture

It is my hope that Drupal never again ships a stable release that contains a dependency required with a non-stable version constraint. It is also my hope that we eventually will get a stable tag for these two components, and will be able to simply remove these exceptions. I understand the circumstances that lead up to these components being included like this (and why we cannot just remove them now). If such an unusual set of circumstances happen to align again, then we can work on generalizing these exceptions as follow-on work. Hopefully it will never come to that.

Mixologic’s picture

Yeah re #54 I worry that a whitelist will get added to. Lets not.

greg.1.anderson’s picture

This should also be backported to 8.8.x

greg.1.anderson’s picture

Version: 8.9.x-dev » 9.0.x-dev
FileSize
363.79 KB
8.5 KB

Re-rolled for 9.0.x.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 58: 3087626-58.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

greg.1.anderson’s picture

Status: Needs work » Needs review
FileSize
7.95 KB
263.85 KB

Fixed some code style issues and added a TESTING.txt file.

Note: #58 inadvertently included the ProjectMessage plugin from another patch. This code is removed in this patch. The removal is not shown in the interdiff, though, as the removed code should not have been included and has no bearing on this issue.

Mixologic’s picture

Issue summary: View changes
Mixologic’s picture

Patch already needed a reroll from another composer.lock update.

Also, Renamed core/tests/Drupal/Tests/Composer/Metapackage/MatapackageUpdateTest.php to MetapackageUpdateTest.php.

Sneaky little vowel.

I did notice that for some reason, when rolling the patch, my composer.lock reverts from dev-master for the behat/mink-selenium2-driver to 1.3.x-dev version, which then causes the pinned-dev-dependencies to not get the wide net version we put in there:

1.4.0 | 1.3.1.1 | 1.3.x-dev

Im not sure what part of the composer lock updating mechanism switches it from dev-master _back_ to 1.3.x-dev but it happens somehow.

Status: Needs review » Needs work

The last submitted patch, 62: 3087626-62.patch, failed testing. View results

greg.1.anderson’s picture

The reason that behat/mink-selenium2-driver switches to 1.3.x-dev is that the 1.3 branch exists there now, whereas previously, 1.3 was a branch alias for master the last time that project was updated in the lock file (ergo the previous dev-master result).

We should add special checking for 1.3.x-dev in addition to dev-master, and also put in a similar check for 1.7.x-dev for behat/mink, in case they make a 1.7 branch for us there as well. (behat/mink will break all old Drupals if they change their branch alias from 1.7 to 1.8 without making the branch).

I'll make those adjustments.

greg.1.anderson’s picture

Oh, wait, it's going the other way -- it was 1.3.x-dev and is now dev-master. I do not know why, but it probably has something to do with the circumstances in #64.

greg.1.anderson’s picture

Status: Needs work » Needs review
FileSize
707 bytes
263.85 KB

This is odd:

$ COMPOSER_ROOT_VERSION=9.0.x-dev composer update behat/mink-selenium2-driver
> Drupal\Composer\Composer::ensureComposerVersion
Loading composer repositories with package information
Updating dependencies (including require-dev)
Package operations: 0 installs, 3 updates, 0 removals
  - Updating pear/archive_tar (1.4.7 => 1.4.8): Loading from cache
    Cleaning: pear/archive_tar
  - Updating typo3/phar-stream-wrapper (v3.1.2 => v3.1.3): Loading from cache
  - Removing drupal/core (9.0.x-dev), source is still present in core
  - Installing drupal/core (9.0.x-dev): Source already present
Package phpunit/phpunit-mock-objects is abandoned, you should avoid using it. No replacement was suggested.
Generating autoload files
> Drupal\Core\Composer\Composer::preAutoloadDump
Hardening vendor directory with .htaccess and web.config files.
Cleaning vendor directory.
> Drupal\Composer\Composer::generateMetapackages
Updated metapackage file composer/Metapackage/PinnedDevDependencies/composer.json.
If you make a patch, ensure that the files above are included.

After running ^^ there were no diffs in my composer.lock file, and behat/mink-selenium2-driver was dev-master in the lock file, as the package generator expects, so the metapackage was fixed up correctly.

Did you remember to remove vendor when switching branches? Composer can behave strangely if that's left around. Ah, and that's why I got some packages changing version when I ran composer update; they changed in the 9.0.x branch, and I did not run composer install before running composer update. Got it.

greg.1.anderson’s picture

Status: Needs review » Needs work

Per discussion with @mixologic and @mradcliff:

Composer wants to float the version of behat/mink-selenium2-driver up to 1.3.x-dev, because there is a 1.3 branch in that project now. Previously that version was converted to dev-master (what's committed in the composer.lock file right now) because there was a branch alias from 1.3.x-dev to dev-master. That's gone now in the upstream project, but it persists in our lock file, resulting in inconsistent behavior.

I'm going to force behat/mink-selenium2-driver up to 1.3.x-dev in our lock file so that core contributors do not keep encountering this problem, which causes the metapackage to be generated incorrectly and results in a corresponding test failure -- and it's hard to know why that happened. Fixing this here will avoid that future pain.

greg.1.anderson’s picture

Status: Needs work » Needs review

I would recommend crediting @mradcliff on this issue; I don't seem to have the permissions to do that.

greg.1.anderson’s picture

Fixed per #67. Sorry if this caused any confusion, but I accidentally named the patch in #66 #68.

Note that now we are going to need to backport to 8.9.x; we won't be able to use the earlier patches. I'll do that after this is committed.

Mixologic’s picture

Status: Needs review » Reviewed & tested by the community

Okay. this is looking solid now, and will allow us to have the metapackages as part of core now, and thus they cannot get out of sync.

greg.1.anderson’s picture

Version: 9.0.x-dev » 8.8.x-dev
FileSize
265.62 KB

Here is the backport to 8.8.x. I understand that we want to label our patches with the lowest release they will be committed to now.

greg.1.anderson’s picture

#71 applies to the 8.9.x branch too, so I queued up another test for that.

xjm’s picture

Version: 8.8.x-dev » 8.9.x-dev

Thanks @greg.1.anderson! For the most part that's true, but I left a nuance out of the tweet. :) Patches may be backported to 8.8.x at committer discretion but should be committed to 8.9.x before deciding that under policy. As per https://groups.drupal.org/node/535412:

Most issues that are only allowed in minor releases will be committed to 8.9.x and 9.0.x only. A few strategic issues may be backported to 8.8.x, but only at committer discretion after the issue is fixed in 8.9.x (so leave them set to 8.9.x unless you are a committer), and only up until the beta deadline.

So, this isn't guaranteed to go into 8.8.x (but there's probably a strong need/case for that based on what I heard from @larowlan or @webchick or someone; I don't know all the details).

Unrelated comment. I still see four statically coded places in the patch (in addition to the two already in HEAD in composer.json and composer.lock) that have to track dependency versions, and no update to documentation or committer tooling or workflows to help maintain them. What's the plan for that? We're at least missing the docs gate here for it, and I'd like to know how those of us not involved with this initiative will be able to maintain this without having to grep core for a unique version number string to find the places.

greg.1.anderson’s picture

Priority: Normal » Critical

@xjm: The composer.json files in the Metapackages directory are generated files. They are updated automatically by a Composer post-update command whenever composer.lock changes. Whenever the Metapackages are updated, you will see a message similar to this:

> Drupal\Composer\Composer::generateMetapackages
Updated metapackage file composer/Metapackage/PinnedDevDependencies/composer.json.
If you make a patch, ensure that the files above are included.

Regarding documentation, it would be swell if the drupal/drupal repository had documentation in markdown that caused pages to be published on drupal.org when the patch was committed. Since we don't have that, though, I am left in sort of a quandary about what to do with issues that alter behavior described in handbook pages. If we alter the handbook before the patch is committed, then existing users will be confused that the documentation does not match the behavior. If we wait until after the patch is committed, then the same problem exists on the other end. If you'd like me to update anything before this patch goes in (maybe with a "disclaimer: coming soon" prequel?), then I'd be glad to do so.

This patch really really needs to be backported to 8.8.x; however, I'm happy to follow whatever process for doing that is appropriate. We have a patch that works in both 8.9.x and 8.8.x; we can commit to 9.0.x and 8.9.x first, and then have whatever discussion is necessary to decide whether to put it into 8.8.x as well.

Bumping the priority here up to "critical", as this patch is blocking making Drush work with Drupal 9.

xjm’s picture

Technically, "blocked not-core-project" would be major/contributed project blocker, but I guess we kind of know already that broken Drush is a special case and will be perceived as a critical regression, so I guess critical's OK here.

Also, I'm not sure how wise it is to have tracked core files being automatically changed by a script, such that a script is patching core? Are we expecting committers to do this? Would it happen with just a composer install, which is the only thing that happens currently on core commit? And also, having committers expected to commit something that's different from what's in a patch is also error-prone and problematic. (All these things are specifics that should go in whatever docs, also.) :)

I don't know where to document it, but it needs to go somewhere in the contributor guide and also in the committer handbook and committer tools if there's something committers have to do that's different from what we do now. Someone on a different issue pointed out that we utterly lack info about composer in the patch creation handbook page, so this would be added there at least.

One can always put a line in the handbook (or change record, release notes, etc.) like "Following [#1234567], [...]" to get around the chicken-and-egg problem. So yah, essentially "Disclaimer: Coming soon".

Mile23’s picture

These changes would only happen when someone updates the lock file.

So it goes like this:

  • composer install
  • [work on stuff]
  • composer update something/something
  • [this updates the lock file, and also generates a new set of metapackages]
  • git add my/work.php
  • git add composer/Metapackages
  • git commit -m 'bunch of work'
  • git diff 8.9.x > my.patch

The reason we do this is so that there's a path repo metapackage in core that represents the lock file. If we don't have that, we can't test building the project templates, per #3086644: LegacyProject composer templates wrongly reference 8.x + fix test coverage So this is how we test whether your patch breaks composer create-project.

webchick’s picture

So would the instructions for patch authors / committers basically read:

"In the event that you are adding or changing Composer libraries/dependencies, you need to do this..."
[what you said]
"Otherwise, the standard process applies"

?

I think that's what we really need to hone in on in the docs (and even more ideally, something like https://github.com/alexpott/d8githooks would remind us if we forget). Patch authors and committers both have decades of "muscle memory" around their respective workflows.

Mile23’s picture

Yah, definitely changes to workflows can be hard to remember. Like in #74, if needed, it tells you:

> Drupal\Composer\Composer::generateMetapackages
Updated metapackage file composer/Metapackage/PinnedDevDependencies/composer.json.
If you make a patch, ensure that the files above are included.

And those changed files will show up in git status and so forth.

I have no doubt there will be other workflow changes as we continue with the composer stuff. This one stands out because we have to test the metapackages per patch, so it has to be in the patch.

But there's not really an extra step here, other than to be sure you add the changes to the metapackages if they've been changed. That could happen if you composer update anything. It's also possible that there won't be a change if you composer update and it ends up with the same lock values.

greg.1.anderson’s picture

@webchick: The muscle memory will be about the same. The difference is that if you run composer.lock, you might get modifications in a few more files in addition to composer.lock. You just need to make sure that these modified files become part of your patch. If someone forgets, there's a test that will fail.

We will update documentation to explain more fully.

greg.1.anderson’s picture

Places to potentially document this behavior:

  1. A child page of the Drupal Core handbook page, perhaps adjacent to the page Core dependency release cycles, security information, and evaluation criteria.
  2. Advanced patch contributor's guide
  3. On a new page, Core contributor's patch guide, sibling page to Advanced patch contributor's guide.

We will update one or more of these pages.

Mixologic’s picture

https://www.drupal.org/node/3089540 is a documentation page I started on that really gets into "As a core developer, I need to change the dependencies of drupal core"

This patch is a *very* small part of it, adds an additional step to be aware of (up to 4 generated files instead of 1), catches you with a test if you didn't see the output, and haven't read the docs.

The impact of these changes should be mostly minimal in that the *only* time it affects people is when they are updating core dependencies.

The git history of the lockfile shows that this is around 3-4 times a month.

Theres an absurd flurry of changes happening now because composer intitative tends to touch composer stuff, and 9.0.x has many,many major updates.

xjm’s picture

So https://www.drupal.org/node/3089540 looks great (thanks @Mixologic!) but I have a new reason to be concerned here:

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.

This strikes me as... very bad?

Edit: Ryan tells me that is another evil side effect of #2912387: Stop using wikimedia/composer-merge-plugin and unrelated to this here issue. (Still... very bad...)

greg.1.anderson’s picture

This patch is moving us in a better direction. I think we should consider an upstream change to Composer to prevent it from ever upgrading packages from path repositories.

larowlan’s picture

Coming in late here, but is this analogous to the es6 vs generated files in Javascript?

In which case we have building tooling in the d8githooks that prevents commits that update one but not the other.

We already do a composer install as part of that, I guess it would need to validate that the meta-packages match?

greg.1.anderson’s picture

@larowlan: Yes, we have a test that verifies that the committed metapackage matches what the composer.lock generates. If someone forgets to commit one, then they will get a test failure.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. Thanks for adding \Drupal\Tests\Composer\Metapackage\MetapackageUpdateTest this will really help core committers have confidence that the metapackages are correct. As far as I can see this patch adds no new burden on core committers that's not already been added by #2912387: Stop using wikimedia/composer-merge-plugin so for that perspective I'm +1.

    I'm more than +1 though because this also neatly solves problems we've had we've keeping webflo/drupal-core-strict and the new drupal/core-recommended up-to-date without any more magic or githooks so that's fantastic.

  2. We're going to need both a 9.0.x and 8.x.x version here because we need to commit it to 9.0.x first and we need to backport this to 8.8.x in order to make sure we don't get in a mess with 8.8.0 and drupal/core-recommended
  3. Running env COMPOSER_ROOT_VERSION=9.0.x-dev composer update --lock shows all the deps in the metapackages moving to Symfony 4 as expected - nice!
  4. +++ b/composer/Metapackage/Generator/BuilderInterface.php
    @@ -0,0 +1,45 @@
    +/**
    + * Produce the output for a metapackage.
    + *
    + * BuilderInterface provides an interface for builder claases which are
    + * called by the PackageGenerator in order to produce a derived metapackage from
    + * the provided source package.
    

    I think this documentation could be improved to describe what a metapackage actually is.

  5. +++ b/composer/Metapackage/Generator/BuilderInterface.php
    @@ -0,0 +1,45 @@
    + * BuilderInterface provides an interface for builder claases which are
    

    typo... classes

  6. +++ b/composer/Metapackage/Generator/BuilderInterface.php
    @@ -0,0 +1,45 @@
    + * The builder class is responsible for two things:
    + *
    + * - getPath: The relative path to the metapackage
    + * - getPackage: The contents of the composer.json file for the new metapackage
    

    This is odd documentation. The methods are just below - i think these docs should be on the respective method if there's anything here that's not covered.

  7. +++ b/core/tests/Drupal/Tests/Composer/Metapackage/BuilderTest.php
    @@ -0,0 +1,210 @@
    +/**
    + * Test DrupalCoreRecommendedBuilder
    + *
    + * @group Metapackage
    + */
    +class BuilderTest extends TestCase {
    +
    +  /**
    +   * Test data for testBuilder
    +   */
    +  public function builderTestData() {
    
    +++ b/core/tests/Drupal/Tests/Composer/Metapackage/fixtures/Drupal/composer.json
    --- /dev/null
    +++ b/core/tests/Drupal/Tests/Composer/Metapackage/fixtures/Drupal/composer.lock
    

    I'm pondering the utility of this test given we have the implicit testing of MetapackageUpdateTest

    This test will get more and more stale over time. I think we should drop it and all the associated fixtures.

  8. I think the actual metapackages and the code to generate shouldn't be at the same level... ie.
    composer/Metapackage/CoreRecommended
    composer/Metapackage/DevDependencies
    composer/Metapackage/PinnedDevDependencies
    vs
    composer/Metapackage/Generator

    I think maybe metapackages should be inside some subfolder or maybe we should have composer/Metapackages and composer/MetapackageGenerator

  9. If I apply the patch to 8.9.x and run env COMPOSER_ROOT_VERSION=8.9.x-dev composer update --lock or env COMPOSER_ROOT_VERSION=8.9.x-dev composer update drupal/core I get the following changes:
    diff --git a/composer.lock b/composer.lock
    index 25bded159a..ef365a0bbc 100644
    --- a/composer.lock
    +++ b/composer.lock
    @@ -3554,16 +3554,16 @@
             },
             {
                 "name": "behat/mink-selenium2-driver",
    -            "version": "1.3.x-dev",
    +            "version": "dev-master",
                 "source": {
                     "type": "git",
                     "url": "https://github.com/minkphp/MinkSelenium2Driver.git",
    -                "reference": "0a09c4341621fca937a726827611b20ce3e2c259"
    +                "reference": "8684ee4e634db7abda9039ea53545f86fc1e105a"
                 },
                 "dist": {
                     "type": "zip",
    -                "url": "https://api.github.com/repos/minkphp/MinkSelenium2Driver/zipball/0a09c4341621fca937a726827611b20ce3e2c259",
    -                "reference": "0a09c4341621fca937a726827611b20ce3e2c259",
    +                "url": "https://api.github.com/repos/minkphp/MinkSelenium2Driver/zipball/8684ee4e634db7abda9039ea53545f86fc1e105a",
    +                "reference": "8684ee4e634db7abda9039ea53545f86fc1e105a",
                     "shasum": ""
                 },
                 "require": {
    @@ -3590,15 +3590,15 @@
                     "MIT"
                 ],
                 "authors": [
    -                {
    -                    "name": "Pete Otaqui",
    -                    "email": "pete@otaqui.com",
    -                    "homepage": "https://github.com/pete-otaqui"
    -                },
                     {
                         "name": "Konstantin Kudryashov",
                         "email": "ever.zet@gmail.com",
                         "homepage": "http://everzet.com"
    +                },
    +                {
    +                    "name": "Pete Otaqui",
    +                    "email": "pete@otaqui.com",
    +                    "homepage": "https://github.com/pete-otaqui"
                     }
                 ],
                 "description": "Selenium2 (WebDriver) driver for Mink framework",
    @@ -3611,7 +3611,7 @@
                     "testing",
                     "webdriver"
                 ],
    -            "time": "2019-09-02T09:46:54+00:00"
    +            "time": "2018-10-10T12:39:06+00:00"
             },
             {
                 "name": "composer/ca-bundle",
    diff --git a/composer/Metapackage/PinnedDevDependencies/composer.json b/composer/Metapackage/PinnedDevDependencies/composer.json
    index 0955f3521d..d0b75a71ca 100644
    --- a/composer/Metapackage/PinnedDevDependencies/composer.json
    +++ b/composer/Metapackage/PinnedDevDependencies/composer.json
    @@ -11,7 +11,7 @@
             "behat/mink": "1.8.0 | 1.7.1.1 | 1.7.x-dev",
             "behat/mink-browserkit-driver": "1.3.3",
             "behat/mink-goutte-driver": "v1.2.1",
    -        "behat/mink-selenium2-driver": "1.4.0 | 1.3.1.1 | 1.3.x-dev",
    +        "behat/mink-selenium2-driver": "dev-master",
             "composer/ca-bundle": "1.2.4",
             "composer/composer": "1.9.0",
             "composer/spdx-licenses": "1.5.2",
    

    Which is quite unexpected.

If we are going to keep \Drupal\Tests\Composer\Metapackage\BuilderTest (I hope not) then we need to also address:

  1. +++ b/core/tests/Drupal/Tests/Composer/Metapackage/Fixtures.php
    @@ -0,0 +1,48 @@
    +
    +  public function drupalCoreComposerFixture($project_name = 'Drupal') {
    +    return new DrupalCoreComposer($this->projectPath($project_name));
    +  }
    

    Needs docs

  2. +++ b/core/tests/Drupal/Tests/Composer/Metapackage/Fixtures.php
    @@ -0,0 +1,48 @@
    +  public function drupalCoreMetapackageFixture($metapackage_name = 'CoreRecommended') {
    +    return json_decode(file_get_contents($this->projectPath('Metapackage') . '/' . $metapackage_name . '/composer.json'), TRUE);
    +
    +  }
    

    This is not used.

greg.1.anderson’s picture

9. Remove your vendor directory, maybe? The result you are getting is caused by having a branch alias for 1.3.x-dev to dev-master for the behat/mink-selenium2-driver. This was happening to us before because that branch alias was in the composer.lock file. That one should have gone away when you applied the patch. I'll experiment and try to reproduce later.

We need to keep BuilderTest because that is the test that determines that the Metapackage Generator is producing correct results. The other test compares the output from the Metapackage Generator (at pre-commit time) with the output of the Metapackage Generator (at test time). That test only confirms that the Metapackage Generator was run. It does not determine that the code produced was correct.

It does not matter if the BuilderTest fixtures become stale, because there is no reason for the fixtures to be the same as the Drupal core contents. In fact, the reason the fixtures exist directly rather than using Drupal's own lockfile (as the other test does) is to avoid having to make continuous updates to the tests as Drupal changes. Maybe it's confusing that the fixtures are an old version of Drupal. This was handy during the initial development, as it was easy to see in the tests that the results looked like we expected them to. If it would be less confusing, though, we could use a fixture that did not look anything like Drupal at all.

I'll fix up the other requested changes later today. n.b. we have two versions of the patch currently, for 8.9.x and 9.0.x. I'll make update both versions when I make the other fixes.

alexpott’s picture

We need to keep BuilderTest because that is the test that determines that the Metapackage Generator is producing correct results. The other test compares the output from the Metapackage Generator (at pre-commit time) with the output of the Metapackage Generator (at test time). That test only confirms that the Metapackage Generator was run. It does not determine that the code produced was correct.

Committing a whole composer.lock file for Drupal as test fixture seems likely to cause confusion. It would be less confusing if the fixtures didn't exist as composer.lock and composer.json files at all. We could inline a tiny composer.json and lock in the test.

I also question whether

That test only confirms that the Metapackage Generator was run. It does not determine that the code produced was correct.

is really the case. Correctness for these metapackages can't really be determined by this either. A correct metapackage is totally determined by Drupal's current set of dependencies and bugs in this code are unlike to be found by only this test. If we break the generator both tests will fail. If we've got something wrong it's most likely that usage of the meta-package is what'll discover that - not either of these tests.

greg.1.anderson’s picture

A correct metapackage is totally determined by Drupal's current set of dependencies and bugs

You are talking about correctness from the standpoint of a functional test. We will add functional tests for the metapackages as build tests in a follow-on issue (#3086644: LegacyProject composer templates wrongly reference 8.x + fix test coverage). I was talking about correctness from the standpoint of a unit test. Given that we have methods that take inputs, we can assert that the outputs match our expectations. The BuilderTest does this, whereas the other one does not. I think that we need the functional test, the unit test and the test that checks to see if the metapackage generator runs, as they all perform useful and important functions.

I will simplify the fixtures for the BuilderTest and inline them. It will only take a few packages to prove correctness.

greg.1.anderson’s picture

Status: Needs work » Needs review
FileSize
261.13 KB
47.98 KB

OK here's an updated patch addressing #86.

1. Thanks!
2. I made the 8.9.x / 8.8.x version first; I will also re-roll for 9.0.x while the tests run here.
3. Thanks!
4. I added a README to the Metapackage directory, and referenced it from the interface comment.
5. Fixed.
6. Removed.
7. Simplified the unit test to use a minimal composer.lock file inlined in the tests.
8. Reorganized as suggested
9. This can happen if you have a vendor directory with the old behat/mink-selenium2-driver installed. I added a safety-check with instructions on what to do if your dependencies are out-of-date and causing problems.

1. Rewrote the way this class worked, adding docblock comments to all surviving methods. This also included the removal of the composer.lock fixture that was being used by the unit tests.
2. Removed.

greg.1.anderson’s picture

Version: 8.9.x-dev » 9.0.x-dev
FileSize
7.45 KB
50.91 KB

Here's the 9.0.x version of the patch.

The last submitted patch, 90: 3087626-90.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Status: Needs review » Needs work

The last submitted patch, 91: 3087626-91.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

greg.1.anderson’s picture

Code style fixes and a unit-test fix.

greg.1.anderson’s picture

Status: Needs work » Needs review

Argh. Run the test.

greg.1.anderson’s picture

This has an additional bugfix over #94.

The error message for a bad selenium2 driver now reads:

Script Drupal\Composer\Composer::ensureBehatDriverVersions handling the post-update-cmd event terminated with an exception

  [RuntimeException]                                                                 
  Drupal requires behat/mink-selenium2-driver:1.3.x-dev in its composer.json        
  file, but it is pinned to dev-master in the composer.lock file.                     
  This sometimes happens when Composer becomes confused. To fix:                     
                                                                                     
  1. `git checkout -- composer.lock`, or otherwise reset to a known-good lock file.  
  2. `rm -rf vendor`                                                                 
  3. `composer install`                                                              
  4. `COMPOSER_ROOT_VERSION=8.9.x-dev composer update ...` (where ... is             
     the update arguments you wish to run, e.g. --lock).                             

Previously, COMPOSER_ROOT_VERSION=8.9.x-dev was rendered as 8.9.0-dev instead of 8.9.x-dev (fixed above).

Also switching to 8.9.x branch here; will post a 9.0.x equivalent next.

greg.1.anderson’s picture

Here's the patch for #94 (9.0.x) updated with the bugfix from #96.

jibran’s picture

Sorry one minor thing.

+++ b/composer/Composer.php
@@ -68,4 +68,14 @@ public static function ensureBehatDriverVersions() {
+  public static function drupalVersionBranch() {
+    return preg_replace('#\.[0-9]+-dev#', '.x-dev', \Drupal::VERSION);
+  }

Let's add a quick test for this method.

alexpott’s picture

Status: Needs review » Needs work
  1. This will need a reroll as the lock file has just changed.
  2. +++ b/core/tests/Drupal/Tests/Composer/Generator/BuilderTest.php
    @@ -0,0 +1,101 @@
    +/**
    + * Test DrupalCoreRecommendedBuilder
    + *
    + * @group Metapackage
    + */
    +class BuilderTest extends TestCase {
    

    This test is not way more manageable - @greg.1.anderson thank you. [EDIT: the not should not have been here - sorry]

  3. +++ b/composer/Generator/Util/DrupalCoreComposer.php
    @@ -0,0 +1,120 @@
    +  /**
    +   * DrupalCoreComposer factory.
    +   *
    +   * @param string $repositoryPath
    +   *   Path to a directory containing a composer.json and composer.lock files.
    +   */
    +  public static function createFromPath(string $repositoryPath) {
    

    Missing @return docs.

  4. +++ b/core/tests/Drupal/Tests/Composer/Generator/BuilderTest.php
    @@ -0,0 +1,101 @@
    +  public function testBuilder($builderClass, $referenceName, $expected) {
    +    $fixtures = new Fixtures();
    +    $drupalCoreInfo = $fixtures->drupalCoreComposerFixture($referenceName);
    

    $referenceName can be removed. It's not used.

jibran’s picture

After discussing with @greg.1.anderson I think we can ignore my feedback from #98.

greg.1.anderson’s picture

Status: Needs work » Needs review
FileSize
2.82 KB
51.27 KB

1. Rerolled

2. Thanks!

3. Rounded out the missing part of the docblock

4. Removed unused fixture value

greg.1.anderson’s picture

Mile23’s picture

The 8.8.x patch in #102 doesn't apply, only because of the content hash in the lock file. That could probably be fixed on commit, but how about an 8.9.x patch, commit, backport.

We should probably ask a maintainer which branch to RTBC first and so forth.

Also this CS error, tho it's kind of a nit:

/var/lib/drupalci/workspace/jenkins-drupal_patches-14425/source/core/tests/Drupal/Tests/Composer/Generator/Fixtures.php ✗ 1 more
15	Data types in @return tags need to be fully namespaced

There are also a ton of CS errors from CSS and JS, but they're unrelated.

greg.1.anderson’s picture

Version: 9.0.x-dev » 8.9.x-dev
Issue summary: View changes
FileSize
590 bytes
176.24 KB

OK, I'm not sure that I have the most up-to-date understanding of how Core development is supposed to happen, but I think:

  • This issue should be labeled "Version 8.9.x-dev", because it is intended to be committed there.
  • I have been making 8.9.x-dev patches so that it can be committed there, but I'm unsure if this is necessary.
  • I will start making the 9.0.x-dev patch the last one posted, since it's supposed to be the first one committed, but I'm unsure if this is necessary.

Along those lines, here's a fix on the 8.9.x branch for the CS error mentioned in #103. I also added a "steps to test" in the issue summary in case that's helpful for anyone once the tests come back green.

greg.1.anderson’s picture

And now, keeping "Version" ad "8.9.x-dev", here is the 9.0.x version of the patch, which I presume is committed first. Actually I think both 9.0.x and 8.9.x will be committed at about the same time, so I don't know if order is relevant.

No 8.8.x patch yet; I will backport once these are committed.

greg.1.anderson’s picture

Issue summary: View changes

Fix typo: ad-hoc test instructions are for 8.9.x-dev.

greg.1.anderson’s picture

Issue summary: View changes

Recommend a more recent patch in the ad-hoc instructions.

greg.1.anderson’s picture

Issue summary: View changes

It's important to remove the `vendor` directory if you've applied this patch before (vendor must be clean)

greg.1.anderson’s picture

Issue summary: View changes

Be more explicit about the error message you see in step 5.

greg.1.anderson’s picture

Issue summary: View changes

Update the ad-hoc testing instructions; user must create a branch and commit in order to use 'git diff'

greg.1.anderson’s picture

Issue summary: View changes

Fix numbers in instructions list

Mile23’s picture

Status: Needs review » Reviewed & tested by the community

Can confirm that both #104 and #105 pass the manual test in the IS. That is, the 8.9.x and the 9.0.x versions work.

They demonstrate that we're catching the errant lock file changes to behat versions, and that the instructions they give actually fix the problem.

We'll need a backport to 8.8.x, per the tag.

jibran’s picture

RTBC+1, nice work @greg.1.anderson. Just on nit not a commit blocker.

+++ b/composer/Composer.php
@@ -0,0 +1,81 @@
+   * @todo: Remove this once we have a stable release.

Maybe link the @todo to https://www.drupal.org/node/3078671

greg.1.anderson’s picture

I updated #3078671: Pin behat/mink and behat/mink-selenium2-driver to use resolvable release to contain another task to remove the test and script that will not be necessary when that issue is done.

alexpott’s picture

Saving issue credit

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

The 8.9.x in #104 contains unrelated change. I think we need to commit this to 9.0.x and 8.9.x at the same time.

Some very small nits.

+++ b/composer/Generator/Util/DrupalCoreComposer.php
@@ -0,0 +1,123 @@
+   * @return DrupalCoreComposer
+   *   New DrupalCoreComposer object containing composer.json and lock data.

+++ b/core/tests/Drupal/Tests/Composer/Generator/Fixtures.php
@@ -0,0 +1,109 @@
+   * @return Drupal\Composer\Generator\Util\DrupalCoreComposer
+   *   DrupalCoreComposer fixture.

Should be a fully qualified class name with a leading \

+++ b/composer/Generator/PackageGenerator.php
@@ -0,0 +1,127 @@
+   * @return BuilderInterface[]
+   */
+  protected function builders() {
...
+   * @param BuilderInterface $builder
+   *   An object that can build a metapackage.
+   *

FQCN too...

jibran’s picture

Assigned: Unassigned » jibran

Addressing #116

jibran’s picture

Assigned: jibran » Unassigned
Status: Needs work » Reviewed & tested by the community
FileSize
2.24 KB
2.86 KB
51.3 KB
50.92 KB
50.92 KB

Setting it RTBC as it is just a reroll/rebase and doc fixes. Uploaded three patches for committer's connivance.

jibran’s picture

FileSize
4.13 KB

Uploading the diff between 8.x and 9.x patches to make sure that code is the same and the only difference is composer files.

alexpott’s picture

Version: 8.9.x-dev » 8.8.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed 60b4730 and pushed to 9.0.x. Thanks!
Committed 55580b7 and pushed to 8.9.x. Thanks!

Ands asked @catch about backporting to 8.8.x as that needs RM approval.

  • alexpott committed 60b4730 on 9.0.x
    Issue #3087626 by greg.1.anderson, Mixologic, jibran, Mile23, alexpott,...

  • alexpott committed 55580b7 on 8.9.x
    Issue #3087626 by greg.1.anderson, Mixologic, jibran, Mile23, alexpott,...
alexpott’s picture

Status: Patch (to be ported) » Fixed

Discussed with @catch who is concerned about the current changes to the way we use composer making things that used to be easy harder but agrees that we should put this in 8.8.x for as "we have to push forward it could just be messy for a while".

Committed 770237b and pushed to 8.8.x. Thanks!

  • alexpott committed 770237b on 8.8.x
    Issue #3087626 by greg.1.anderson, Mixologic, jibran, Mile23, alexpott,...
jibran’s picture

Removing the tag.

mondrake’s picture

I don't know if this is related, but this was filed in Imagemagick's issue queue: #3090559: Composer install fails with "Package drupal/file_mdm_exif-1.1.0.0 must have a source or dist specified". Any comment or review appreciated.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

cilefen’s picture