Package manager/ Automatic Updates should disallow composer patches by default

I agree with not allowing composer patches by default, for the reasons explained in the issue summary.

Tagging this as a package manager/automatic updates beta blocker, we could potentially move it to a stable blocker later but should at least discuss more before beta.

This validation is done in ComposerPatchesValidator, currently it checks if composer patches is a root composer dependency, and issues a validation error if it's a dependency anywhere else and not root.

Probably what we'd need here is a settings.php flag to enable that behaviour, defaulting to FALSE, and then only allow composer patches as a root requirement when it's TRUE. This would force people to make an explicit choice to use package_manager with composer patches, rather than potentially doing it by accident/without thinking about the consequences.

My main concern here is that someone installing Drupal CMS (and eventually site templates) via the UI, never gets into a situation where they have to manually edit JSON to update their site. Requiring a settings.php flag would mean that only pre-release versions of site templates could ship with patches applied, which is fine for testing, but they would need to be committed upstream before a stable release and the composer dependency removed in order to work with automatic updates/project browser.

I'm not sure how this interacts with 📌 Dependencies should be 'unpacked' to the root composer.json and merged/resolved Active - e.g. if recipes specify dependencies on composer patches, and those then get unpacked to the root composer.json, will that trigger the validation or not? From reading it, it looks like it prevents that situation from happening. I think if the answer to this is that it's impossible for that to happen and it will produce a validation error, we could move this to a stable rather than beta blocker.

Adjusting credit, since the MR is a slimmed-down version of the patch in #7.

Seeing as how ComposerPatchesValidatorTest failed until I specifically enabled the setting in that test, I'd hazard to guess that there's probably no additional test coverage needed here. Crediting myself for the MR.

See MR comment

I like that a lot better. We don't need to worry about versions, we support both majors in ComposerPatchesValidator.

That looks simpler, but does it need new test coverage then?

It would duplicate other test coverage. Now the patcher is just treated like any other "untrusted" plugin, which will raise a validation error unless explicitly allowed in configuration.

That was my only question, this is much simpler than the new setting.

Committed to 11.x - thanks all

larowlan → closed merge request !11903

Take 2, committed the right thing this time. Been having grief with gitlab signing me out and looks like the signin button sent me to a previous MR I'd viewed. Still my fault for not doing git show before I pushed. Resolved now