"Unpack" patches from a recipes composer.json to the root composer.json

This is a duplicate of 📌 Dependencies should be 'unpacked' to the root composer.json and merged/resolved Active .

Never mind, not a duplicate.

I see the need for this feature when we unpack. for sure.

At this point use the method of an independent package to supply needed patches only, but without requiring them
to let any root composer.json use the patch storage. ( by the way, we borrowed that idea from distributions_recipes )

In some cases, a package for a recipe may use a rarely-used module - or for selected cases, like a commerce recipe for example we supply the patch in the recipe/module's composer.json file.

Unpack patches was removed from 📌 Dependencies should be 'unpacked' to the root composer.json and merged/resolved Active to handle it on this ticket.

This is the commit with the removed patch unpacked functionality.

Was it already working?

Anyway, I was wondering if this is a good idea in general..
If everything supplied by recipes should be optional, how do we (let a site builder) remove/override a patch once "unpacked"?
Wouldn't this need to be optional and come with a warning to the user applying the recipe.. or even on the site UI, eg. at /admin/reports/updates once a new release is available for patched modules?

(in this last regard, patchinfo → module might be a relevant example)

Providing patches by requiring your own custom module makes more sense to me, as the user will have some UI feedback (at least at /admin/modules) to find out possible conflicts, and we keep code separate from config. I'm not an expert though: how is #4 even working right now?

I think that having patches in one place is still a good idea for having and overview. So when all the patches are listed in root `composer.json` then it is easier to find them and to manage them when they need a re-roll or should be removed. This is partially fixed by composer patches version 2, as there is now the patches.lock file. But once the recipe is unpacked and the list is relocked, all the patches that were in the recipe will be vanished.

+1 on this, I have just wanted to open a similar ticket after I have seen New Recipe Unpack composer plugin → being published.

I am not at all convinced this is a good idea, and I think we should close this as a won't-fix.

Something we've learned from working on Drupal CMS is that dependencies really, really should not be patching sites. It's a violation of boundaries and can break sites with no recourse for people who don't have the strong technical skills necessary to recover from a broken patch, or combination of patches.

If dependencies ship patches:

• Several patches in combination may fail to apply.
• Patches are generally only applicable to a very specific version of a dependency -- sometimes even as specific as a range of commits.
• When a patch fails to apply, Composer dies with an exception. If you don't know how to manage patches (you really have to be a developer to do that), you're shit out of luck if that happens to you.
• And it can happen to you at any point, if a patched dependency releases a new version that conflicts with the patch.
• Unpacking exacerbates this, because it means the dependencies that are supplying patches cannot release a new version that would update the patch to something that would work. No: your only option is to know how to dissect composer.json and remove the broken patch -- which, in turn, might outright break your site, depending on what the patch was doing.
• The most responsible way to patch a dependency is to pin the dependency, which means it no longer gets updated. If the dependency suddenly has a security release, that is a security risk for your site.

There are other reasons, too...but these reasons alone are good reason to slam the lid shut on this. Shipping patches with dependencies is a release management nightmare.

It's true that all of these problems can be overcome by a person with strong technical skills. But by the very same token, patches should only be deployed for a specific site, for specific reasons, by a developer. Patching, by its nature, is an idiosyncratic thing that is useful in very particular circumstances. Those circumstances are almost always so specific to the quirks of a single site, in fact, that dependencies cannot anticipate them properly.

So I am a very strong -1 on this, and I'm fairly certain it would be nearly impossible to convince the core release managers to go along with this. Patching is for individual sites, not for dependencies.

Yes I agree with #10 overall.

There are a couple of questions maybe:

1. If we're not going to unpack patches, should we have an assert() or exception when we find them?

2. What does this mean for pre-release recipes that do need patches applied temporarily?

Recipe support still doesn't seem like the right way to handle #2 because a new pre-release or release version of the recipe can't un-apply the patch on existing sites, but can a recipe ship with a module that does this or something like that?

If we're not going to unpack patches, should we have an assert() or exception when we find them?

Yes, I think we need something like this. I agree that unpacking the patch isn’t a good idea but ignoring it also doesn’t work.

Separately, we probably won’t want to show recipes in Project Browser whose stable release uses any patches — since you can’t do anything about the exception from the UI.

we probably won’t want to show recipes in Project Browser whose stable release uses any patches

I don't know how we'll be able to filter these out.

But that said, it's actually safe to install a project which carries patches, because Package Manager doesn't by default support the cweagans/composer-patches plugin and will complain if it's enabled. You'd have to explicitly configure it to be allowed, and there is no UI for that.

So, although you can definitely screw up your site with patches, it's off by default and you can't enable it without some level of technical understanding.

I agree that, when unpacking, we should warn about patches, but I don't think we should throw an exception. If someone insists on patching -- which means they explicitly installed and enabled the cweagans/composer-patches plugin -- let them do it. But warning that patches won't be unpacked does make sense to me.

If everyone agrees at least adding the warning, could there be an option at least to list the patches that were found in the recipe/s that was/were unpacked? For technical person it will not be difficult to add those patches to the root composer, but I think the most difficult part (or the most time and effort consuming) is to collect those patches from the packages (also maybe nested recipes that make it even more difficult). If unpacker command will return the list of patches that were in the unpacked recipes back (maybe optionally with some command key) that would be great. Then a technical person will decide on their own which patches to add and all the conflicts that this can bring will be their responsibility. But the list of patches would be very valuable in my opinion.

+1 for that. Keeps the site safe, helps developers. Everybody wins!