Replace cweagans/composer-patches with vaimo/composer-patches

[boobaa]

The cweagans/composer-patches project has a list of issues which are basically around one thing: patches that add new files are NOT handled properly (cweagans/composer-patches#43, cweagans/composer-patches#101 and cweagans/composer-patches#178 to name a few).

I'm using this core patch as an example.

While using cweagans/composer-patches, I'm having some artifacts like $gitroot/web/core/b/core/modules/taxonomy/taxonomy.install and $gitroot/web/core/core/modules/taxonomy/taxonomy.install, which are obviously wrong (correct one would be $gitroot/web/core/modules/taxonomy/taxonomy.install, tho I must admit that this file gets also created). Searching for the root reason for this reveals that cweagans/composer-patches tries to invent the -p level for the patch command by doing some try&error, with a hardcoded order of -p1, -p0, -p2 and -p4 (in this hardcoded, non-configurable order). With this approach, the patches that are creating new files will just apply cleanly in just about either case. Seemingly cweagans/composer-patches managed to pick -p2, but doesn't clear up the mess it created. The author/maintainer @cweagans says the upcoming 2.x release will/does have a configuration option for each patch that would allow specifying the -p level, but we're just not there yet.

There are some known workarounds to remedy the situation: eg. acquia/blt#2809 simply runs an rm -rf core/core core/b during post-autoload-dump.

I did some research on alternatives to cweagans/composer-patches. First I came across netresearch/composer-patches-plugin which seems really-really flexible (eg. it would even allow patching a required package from another required package), but it turns out that even when I'm providing the correct -p2 as the args for my patch and it says it's applied, actually it is not. That's a clear showstopper for me.

Then I came across vaimo/composer-patches. It also allows providing the correct -p level manually and does apply the patch when it says so. Its only drawback I'm currently aware of is the inability of patching a package from another (like asking for a core patch within a particular contrib/custom module's composer.json file; see vaimo/composer-patches#13 for more details). I'm also tempted to create a PR for it that would copy the lacking "enable-patching": true functionality from cweagans/composer-patches.

As I side note, I think it's also worth mentioning that vaimo/composer-patches seems to be forked from cweagans/composer-patches back in 2016, and seemingly both have some features since then which the other one lacks.

I'm not creating a PR just yet as I'm interested in your opinion first.

[cweagans]

FWIW, I'm pretty sure cweagans/composer-patches#189 resolves the issue you're having, and that's in 1.x.

[boobaa]

Thanks for the quick answer, will have a look at it and report back.

[boobaa]

Did some more homework.

For managing a Drupal 8 project's codebase, we're having these requirements:

1. Both core (-p2) and contrib (-p1) patches should be handled properly.
2. There shouldn't be any "artifacts" (eg. like $gitroot/web/core/b/core/modules/taxonomy/taxonomy.install for this patch) after Composer finishes its job.
3. There should be a way to patch core from a contrib and/or a custom module. (We're reusing some custom modules across projects that need core patches.)

Here are my findings.

• Latest stable cweagans/composer-patches fails on the first two requirements (okay, they're the effect of the same problem).
• Latest dev of it (or at least one with the 2ec4f00 commit merged in 1.x backport: Allows patch-level to be specified per-project. cweagans/composer-patches#189) solves the first two requirements.
• Latest stable vaimo/composer-patches doesn't have a problem with the first two requirements.
• Latest dev cweagans/composer-patches support explicit providing of the -p level, but only per-package; latest stable vaimo/composer-patches support explicit providing the -p level even per-patch. For core patches,
• To meet the third requirement, wikimedia/composer-merge-plugin is needed in both cases (with extra/merge-plugin/merge-extra-deep: true); for vaimo/composer-patches an extra/enable-patching: true is needed in either the root composer.json or the custom module's one; this is not needed for cweagans/composer-patches (TBH I must admit IDK why).
• Also did some benchmarking on a D8 project with two custom modules (one needing a core patch), two other core patches, a whole bunch of contrib modules with three of them needing patches, and some PHP and JS libs. The same codebase was built by cweagans/composer-patches in 10 minutes and by vaimo/composer-patches in 6 minutes. (Basically cweagans/composer-patches applies the two patches from the root composer.json during installation of core, then after generating the autoload files it removes core and readds it with all the three patches, which wastes quite a bit of time.)

Summing it up, there are only two reasons for replacing cweagans/composer-patches with vaimo/composer-patches (after the 2ec4f00 commit in the former):

• vaimo/composer-patches allows explicit providing of the -p level per-patch (which might be a good thing if one needs to apply a -p1 core patch, if there's any).
• vaimo/composer-patches might run faster (by not wasting time on patching core twice in some not-so-corner cases).
• There are some other benefits for vaimo/composer-patches (like it's generally more flexible and provides some composer patch commands), but I find those marginal.
• OTOH, as the extra/patches syntax is different, this change might cause some minor headaches for folks already using drupal-composer/drupal-project.

[cweagans]

this is not needed for cweagans/composer-patches (TBH I must admit IDK why).

Grabbing patches from dependencies' extra section is baked into composer-patches: https://github.com/cweagans/composer-patches/blob/1.x/src/Patches.php#L105-L119

Basically cweagans/composer-patches applies the two patches from the root composer.json during installation of core, then after generating the autoload files it removes core and readds it with all the three patches, which wastes quite a bit of time

This is a bug. I ran across this last night, and it's definitely not the right behavior.

after the 2ec4f00 commit in the former

If you need a release, I'm happy to review the queue tonight and roll a new one.

vaimo/composer-patches allows explicit providing of the -p level per-patch (which might be a good thing if one needs to apply a -p1 core patch, if there's any).

I just committed a change to master last night that adds a more verbose patch format, which would allow you to optionally provide a patch depth and sha1 hash to validate the patch against. There are other things this could be used for as well, but those are the two immediate needs.

There are some other benefits for vaimo/composer-patches (like it's generally more flexible and provides some composer patch commands), but I find those marginal.

This is also coming. Commands to manage patches, user-extensible patch resolvers, etc. I haven't had much traction on getting people to work on composer-patches 2.0 with me, so it really just comes down to the amount of time I can spend on it. If y'all are interested in collaborating, I'd be happy to spec out what I'm looking at for 2.0.

[rodrigoaguilera]

That sounds very good. Thank you @cweagans for the effort you put in the plugin.

I think we should keep the current plugin

[cweagans]

Just to be clear: I'm not advocating for one plugin or another (of course, I hope mine is useful enough to keep). I'm just providing what information I can to make the decision-making process easier.

[webflo]

@cweagans Thanks for the hard work.

I'll keep cweagans/composer-patches as the default.