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

I think for 1&2 were we ignore its existence and don't update sites that use it; we are going to leave a lot of unhappy folks. especially for minor patches in contrib, 90% of the time the patches re-apply. Something that incorporates #3 into it seems like the best of all worlds. And maybe give a warning that the site is using it and it _could_ cause unexpected issues.

Some possible doom and gloom cases are when something goes from contrib into core and core updates and you need to remove the patch from contrib for the site to function. The patches apply still in contrib. Obviously they work in core too, it is part of core. But then in this scenario, the site stops functioning. Given we have sandboxing and hopefully a breaking change like that doesn't happen within a minor core release, we are probably OK. But worst case is a broken site. Best case, the site updates successfully 90-95% of the time.

I think for 1&2 were we ignore its existence and don't update sites that use it; we are going to leave a lot of unhappy folks

Agreed. This plugin is very widely used and IMHO we cannot just ignore it or deny service.

Check for composer-exit-on-patch-failure to make sure it will error out and warn or error if not set

To me, this is an appropriate solution for MVP.

check the patches applied in stage and make sure they are applied the same as active

I think this is the most robust way, but it could be implemented in contrib.

One other thing to consider: if you're invoking `composer install` on a shell (as opposed to just running the Composer code directly from Drupal or something), you can set the env var `COMPOSER_EXIT_ON_PATCH_FAILURE` to true to force the behavior you want.

Also: it might be worth verifying somehow that the majority of Drupal 8/9 sites actually use Composer Patches. Not sure how many people would be using Composer and _not_ be using Composer Patches, but maybe it's more common than expected and you can sidestep this work altogether. I think it would be pretty reasonable for an MVP to not support patching, build support in contrib, and pull it into core in a later version.

We don't have TUF signing for patches and won't for MVP. Therefore, I think that if a patch needs to be re-downloaded, we need to error out by default.

If core isn't patched, then we should still allow composer-patches to exist and be used for patching modules that aren't being auto-updated. If we think it's important to support the use-case of core being patched (I'm not convinced it is for MVP), then I think we either need to do that via a separate contrib module (e.g., automatic_updates_extras) that could allow non-TUF-verified downloads, or we'd need composer-patches to make a local copy of patches that get downloaded during non-automatic install/update and only use that local copy during automatic updates.

Discussed this with @tedbow, so let me summarize how we agreed to proceed. We will write a new validator, which will be included in the core Auto-Updates module, to implement a restrictive policy about patching. It will listen to ReadinessCheckEvent and PreApplyEvent. This is the logic it will use.

During ReadinessCheckEvent:

- Is cweagans/composer-patches in the active directory? If not, bail out; there's nothing to validate.
- Are we explicitly intending to update drupal/core? If so, is it currently patched in the active directory? If it is, flag an error; we don't support patching core right now.
- Is anything else (vendor dependency, contrib module or theme) patched? If so, flag a warning that we don't support patching, and if any of these things get updated in the stage directory, it will stop the update.

During PreApplyEvent:

- Is cweagans/composer-patches in the stage directory? If not, bail out; there's nothing to validate.
- Has drupal/core been patched in the stage directory? If it is, flag an error; we don't support patching core right now.
- Did anything else (vendor dependency, contrib module or theme) that is patched in the active directory, get updated in the stage directory? If so, flag an error, because we don't support updating patched things.

So, what I think this adds up to is two things:

- We won't let you patch core.
- We won't update anything else you've patched, and will stop the entire process if you try.

@tedbow, can you confirm this understanding?

Opened #3293381: If cweagans/composer-patches is installed, require composer-exit-on-patch-failure.

I don't see how we can not have an answer for this.

I see that #3293381: If cweagans/composer-patches is installed, require composer-exit-on-patch-failure addressed some portion of it at least. But we absolutely need clarity on this. It could be in the form of documentation, not code. But then I think requiring test coverage that goes further than #3293381's package_manager/tests/src/Kernel/ComposerPatchesValidatorTest.php is critical: we need to verify through tests not only that we complain loudly if composer-exit-on-patch-failure is not set to true; we also need to verify that we can indeed successfully update with patches applied! For example … a simple core patch.

While working on #3331168: Limit trusted Composer plugins to a known list, allow user to add more, I also noticed that \Drupal\package_manager\Validator\ComposerPatchesValidator currently has a critical oversight: it only checks the active composer, not the stage one!

That means if some module has a dependency on cweagans/composer-patches, it could cause it to get installed, and then the value of composer-exit-on-patch-failure would never be checked by ComposerPatchesValidator!

#17 made it sound like cweagans/composer-patches isn't reliable. That's not what I meant — I meant PM/AU's existing safeguards are inadequate and untested.

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 😄

@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 test_dir --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 the presence of .git directory 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. #3338666: Add functional test that proves there is reasonable UX whenever a stage event subscriber has an exception
2. #3338667: Add build test to test cweagans/composer-patches end-to-end

The third point is up to @kunal.sachdev 😊

Extracted the build test work Kunal did to #3338667: Add build test to test cweagans/composer-patches end-to-end'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 nitty things that aren't blocking, and then one thing that looks like a genuine mistake that I'm surprised is not breaking tests.

YAY!

Unpostponed #3338667: Add build test to test cweagans/composer-patches end-to-end.

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

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 #3340022: Tighten ComposerPluginsValidator: support only specified version constraint, 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 😊👍