Standarize on patch workflow/plugins

[webflo]

We have a few different composer plugins to apply patches files after a package is installed.

• netresearch/composer-patches-plugin
• jpstacey/composer-patcher
• webflo/composer-patcher (fork of jpstacey/composer-patcher)

I forked jpstaceys repos because i think its very convenient to manage all patches in the root composer.json file. And because of our own packagist we dont have the package definition in our composer.json. This means, we need to store this information somewhere else.

// cc @kasperg, @derhasi, @greg-1-anderson, @frega, @jpstacey: Whats your opinion?

[webflo]

Or is anyone aware of other solutions?

[greg-1-anderson]

I have to admit that I don't use the patch workflow much. If I have to patch a contrib module, I take it out of the composer.json / makefile, and check the whole thing in to git (sites/all/modules/contrib, sites/all/modules/custom, sites/all/modules/modified, with the last two under revision control). I considered a patch workflow some time ago with kitten-o-matic, but decided against it. If I patched more, I'd probably adopt a patch workflow.

With that disclaimer in mind, here are my thoughts. There seems to be two lines of reasoning here:

1. We'd better keep one patch per release of the module we're patching, because patches might not apply when things change. (netresearch/composer-patches-plugin).
2. Let's try to apply our patches to modules, regardless of version, and see if it works. (webflo/composer-patcher).

I tend to think that I would favor #2. In either event, nothing changes until composer update time. If something breaks, then you're going to have to re-roll the patch regardless. It's extra work to tie the version of the module to the patch, and I don't really see the value of keeping that information around (the patch for the older version of the module, once you've updated to the newer version of the module) in the composer.json file, as you can always roll back to it in the vcs. So, keep the composer.json lighter.

There may be other aspects of these different solutions that I am not considering, though, as I have not evaluated them deeply.

[kasperg]

Nice writeup @greg-1-anderson . 👍 for option 2 as well from me.

composer-patches-plugin allows you to have a repository of tried and true patches that work across versions which you can include in all your projects. None the less that does not match how I use patches.

[derhasi]

I wasn't aware of jpstacey's and webflo's solutions. As mentioned in a small discussion on gdo I'm also for option 2, maybe with adding an optional version constraint.

The generell problem with a patches plugin is, that you have to know if a patch was already applied or not. I'm currently not sure if there is a way to store that information in the composer.lock or the installed.json. Due to that problem netresearch/composer-patches-plugin seems to have gone the way with a separate package. Otherwise we could use a separate patches.lock or a separate command to reinstall a package for applying new patches. (composer patch ...).

[jpstacey]

I'm more than happy to deprecate mine in favour of the other too. It was a POC at a time when I couldn't find other resources out there. In fact, as composer's own APIs for event handling seemed to have changed last time I checked (some methods no longer available on Event), and as I don't have a client to sponsor updating it, I've already done so in favour of netresearch: but if the preference goes to webflo, let me know and I'll change the relevant notices.

[yched]

I am trying webflo/composer-patcher, and am still having a hard time getting any patch applied - still investigating.

However, I would think that running on post-package-install means patches only get applied if the corresponding package/module is downloaded.
So, if I add a patch for a package that is already in my composer.json, without any version bump, composer update won't do anything with on that package (no version change), and thus won't run its post-package-install, and thus won't apply the new patch ?

In a typical workflow, adding a new patch for a module should mean the module gets re-downloaded and all its patches get re-applied on the fresh download ?

Didn't check yet how netresearch/composer-patches-plugin handles that.

[webflo]

@yched Your analysis is corrects. Thats indeed a problem because there is no composer event for already installed projects. Maybe we should add an additional command to force the patch apply on already existing modules?

I think the netresearch/composer-patches-plugin handles this use-case because it leverages the composer dependency resolution. I just started webflo/composer-patcher because it think the config for netresearch/composer-patches-plugin is to verbose.

[cweagans]

I think it wouldn't be too hard to extend cweagans/composer-patches to do this. I'll probably work on this today because we need it at work.

[yched]

@cweagans oh, awesome :-) I wasn't aware of that other "composer patch" option.

Here are a couple thoughts after having tried netresearch/composer-patches-plugin:

As discussed above, its behavior of reapplying patches if the patch definitions changes, instead of when the patched module changes, is a must-have IMO. Being able to add a patch for a module that is already present feels pretty important ?

It works that way because it defines a set of patches as a package definition, so changes in the patch set can be caught (well, provided you manually bump the package "version" on each change, which is not too great, not sure there's another way)

On the minus side :

1. It does seem its code could be simpler :)
2. Having to specify patches by exact version of the patched package is tedious and not really needed IMO. Just try to apply the patches, and abort if they don't apply.
3. It only knows "patch -p1", no "git apply", meaning no support for binary content (e.g patches adding / changing an icon)
4. It fails on core patches with the structure defined in drupal-composer/drupal-project, because it tries to apply them from the web/core directory (since it's the installer-path for the drupal/core package), while they are typically rolled to be applied one directory up.
5. When a change in the patch set is detected, it tries to unapply (revert) all the patches from the previous version of the patch set, so that it can apply the new patch set. That feels very brittle, and breaks badly when one of your patches doesn't apply to begin with : the next attempt with a new patch will first try to revert the previous one, which fails since that one could never be applied in the first place. It's then very hard to get your way out of there :-/.
   Ideally, when a patch set is changed, we should start from scratch with a fresh download of the module to patch. Not sure how easily composer would let us do that though (an update in a package triggering a re-download of another package)

[yched]

Also, re-downloading the module when its associated patch set changes means you need separate "patch packages" per module, so that you don't re-download modules whose patch set hasn't changed.
Doable, but adds more verbosity :-/

[cweagans]

So I looked into this, and it's going to be kind of a pain in the ass. Would a composer reinstall command be a reasonable path forward? Currently, the painful part of the workflow is that you have to remove the installed package and re-run composer install. If you could composer reinstall drupal/core and get the updated patches, would that be an okay workflow?

I was thinking that composer reinstall --all could be a good thing to have too.

[yched]

IMO having the ability to force a re-download of a given package would be instrumental, yes.

If we have that, I think we can come up we various ways of handling a "track and apply a set of patches" workflow :

• either with a custom plugin that runs within "composer install/update" itself if that "force redownload" operation can be triggered in the plugin code (and possibly leveraging separate patches.json and/or patches.lock files for defining and tracking patch sets ?)
• or by an external script that runs outside of / after composer, and does "composer reinstall drupal/some_module" before re-applying patches

Having it all run through plugins within "composer install/update" seems nicer though ?