Reliably support cweagans/composer-patches in Package Manager & Automatic Updates: validate stage

Paired with @kunal.sachdev on a way forward to do #17 — realized that we cannot test that in a Kernel test, so helped him get the outline in place for testing that in a Build test! 👍 Good luck, @kunal.sachdev 😄

The summary currently still has "Possible solutions:" so it should updated describing what the current merge request is meant to do. Also we specify which merge request is the chosen fix, and close the other if that is appropriate

@tedbow identified an extra thing we need to protect against. I posted a proposal on how to do that.

I identified a few clarity nits … but also just spotted one more thing we need to test: https://github.com/cweagans/composer-patches#allowing-patches-to-be-appl... — we should expand our test so that the drupal/alpha project specifies a drupal/core patch, which will be installed if and only if config.extra.enable-patching === true.

GREAT job on getting this test to work! 🤩 It looks excellent! With just these few additions (from @tedbow and the one I just asked for), we'll be able to be highly confident in our support for https://github.com/cweagans/composer-patches 😊

So, for the build test failure
Failed to copy "/tmp/.package_manager1138399f-efbb-4de5-9615-4d8919835d23/cYjbXr4SVceHzcM14tPFfA2Jnk3JCLZvpjYgEVtk-Xo/vendor/drupal/coder/.git/objects/pack/pack-4a9af0fe0580e9f82296d111933506654039d16a.idx" to "/tmp/build_workspace_1f164ac4a984a2ad22cbee436c59452bfdpZqI/project/vendor/drupal/coder/.git/objects/pack/pack-4a9af0fe0580e9f82296d111933506654039d16a.idx". in Drupal\package_manager\Stage->apply() (line 495 of modules/contrib/automatic_updates/package_manager/src/Stage.php) which is currently not there as we have a temp fix in GitExcluder , I tried to update drupal/core with Automatic Updates in a real drupal project:

1. Created drupal project using composer create-project drupal/recommended-project:10.0.0 ten_zero --no-install --no-interaction
2. Ran following commands :
   

   composer config allow-plugins.composer/installers true                                          
   composer config allow-plugins.drupal/core-project-message true
   composer config allow-plugins.drupal/core-composer-scaffold true

3. Ran composer require drupal/core-dev as drupal/core-dev has a dependency on drupal/coder
4. Confirmed presence of the .git folder in vendor/drupal/coder
5. Updated drupal/core from 10.0.0 to 10.0.2 without any problem with Automatic Updates

This points out that we may not have this problem in real time.

Discussed in detail with @tedbow & @kunal.sachdev.

Conclusions:

1. doing a build test for composer-patches is something that would be good to have, but it's post-MVP
2. instead, we do want for MVP a build test that proves there is a reasonable UX whenever composer has a hard failure — no matter whether that is due to a patch failing to apply from composer-patches or whether that is from a networking error, wrong requirement, whatever → this needs a separate issue
3. in fact, @tedbow spotted one more thing we should validate: composer-patches should never be installed nor removed — it should stay exactly as it is: so if it's absent in the active, it should be in stage, and vice versa

Executing #27:

1. doing a build test for composer-patches is something that would be good to have, but it's post-MVP
2. instead, we do want for MVP a build test that proves there is a reasonable UX whenever composer has a hard failure — no matter whether that is due to a patch failing to apply from composer-patches or whether that is from a networking error, wrong requirement, whatever → this needs a separate issue

Done:

1. 📌 Add functional test that proves there is reasonable UX whenever a stage event subscriber has an exception Fixed
2. 📌 Add build test to test cweagans/composer-patches end-to-end Closed: won't fix

The third point is up to @kunal.sachdev 😊

Extracted the build test work Kunal did to 📌 Add build test to test cweagans/composer-patches end-to-end Closed: won't fix 's MR, while preserving Kunal's commits 👍

Just pushed a commit that now reverts those changes from this MR.

Note that this still uses ::getStageComposer() and ::getActiveComposer(), but that was already the case in HEAD, so let's not delay it over that.

This merge request is looking great, but is still in need of a range of finishing touches before this is RTBC. I'm confident you can get that done by the next time we meet, @kunal.sachdev! 😄

Excellent work here, @kunal.sachdev! 👏

Docs: ✅
Tests: ✅

Issue summary update: just did that.

Found a few small things that aren't blocking, and then one thing that looks like a genuine mistake that I'm surprised is not breaking tests.

phenaproxima → closed merge request !177

Test failing on 7.4 ,core 9.5

RTBC again. I haven't reviewed this more, just fixed the php 7.4 error(pretty sure tests will pass now)

Leaving assigned to @phenaproxima

Sorry I know this issue has been long slog. See MR comments

I created 📌 Create StageBase::stageDirectoryExists() for improved DX to see if stage directory exists. Fixed to help with this https://git.drupalcode.org/project/automatic_updates/-/merge_requests/66...

Looks great!

🎉 Thanks @kunal.sachdev, @Wim Leers, and @phenaproxima

And @cweagans for making a plugin that is so useful to so many people in Drupal that we can't just ignore it😜

☝️ gif reserved for critical, core-mvp issues

YAY!

Unpostponed 📌 Add build test to test cweagans/composer-patches end-to-end Closed: won't fix .

A regression was introduced shortly before commit, spotted by @kunal.sachdev: 🐛 Fix language nit in package_manager_help() Fixed .

I saw this recent comment from the maintainer of composer-patches: https://github.com/cweagans/composer-patches/issues/411#issuecomment-142...

main doesn't resolve patches from dependencies anymore.

I take that to mean that a future release of cweagans/composer-patches simply won't allow patches from this module to apply to a site: the patches *must* be defined in the site's composer.json file.

Testing on dev-main of Composer Patches (#f88fd4) seems to confirm this. Can someone corroborate this? Does this mean this approach for automatic_updates simply won't work?

I can corroborate that :) probably don’t take action on that right away though. Currently traveling, but I’ve gotten a lot of feedback on that and I’m thinking it through a bit more.

@mark_fullmer Thanks for bringing that to our attention! But …

1. We need to ensure the existing version of composer-patches also works reliably!
2. In 📌 Tighten ComposerPluginsValidator: support only specified version constraint Fixed , we're tightening support for composer plugins further: only the known to be supported version range will be allowed. That means we'll be forced to revisit the validation strategy when a new major version of composer-patches is released … and this very upstream change proves that that is a valuable and essential requirement 😊👍