Feedback on removal of dependency patch resolution

[RoSk0]

We have number of websites build on top of Sector distribution. Sector provides patches for know issues in the Drupal core and contributed modules.

We also maintain our own internal package that holds number of shared dependencies and patches for them.

So, removal of this functionality, would be quite devastating for us.

[craigra]

We have a similar use case. A single internal package that maintains a common set of composer dependencies that is shared by many projects.

Being able to lock dependency versions and apply specific patches in a shared package, provides consistency across a fleet of websites. Having to reproduce and maintain this list individually for every website would be painful.

The current state is very clean with a set of patches that are applied globally to all websites, and then any additional site specific patches in their own individual composer.json file.

[cweagans]

Noted! Thank you for the feedback.

My theory is that this functionality isn't something that the majority of composer patches users need, so I'd rather take it out of the core plugin. That said, the plugin is very extensible now. I've made it possible for other Composer plugins to extend this one via Capabilities. Essentially, any plugin can provide a method of resolving, downloading, or applying patches.

If there's enough interest, I may write another plugin that provides the dependency patch resolver. We'll see what happens this week :)

[Kingdutch]

Just wanted to add Open Social to the list of projects that rely on this behaviour.

[mxr576]

+1 Our projects also leverage this feature for a similar use case that was described in the conversation starter. Even if I do not like this approach, it is still better than forking projects and maintaining forks... BUT, how about replacing enable-patching with something that is more secure?

I understand why developers would like to deliver/bundle patches with libraries and if we need a similar feature my proposed alternative would be something like Composer 2.2.0 introduced with allow-plugins. Let's deprecate "enable-patching" and replace with for example an apply-patches-from list that contains all those trusted packages that can apply patches on a project. Example:

{
    "name": "mxr576/my-project",
    "type": "project",
    "minimum-stability": "stable",
    "extra": {
        "apply-patches-from": {
            "mxr576/trusted_lib1",
            "anyvendor/awesomepackage"
        }
    }
}

(Source: #354 (comment))

My theory is that this functionality isn't something that the majority of composer patches users need,

There are plenty public results already that proves this feature is widely used: https://github.com/search?l=&q=%22enable-patching%22+filename%3Acomposer.json&type=code

[cweagans]

There are plenty public results already that proves this feature is widely used

Not disputing that it's widely used nor that it has some utility. I'm not sure that it's something that the majority of users use though. The search you referenced is ~1.2k files. Composer Patches gets 55k+ installs per day.

[mxr576]

The search you referenced is ~1.2k files. Composer Patches gets 55k+ installs per day.

I do not think we have quantitative data at hand to confirm or deny either of our theories.

That 55k most likely includes builds on CIs, for PR/MR/CRs and who knows what else. My results only includes public repositories on Github. Nobody can guess how many private projects exists that uses this feature and nobody has time to crawl the public web to get a better aggregated result.

I think we/you must depend on community feedback. It is a complicated situation, I understand.

[cweagans]

For sure. It's not a done deal. I'm open to making the functionality available again if removing it is a deal breaker.

[podarok]

YMCA Website Services ( former Open Y ) rely on this functionality

[Sweetchuck]

.. dependency patch resolution ...

For me this feature caused several problems.

I am not familiar with all the features of version 2.x, but it would be useful to see a list of patches defined in the dependencies. Based on that list I could decide if I want to "cherry-pick" a patch definition into the root project or not.

[cweagans]

To all have that weighed in here, thank you. I am considering other options and will update when I have more information.

[jackrabbithanna]

CiviCRM has patches that are applied, and then we pull in CiviCRM Core to a website's codebase.
https://github.com/civicrm/civicrm-core/blob/master/composer.json#L272

We need those patches to apply, and we also sometimes have our own additional patches in the root composer.json

A change in behavior would cause many sites friction

[cweagans]

Noted, thank you!

[davereid-pfg]

We have a private, internal install profile that helps run 10+ websites on my current client, and we list mostly bugfix patches in our composer.json to help distribute the same patches to everyone since we don't control everyone's root composer.json files. This would leave us in a pretty bad spot if this went away.

[davereid-pfg]

Seems to be a thread here that this is most essential for install profiles/distributions and not as much for individual projects.

[cweagans]

Yep. That's what I'm seeing too. Probably going to re-add it.

[greg-1-anderson]

Pantheon custom upstreams use path repositories to allow the upstream to define dependencies in a subproject that lives inside the upstream repository. This allows individual sites to control the top-level composer.json file, and keeps the dependencies provided by the upstream separate. The Pantheon upstream update process is based on git merge, so the separation of files (especially .json files) is important to avoid merge conflicts.

Upstreams frequently need to add patches. This use case is similar to the install profile / distribution use case mentioned above. Having the continuing capability to patch from dependencies in composer-patches core would be useful.

[mxr576]

@cweagans Do you have any update that you can share about the future plans? A couple of days passed, I wonder what alternative solutions you have in your mind for replacing this feature. Of course, if you need more time, no problem, I've just wanted to ask...

[cweagans]

Hi all, thank you for your patience. I've made the decision to not include this functionality in 2.0.0. I know this is not the decision that any of you wanted and I'm sorry for the hassle it's going to cause. I wrote down some of my reasoning here: https://www.cweagans.net/2023/07/dependency-patch-resolution/

I gave some consideration to making the dependency patch resolution functionality a paid plugin somehow (either through distribution with keygen.sh or paid repo access via Github Sponsors or something along those lines), but the logistics of doing so would cause more problems than it was worth -- not to mention all of the headaches that come with maintaining that functionality in the first place.

I'll leave this discussion open for a bit in case there are any follow up questions/concerns/etc.

[mxr576]

Thanks @cweagans for the detailed answer and your insights on the burden around maintaining this widely used package. This is probably not the answer that the community and several ppl wanted to see but it is the answer you needed to have. 💚

Personally I do not like that the world needs to run on patched software versions and most probably it become (would become) a better and safer place without supporting applying patches from transitive dependencies.

[hudri]

I can understand that this needs some sort of control by the root composer.json

Would it be possible to include an explicit, manual, safe-listing in the root somehow similar to allow-plugin ? E.g. we have something like this in root

  "config": {
    "allow-plugins": {
      "composer/installers": true,
      "drupal/core-composer-scaffold": true,
      "phpstan/extension-installer": true,
      "dealerdirect/phpcodesniffer-composer-installer": true,
      "cweagans/composer-patches": true
    }
  },

Would it be feasible to create something similar for patches?

  "config": {
    "allow-patches": {
      "my_custom/metapackage": true,
      "drupal/big_distro": true
    }
  },

The root would be implicitly allowed, but all other dependencies need to be in the safelist. This way it would be controlled by the user of the root.

[mxr576]

This idea was surely raised before (check closed issues), but did not get picked up. It also seems a viable approach to me.

[cweagans]

Could work. Not sure. Somebody else is going to have to own that though. It's not something that I'll be implementing.

[markfullmer]

For folks who have been following this thread and continue to have the need to define patches in packages (dependencies) outside the Composer project composer.json, I'd like to call out an alternate library that does continue to support this use case: https://github.com/vaimo/composer-patches

Patches can be defined both on project and on package level in package config or separate JSON file.

[cweagans]

Hi all, thank you again for all the feedback here. I've been thinking about this a lot and it seems like this is really the biggest thing that is holding people up from switching to 2.x. I had an idea on this front that might make it possible to support resolving patches from dependencies without making my life difficult.

Issue can be found here: #546

I don't know when I'm going to have time to work on it, but feedback on the approach would be appreciated.

[drumm]

Packages.drupal.org traffic is already less than 0.5% Composer 1. Modern Drupal Core has required Composer 2 for some time. I don't think people are held back.

[markfullmer]

I think version 2 here is referring to cweagans/composer-patches (which drops support for patches in dependencies), not composer/composer version 2.

[rajeshreeputra]

I have extended 2.x version and created separate plugin rajeshreeputra/composer-dynamic-patching which provide an ability to apply version based patching, please review and provide your feedback. Thanks!

[cweagans]

Welp. I started building the import command and it turns out that most of what that command would be doing is already done by the resolver functionality already in the plugin. Instead of doing that thing I just said...well..

#547