Patches in packages not installed unless root-level composer.json defines a patches key in extra.

[mathieuhelie]

In Patches.php:104 there is a return statement when no patches or patches-file key is found in the root-level extra key of composer. This means that patches that are defined in required packages are not read or applied (this is done at line 107). In order to get patches in required packages to apply, we must add "patches": {} in the extra section of the root-level composer.json file.

If this is desired behavior, an exception should be thrown explaining what is expected.

[timmillwood]

I have been having the same issue, it's not ideal. (Will work on a PR).

[cweagans]

Honestly, I think I'm okay with this. It avoids the situation where a dependency package requires cweagans/composer-patches and then starts patching any number of other arbitrary projects. Having to add the line in the root composer.json is kind of an "opt-in" for being able to patch things.

Can you describe what circumstances you'd actually want this to happen?

[timmillwood]

ok, my use case is:

A Drupal module (currently custom, might be contrib soon) depends on a php library, but there is an open PR for the php library which is needed for this. Therefore looking to add that patch to the PHP library when the module is added (via composer). Therefore the patch plugin won't be defined in the root composer.json, it'd be defined in the module's composer.json.

[cweagans]

Right, I understand that part. Let's say some other user wants to use that module and they currently don't have any patches on their project. Is it okay to start mucking around with dependencies on the rest of their site without them knowing it happened?

As it stands right now, they have to opt-in to the patches behavior. If we go down the route of making it on-by-default, we need to have an opt-out switch too. The question is, though, is on-by-default a good choice for something that could potentially introduce security holes into somebody's application? It only takes one malicious library doing that to cause a very large problem for anyone using the lib, so I want to make sure we're covering our bases here.

[cweagans]

@mathieuhelie If you have time, I'd like your input on ^ too.

[timmillwood]

Ah, that makes sense. Maybe my use case is pretty unique.

[cweagans]

I'm not saying no. Let's wait till we have more feedback on it, though.

[mathieuhelie]

In my opinion we shouldn't be using patches for requiring libraries, since other packages might be requiring this library and be incompatible with the patch. This kind of conflict resolution should be handled by Composer's versioning syntax only. If your module requires a PR then you should be making it dependent on a fork of that library with that PR in (timmillwood/thelibrary instead of libraryguys/thelibrary), therefore it's obvious when we build the project which version of the library we have.

The patching workflow should stay within the boundary of Drupal's ecosystem, since we have no simple way to clone and PR Drupal modules and core.

I raised this issue to highlight a surprising behavior I encountered when building a project with a custom profile using composer, featuring patches in the profile. I agree with requiring the root-level composer.json to define a patches key as an "opt-in" of sorts, but the behavior should be better documented. (We should know what the problem is without having to dive into the code.) A simple exception, or even a notice message before returning control to composer, would resolve this issue.

[ericduran]

I'm with @mathieuhelie on this. I think it's a matter of not getting proper info right away. I think even @cweagans saw this as he was writing it since the return statement is prefix with // @todo: should we throw an exception here? :)

I would be in favor us just throwing an exception, the thing that sounds scary to me is the scenario @cweagans describe:

It avoids the situation where a dependency package requires cweagans/composer-patches and then starts patching any number of other arbitrary projects.

[deviantintegral]

I've just run into this myself - we've been having to rely pretty heavily on patches for various modules still in early ports to Drupal 8. I think the biggest issue is the README stating:

This plugin will gather patches from all dependencies and apply them as if they were in the root composer.json

which is clearly false :)

Two ways I see to handle this:

• Add a config key (root only) along the lines of "always patch". This requires users to opt-in to patches, but could be easily missed by module users.
• Add a config to extra along the lines of "patch when package is required".

Actually, these aren't exclusive. How about implementing both of them?

[deviantintegral]

Actually, setting patches to an empty object in the extra config doesn't work, because of array() == FALSE in https://github.com/cweagans/composer-patches/blob/master/src/Patches.php#L95

[cweagans]

I think point one should have been handled by #36. I definitely don't want to make patching opt-in, but I'm okay with adding config flags as long as there are sensible defaults (i.e. a user only has to add cweagans/composer-patches to their require list + specify the patches that they want to apply without configuring specific behaviors). The other thing is that if a user doesn't have anything about patching in their root composer.json, we don't want some potentially malicious package specifying patches for other projects and patching a bunch of things without the user's knowledge or consent.

As long as those two criteria are satisfied, I'll probably merge a PR.

[deviantintegral]

I've published a 1.4.0-p1 tag off of master over at https://github.com/deviantintegral/composer-patches if anyone wants to use it for testing. That plus enable-patching is working for me. I think it probably just needs a docs update as right now that setting is hidden in the code.

[cweagans]

@deviantintegral If you're happy with master, I don't mind tagging a release here. It's been a while since the last one. I agree with you on the docs fix, though. Probably a good idea to update that.

[deviantintegral]

As long as the last few issues filed aren't regressions, master + the docs update for enable-patching is good by me.

[acbramley]

I'm still having this issue with 1.5.0, this is due to Patches::grabPatches returning an empty array, which is equal to FALSE so it doesn't go into checking each package.

Infact, adding an empty patches key doesn't fix the issue either, the root package must have at least 1 patch in it to work.

EDIT: To clarify, it seems to work on the initial composer update when adding the depedency, but subsequent patches added to that dependency aren't added to the root package.

[milkovsky]

The issue is still present

[cweagans]

main no longer resolves patches from dependencies automatically, so I think this is now a non-issue.

[sprankhub]

main no longer resolves patches from dependencies automatically, so I think this is now a non-issue.

@cweagans, do I understand it correctly that it is now impossible to apply patches from dependencies? At least I could not find any configuration for this...? Thanks!