Comments (19)
I have been having the same issue, it's not ideal. (Will work on a PR).
from composer-patches.
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?
from composer-patches.
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.
from composer-patches.
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.
from composer-patches.
@mathieuhelie If you have time, I'd like your input on ^ too.
from composer-patches.
Ah, that makes sense. Maybe my use case is pretty unique.
from composer-patches.
I'm not saying no. Let's wait till we have more feedback on it, though.
from composer-patches.
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.
from composer-patches.
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.
from composer-patches.
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?
from composer-patches.
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
from composer-patches.
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.
from composer-patches.
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.
from composer-patches.
@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.
from composer-patches.
As long as the last few issues filed aren't regressions, master + the docs update for enable-patching is good by me.
from composer-patches.
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.
from composer-patches.
The issue is still present
from composer-patches.
main
no longer resolves patches from dependencies automatically, so I think this is now a non-issue.
from composer-patches.
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!
from composer-patches.
Related Issues (20)
- The patch is partially applied HOT 3
- [2.0.0-beta1] No available patcher was able to apply patch ( for drupal/core ) HOT 7
- Composer require hangs sometimes when a patch applies HOT 2
- Removing package for re-install and re-patch does not trigger if patches definition is empty (1.7.3) HOT 1
- I'm Confused By Some of the Directions HOT 5
- [2.*] Make lock file path configurable HOT 2
- Write copy of a patch to a project directory for code review HOT 6
- Link in the README 404s: Documentation: https://docs.cweagans.net/composer-patches HOT 2
- Add a `patches-import` subcommand as a way to support discovering patches from dependencies HOT 4
- Dependency patch resolution: use patches file from dependency instead of root composer if configured HOT 1
- Dependency patch resolution: resolve file paths to local patches stored in dependencies HOT 5
- Mention my d10up article
- Dependency patch resolution: Only allow patches from trusted dependencies HOT 10
- Add info about composer -v for extended verbosity HOT 2
- Support and documentation on the upgrade process from 1.7.3 to 2.0.0-beta2 HOT 17
- Add a way to specify the `--include` and `--exclude` flags for `git apply` HOT 1
- Is patch provenance reported correctly?
- Add section about manually making sure patches apply
- Rename `PatchEvents:: PRE_PATCH_GUESS_DEPTH` HOT 1
- Possibilities to give the command "patches-repatch" options for the composer install HOT 4
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from composer-patches.