[#551] Dependency patch resolution: Only allow patches from trusted d… #588
Conversation
…rusted dependencies
| ], | ||
| "allow-dependency-patches" => [ | ||
| 'type' => 'list', | ||
| 'default' => null, |
There was a problem hiding this comment.
This should probably default to []
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
There aren't existing projects that are using 2.x with this functionality, as this functionality does not yet exist.
2.x uses explicit configuration options and very little is implied by other options. If you don't want dependency patch resolution at all, then you simply disable that resolver. This option will be for changing the behavior, not disabling it.
| $ignored_dependencies = $this->plugin->getConfig('ignore-dependency-patches'); | ||
|
|
||
| // First check, if we do allow dependency patches at all. | ||
| if ($allowed_dependencies === []) { |
There was a problem hiding this comment.
Maybe empty($allowed_dependencies) in light of defaulting the value to []?
| if (in_array($p['name'], $ignored_dependencies)) { | ||
| continue; | ||
| $allowed = in_array($p['name'], $allowed_dependencies ?? []); | ||
| $ignored = in_array($p['name'], $ignored_dependencies); |
There was a problem hiding this comment.
Maybe we should simplify this a bit and disallow setting both allowed/ignored dependencies. You can either say "I only want patches from these listed deps" or "I want patches from all deps except these specific ones"
It doesn't make sense to say "I want patches from all deps except these specific ones, but only from these listed deps"
There was a problem hiding this comment.
We have 4 possibilities, if we remain the ignored dependencies possibility:
a) Current behavior: i have no allow-dependency-patches, and no ignored-dependency-patches, i wanted to preserve that all dependent patches will be used. (Case null)
b) Current behavior: I have no allow-dependency-patches, and some ignored-dependency-patches, all dependent patches should be installed except those listed in ignored. (Case null)
c) New behavior: I do not want any dependency patches, so allow-dependency should be a empty list. (Case [] )
d) New behavior: I want do allow some packages, but because of composer-merge i forgot, i have forbitten in another file package x/y to dependent patches. So this will not break existing installations but is really an enhancement without breaking.
If its okay to break current behavior, we could do differently. But i wanted to keep existing behavior, and wanted to keep expected behavior, like an empty list of allowed dependency implicit none is allowed, and if i ignore something, it will be ignored.
|
Bumping to draft for now. I might fix this up if you don't beat me to it @metalbote |
3 participants
…ependencies
Description
Relates to #551.
Added config key as decribed in the issue, added handling for whitelist, while not breaking current behavior if new config key is omitted.
Related tasks