Add php-tuf/composer-stager to core dependencies and governance — for experimental Automatic Updates & Project Browser modules

I never looked at this again because it was still marked . But it looks like ever since #9, it's been ready for review 😅

Slightly tweaked the issue summary markup for easier reading 👍

One necessary next step here is to open issues for each of Composer Stager's dev dependencies that isn't already a core dev dependency, that explains the benefit of that dev dependency, so that we can evaluate whether the benefit is worth whatever the burden of that dependency is.

https://github.com/php-tuf/composer-stager/issues/56 is an example where we've started the discussion for infection/infection, and I think the conclusion there so far is to remove that dependency unless @TravisCarden wants to argue for keeping it.

Reviewing the code itself can happen in parallel to that, so I agree with "Needs review" as the correct issue status here.

- Composer Stager is currently in the PHP-TUF namespace/organization. Decide if we want to move it into the Drupal namespace/organization.
- Decide if we want to continue maintaining it as a GitHub repo, or whether to move it to drupal.org's GitLab, or even into the Drupal core repository and mirrored to GitHub, like we do for https://github.com/drupal/recommended-project.

Removing these from the remaining tasks, because I discussed this with other committers, and we decided to keep it in the PHP-TUF namespace and on GitHub, to make it easier for other projects (e.g., Joomla, Typo3) to co-maintain it with us if they want to.

- Decide if/how we want to empower non-Drupal projects to help contribute to / maintain Composer Stager, if it's under Drupal core's governance.

I replaced that with:

Add needed governance documents (security policy, links to Drupal core governance documents, etc.) to the repo.

Added a remaining task of performing evaluations for Composer Stager's dev dependencies. https://github.com/php-tuf/composer-stager/issues/78 is the issue for that (currently that issue only lists one dev dependency but the others will be added to it soon).

Added link to https://github.com/php-tuf/composer-stager/issues/60.

Adding a link to the summary to the Composer Stager issue to define a security policy (https://github.com/php-tuf/composer-stager/issues/79).

I added a "Code overview for reviewers" section to the issue summary to help orient reviewers.

👏 That looks excellent, @effulgentsia!

Discussing this at DrupalCamp NJ. I think it would be worth shipping a composer.phar with Drupal core so that there is a known and valid version of the composer executable always available.

Advantages:
You can always be using a version of composer against which all the automated/manual tests have run and passed.
There is no dependency on the hosting provider to have installed a usable version of composer

There might be other ways to handle a missing or invalid composer, but this is at least simple. since it's just a phar, it's not going to be the composer/composer package which is causing issues mentioned by Dave in 📌 [policy] Move composer/composer from a dev dependency to a production dependency Closed: won't fix

Also, there should be a way to run the autoupdates on the cli so that you can run as a system user that can write the files. Possibly we could update core/scripts/drupal to include some commands that actually bootstrap to do those things in a light-weight way?

https://github.com/php-tuf/composer-stager/releases/tag/v2.0.0-alpha1 was tagged.

See https://github.com/php-tuf/composer-stager/milestone/1 for what remains to reach 2.0.0.

This patch adds php-tuf/composer-stager 2.0.0-alpha1 to Drupal core and was generated using

COMPOSER_ROOT_VERSION=10.1.x-dev composer require php-tuf/composer-stager 2.0.0-alpha1

Thanks for such a comprehensive summary! I hope to have time to review this “soon”. 🤞 meanwhile, I hope adding “and governance” to this title is a friendly amendment, because that’s the full scope here, no?

ComposerProjectTemplatesTest fails because 2.0.0-alpha1 does not meet the minimum stability of stable.

Not sure how we want to proceed here because https://www.drupal.org/about/core/policies/core-change-policies/experime... → does not document either:

• how new dependencies that are necessary only for experimental modules are or aren't removed from packaged releases of Drupal core
• whether such dependencies must meet core's minimum stability guidelines too

… but given that this is an alpha-experimental module, a dependency on an alpha release of a package that as @xjm said in #6 we are actually maintaining … seems reasonable?

The last submitted patch, 20: 3331078-20.patch, failed testing. View results →

The last submitted patch, 22: 3331078-22.patch, failed testing. View results →

#20 had 3 failures, #22 has 2. But the 2 failures already fail locally on a fresh Drupal core clone … 🤔😬

Fortunately the error message is clearer on DrupalCI than on my local — they fail like this:

Your requirements could not be resolved to an installable set of packages.

  Problem 1
    - Root composer.json requires drupal/core-recommended ^10.1 -> satisfiable by drupal/core-recommended[10.1.0].
    - drupal/core-recommended 10.1.0 requires php-tuf/composer-stager ~v2.0.0-alpha1 -> found php-tuf/composer-stager[v2.0.0-alpha1] but it does not match your minimum-stability.

That's pretty clear. AFAICT that means this is hard-blocked on a 2.0.0 tag of php-tuf/composer-stager?

Attempting a work-around: use a stability flag to bypass core's requirement: https://getcomposer.org/doc/04-schema.md#package-links.

This blocks ✨ Add Alpha level Experimental Package Manager module Needs review , which in turn blocks Automatic Updates ( ✨ Add Alpha level Experimental Automatic Updates module Needs work ) and Project Browser (no core issue yet — roadmap: 🌱 Roadmap to experimental Project Browser in Drupal Core Active ).

Looks like #26 will still fail.

Still, it doesn't even really matter: @tedbow pointed out that he actually thinks the very goal is to have an alpha release of php-tuf/composer-stager:

1. at least for now (to avoid the need for BC breaks based on core committer reviews)
2. arguably even in core, to allow it to evolve further while Package Manager/Automatic Updates are alpha-experimental anyway

What do core framework managers & release managers think? 😊

The last submitted patch, 26: 3331078-26.patch, failed testing. View results →

I think that the patch for this issue can/should change ComposerProjectTemplatesTest::MINIMUM_STABILITY from "stable" to "alpha". I think it's okay to do that, because after we branch 10.2.x, we can remove this dependency from 10.1.x and then raise ComposerProjectTemplatesTest::MINIMUM_STABILITY back to what we want to prior to tagging any 10.1 pre-releases.

@pwolanin, re #18 and #19, that's really good feedback, but I think #18 is a topic for ✨ Add Alpha level Experimental Package Manager module Needs review and #19 is a topic for ✨ Add Alpha level Experimental Automatic Updates module Needs work . Do you agree, or do you see those as something in scope for Composer Stager?

The sequence of issues is:

• Composer Stager: this issue. A PHP library agnostic of anything Drupal-specific, so that other CMSes could use it too if they want to.
• Package Manager: ✨ Add Alpha level Experimental Package Manager module Needs review . Drupal module that depends on Composer Stager but that is independent of whether the use-case is installing (Project Browser) or updating (Automatic Updates).
• Automatic Updates: ✨ Add Alpha level Experimental Automatic Updates module Needs work . Depends on Package Manager.

I'll try to post some updates here as I make progress toward a stable v2.0.0 release of Composer Stager. I knocked two issues off the milestone checklist today. Of the two remaining, one should be pretty simple. Eliminating the dependency on Symfony Filesystem could potentially take some doing (but I can't say until I get started). I do plan to add an issue to the milestone for some refactoring that I want to do while I can still make BC-breaking changes. I don't want that opportunity to get away, but I'm trying not to go crazy.

For #33 — we indeed need to document where we're at and what remains on this issue to make it actually realistically reviewable by framework managers & release managers.

Tell me when you push this dependency onto stable so I can update. Thanks.

Update from my end: I'm continuing to work on the Composer Stager v2.0.0 milestone, most notably Add support for string translation for names, descriptions, and messages. Things are moving along nicely, but there's a non-trivial amount of work left.

Update: there's ongoing requirements-and-design discussion on Add support for string translation for names, descriptions, and messages. The scope has increased somewhat as a result, and there's still quite a bit of work left. But it's still moving along at a good pace without any surprises.

Update: Add support for string translation for names, descriptions, and messages is finished. There are some follow-ups and of course the other issues assigned to the v2.0.0 milestone.

Composer Stager 2.0.0-beta1 has been released. I updated the issue summary to be up to date with that.

I believe Composer Stager now implements everything that is needed for an Automatic Updates in Drupal core MVP and is therefore fully ready for reviews.

Updated patch to pull in the 2.0.0-beta1 version.

Fixed broken links in the issue summary that were due to refactoring of class names from 1.x to 2.x.

1. +++ b/composer.lock
   @@ -2402,6 +2485,69 @@
   +        {
   +            "name": "symfony/filesystem",
   +            "version": "v6.3.0",
   +            "source": {
   +                "type": "git",
   +                "url": "https://github.com/symfony/filesystem.git",
   +                "reference": "97b698e1d77d356304def77a8d0cd73090b359ea"
   
   @@ -8835,69 +8981,6 @@
   -        {
   -            "name": "symfony/filesystem",
   -            "version": "v6.3.0",
   -            "source": {
   -                "type": "git",
   -                "url": "https://github.com/symfony/filesystem.git",
   -                "reference": "97b698e1d77d356304def77a8d0cd73090b359ea"
   

   So weird that composer just moved this exact same dependency around in composer.lock! 🙃🤷‍♂️

   You can see that even the hash is identical.

   It appears that different composer versions just have different sorting logic?

   EDIT: ah, now I see — it is being moved from the pinned dev dependencies to an indirect non-dev dependency. That explains it 👍

2. +++ b/core/tests/Drupal/BuildTests/Composer/Template/ComposerProjectTemplatesTest.php
   @@ -30,7 +30,7 @@ class ComposerProjectTemplatesTest extends ComposerBuildTestBase {
   -  protected const MINIMUM_STABILITY = 'stable';
   +  protected const MINIMUM_STABILITY = 'beta';
   

   Is this really acceptable? 🤔

   We usually only do this when updating Symfony to the release we know we'll ship the next Drupal core minor with. For example, 📌 Update to Symfony 6.3 Fixed changed this to 'beta' too, then 📌 Update to Symfony 6.3 RC1 Fixed changed it to 'RC' and 📌 Update to Symfony 6.3 Fixed restored it to 'stable'.

   I guess we're saying … that it'll be okay to set this temporarily, because we'll pin it to a stable release of php-tuf/composer-stager prior to tagging Drupal 10.2?

3. +++ b/core/tests/Drupal/BuildTests/Composer/Template/ComposerProjectTemplatesTest.php
   @@ -30,7 +30,7 @@ class ComposerProjectTemplatesTest extends ComposerBuildTestBase {
   -  protected const MINIMUM_STABILITY = 'stable';
   +  protected const MINIMUM_STABILITY = 'beta';
   

   🤔 Arguably, this is wrong. Or rather, the assertions associated with this are wrong:

         $project_stability = VersionParser::parseStability($project['version']);
   …
         $this->assertGreaterThanOrEqual($minimum_stability_order_index, $project_stability_order_index, sprintf(
           "Dependency %s with stability %s does not meet minimum stability %s.",
           $project['name'],
           $project_stability,
           static::MINIMUM_STABILITY,
         ));
       }
   
       // At least one project should be at the minimum stability.
       $this->assertContains(static::MINIMUM_STABILITY, $project_stabilities);
   

   This is wrong because it means that this test is testing something that violates Composer's stability flags:

   They take the form "constraint@stability flag". These allow you to further restrict or expand the stability of a package beyond the scope of the minimum-stability setting. You can apply them to a constraint, or apply them to an empty constraint if you want to allow unstable packages of a dependency for example.

   Whenever a stability flag is encountered, it should be special-cased.

   OTOH this comment in the test:

       // Ensure that static::MINIMUM_STABILITY is the same as the least stable
       // dependency.
       // - We can't set it stricter than our least stable dependency.
       // - We don't want to set it looser than we need to, because we don't want
       //   to in the future accidentally commit a dependency that regresses our
       //   actual stability requirement without us explicitly changing this
       //   constant.
   

   explains WHY we are doing it this specific way.

   So … 👍

Is this really acceptable?

We have test coverage that when we tag an rc (I think) will fail if the minimum stability doesn't match - so it becomes rc-blocking to sort out. As you point out we usually only do it with Symfony because their release timelines are pretty reliable, but I think it's OK to apply the same pattern here.

The last submitted patch, 42: 3331078-42.patch, failed testing. View results →

Known, unrelated random fail:

1) Drupal\Tests\Component\Utility\RandomTest::testRandomMachineNamesUniqueness
RuntimeException: Unable to generate a unique random machine name

The last submitted patch, 42: 3331078-42.patch, failed testing. View results →

#49 looks like a random failure, but this rerolls for composer.lock changes since #42, and also updates the composer-stager requirement to beta3.

Back to RTBC, optimistically assuming it'll pass tests. Note that the RTBC is for the patch itself, but putting composer-stager under core governance still requires framework and release manager review of composer-stager itself, per this issue's tags.

As you point out we usually only do it with Symfony because their release timelines are pretty reliable, but I think it's OK to apply the same pattern here.

Yeah, in this case, part of putting composer-stager into core governance, is then its release timeline is under our control. Also, there are currently no known issues blocking a stable 2.0.0 release of composer-stager. Only thing keeping it at beta right now is that it hasn't been reviewed by any Drupal core committers other than me.

The last submitted patch, 50: 3331078-50.patch, failed testing. View results →

I think #51 is a random failure. Queued a re-test and optimistically re-RTBC'ing.

The last submitted patch, 50: 3331078-50.patch, failed testing. View results →

Two different randoms.

My main concern here is that the committer team routinely struggles to keep the RTBC queue under 150 issues at a time, and this adds a separate queue on a separate system to manage, with a code base that is entirely new to most committers.

So what I would propose is we add core committers to the github repo, but in addition to the existing maintainers, not in addition. We can figure out exactly what responsibilities are as time goes on, but an immediate switch from one set of maintainers to another doesn't seem like a good idea.

Right, and core maintainers will still control which versions are pinned in core-recommended at least. I'd suggest this:

• Current maintainers are 100% authorized to commit trivial fixes (like dev dependency updates).
• We gradually transition responsibility for substantive fixes onto core release and backend framework managers.
• Release manager signoff is required for tagging a new release of the GitHub-hosted packages.

+++ b/core/tests/Drupal/BuildTests/Composer/Template/ComposerProjectTemplatesTest.php
@@ -30,7 +30,7 @@ class ComposerProjectTemplatesTest extends ComposerBuildTestBase {
-  protected const MINIMUM_STABILITY = 'stable';
+  protected const MINIMUM_STABILITY = 'beta';

This is hopefully only a temporary change, like we make between alpha and RC for core for Symfony? If so we need a followup to switch it back before 10.2.0-rc1.

Also possibly we should branch 10.2.x (which we are planning to do ASAP) before we actually commit this, since needing to revert lockfile changes before RC would be messy.

@catch and I both would really like to see some of the excess dev dependencies eliminated. If possible, let's please focus on dev deps that core already uses. E.g., seems what grumphp is doing could mostly be accomplished with phpcs and phpstan etc.? I saw https://github.com/php-tuf/composer-stager/issues/78 but in a way this is almost asking release managers to do too much work. Dev dependencies core already has for dev or prod are fine and don't even need their own evaluations, but I'd really like to see fewer additional dependencies proposed, and a strong case made as to why they are essential to the maintenance of the package.

Also, let's get a followup for #57, and an issue in the composer-stager queue to add a GOVERNANCE.md where we can start documenting the above stuff?

@xjm: Evaluate dev dependencies · Issue #78 · php-tuf/composer-stager has been updated with issues for each dev dependencies, including justifications for each. I've knocked off a half dozen I felt OK about eliminating without debate.

And I have created Create a governance policy · Issue #300 · php-tuf/composer-stager

I've added:

Existing composer maintainers would maintain commit access to the repository, but should co-ordinate any new releases or major changes with core committers.

to the issue summary since I think that represents the current plan.

Existing maintainers would maintain commit access to the repository, but should co-ordinate any new releases or major changes with core committers.

This is reasonable place to start. And there will certainly be an evolution of the responsibilities of the roles and the points in #56 indicate a direction on that.

Tagging for issue summary update now that https://github.com/php-tuf/composer-stager/releases/tag/v2.0.0-beta4 was released, which removed the PHP filesyncer per 🌱 [policy] Require rsync for automatic updates in Drupal core and punt other syncers to contrib RTBC .

https://github.com/php-tuf/composer-stager/issues/350 just got completed, making Composer Stager compatible with both Symfony 6 and 7, so that it's compatible with Drupal 10 and 11. So next steps for this issue are:

• Release Composer Stager 2.0.0-beta5, so the above fix is in a tagged release. Note that Composer Stager 2.0 is only in beta status because it hasn't been sufficiently reviewed yet. But as far as we know, it's complete, so we can release RC or stable at any time.
• Open an MR for this issue that adds Composer Stager ^2.0.0-beta5 to Drupal core's dependencies. See #50 for the latest patch that did this with an earlier version, but we don't do patches anymore.
• Update the issue summary of this issue for accuracy (e.g., remove the info about the PHP file syncer and check the rest of the issue summary for whether it accurately reflects the current state of Composer Stager).
• For anyone following this issue, and especially core committers, please review the Composer Stager codebase and open GitHub issues for any problems/concerns that you find.

Assigning to myself as I take a first pass at the merge request

This went in as part of ✨ Add Alpha level Experimental Package Manager module Needs review

We still need to do the governance steps here. I'm not sure what's left but there's no other issue to track that in.