-
-
Notifications
You must be signed in to change notification settings - Fork 242
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[#551] Dependency patch resolution: Only allow patches from trusted d… #588
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -24,21 +24,42 @@ public function resolve(PatchCollection $collection): void | |
return; | ||
} | ||
|
||
$this->io->write(' - <info>Resolving patches from dependencies.</info>'); | ||
|
||
// Using both config keys, if $allowed_dependencies is set, it takes precedence | ||
$allowed_dependencies = $this->plugin->getConfig('allow-dependency-patches'); | ||
$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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe |
||
$this->io->write(' - <info>No patches from dependencies are allowed.</info>'); | ||
return; | ||
} | ||
|
||
$this->io->write(' - <info>Resolving patches from dependencies.</info>'); | ||
|
||
$lockdata = $locker->getLockData(); | ||
foreach ($lockdata['packages'] as $p) { | ||
// If we're supposed to skip gathering patches from a dependency, do that. | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We have 4 possibilities, if we remain the ignored dependencies possibility: 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. |
||
|
||
// Allowed dependencies is not set in composer.json, and we're not | ||
// supposed to skip gathering patches from this dependency | ||
if (is_null($allowed_dependencies) && !$ignored) { | ||
$this->lookForPatches($p, $collection); | ||
} | ||
|
||
// Find patches in the composer.json for dependencies. | ||
if (!isset($p['extra']) || !isset($p['extra']['patches'])) { | ||
continue; | ||
// Allowed dependencies are set, act only on allowed, if they're not | ||
// ignored also. | ||
if ($allowed && !$ignored) { | ||
$this->lookForPatches($p, $collection); | ||
} | ||
} | ||
} | ||
|
||
private function lookForPatches(array $p, PatchCollection $collection): void | ||
{ | ||
|
||
// Find patches in the composer.json for dependencies. | ||
if (isset($p['extra']['patches'])) { | ||
foreach ($this->findPatchesInJson($p['extra']['patches']) as $package => $patches) { | ||
foreach ($patches as $patch) { | ||
$patch->extra['provenance'] = "dependency:" . $package; | ||
|
@@ -47,8 +68,11 @@ public function resolve(PatchCollection $collection): void | |
$collection->addPatch($patch); | ||
} | ||
} | ||
} | ||
|
||
// TODO: Also find patches in a configured patches.json for the dependency. | ||
// Find patches in a patch-file for dependencies. | ||
if (isset($p['extra']['composer-patches']['patches-file'])) { | ||
// @todo Handle patch files. | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably default to
[]