From fdbb6adb132e242bb66c0c37af4ab0d8376e27fc Mon Sep 17 00:00:00 2001 From: Vasily Pyatykh Date: Wed, 20 Sep 2023 14:02:13 +0200 Subject: [PATCH] Fix a bug with a disjunctive license (#192) This PR fixes the behavior with a disjunctive license (e.g. "MIT, WTFPL, GPL-3.0-only"). Disjunctive license means that the user can choose any license from the list. Therefore, `composer-license-checker` should allow the package if _at least one_ license is allowed. Before, it was checking all licenses from the list and demanding all of them to be allowed. --------- Co-authored-by: vasily.pyatykh --- src/Event/PackageWithViolatingLicense.php | 5 +- src/LicenseChecker.php | 14 +++--- src/LicenseConfiguration.php | 15 +++++- src/Licenses.php | 39 +++++++++++++++ src/Output/Display.php | 5 +- src/Output/DisplayOutputSubscriber.php | 2 +- src/Output/HumanReadableDisplay.php | 26 +++++++--- src/Output/JsonDisplay.php | 6 ++- src/Package.php | 10 ++-- .../ComposerInstalledJsonPackagesProvider.php | 4 +- .../ComposerLicensesPackagesProvider.php | 3 +- tests/e2e/LicenseCheckerCase.php | 1 - .../Output/DisplayOutputSubscriberTest.php | 10 ++-- tests/support/CommandTesterAsserter.php | 13 +++-- tests/support/PackageAsserter.php | 20 +++----- tests/support/PackagesAsserter.php | 49 ++++--------------- ...poserInstalledJsonPackagesProviderTest.php | 13 ++--- .../ComposerLicensesPackagesProviderTest.php | 4 +- tests/unit/PackagesTest.php | 13 ++--- 19 files changed, 141 insertions(+), 111 deletions(-) create mode 100644 src/Licenses.php diff --git a/src/Event/PackageWithViolatingLicense.php b/src/Event/PackageWithViolatingLicense.php index 64ce0d1f..082371e8 100644 --- a/src/Event/PackageWithViolatingLicense.php +++ b/src/Event/PackageWithViolatingLicense.php @@ -8,10 +8,7 @@ final class PackageWithViolatingLicense implements Event { - /** - * @param non-empty-string $license - */ - public function __construct(public readonly Package $package, public readonly string $license) + public function __construct(public readonly Package $package) { } } diff --git a/src/LicenseChecker.php b/src/LicenseChecker.php index 92e9d0b6..55922808 100644 --- a/src/LicenseChecker.php +++ b/src/LicenseChecker.php @@ -167,21 +167,19 @@ protected function execute(InputInterface $input, OutputInterface $output): int continue; } - if ($package->licenses === []) { + if ($package->isUnlicensed()) { $violation = true; $this->dispatcher->dispatch(new UnlicensedPackageNotExplicitlyAllowed($package)); continue; } - foreach ($package->licenses as $license) { - if ($config->allowsLicense($license)) { - continue; - } - - $violation = true; - $this->dispatcher->dispatch(new PackageWithViolatingLicense($package, $license)); + if ($config->allowsLicenseOfPackage($package)) { + continue; } + + $violation = true; + $this->dispatcher->dispatch(new PackageWithViolatingLicense($package)); } if ($violation) { diff --git a/src/LicenseConfiguration.php b/src/LicenseConfiguration.php index baf7c883..f8896e77 100644 --- a/src/LicenseConfiguration.php +++ b/src/LicenseConfiguration.php @@ -17,9 +17,15 @@ public function __construct( ) { } - public function allowsLicense(string $license): bool + public function allowsLicenseOfPackage(Package $package): bool { - return \in_array($license, $this->allowedLicenses, true); + foreach ($package->licenses as $license) { + if ($this->allowsLicense($license)) { + return true; + } + } + + return false; } public function allowsPackage(string $package): bool @@ -32,4 +38,9 @@ public function allowsPackage(string $package): bool return false; } + + private function allowsLicense(string $license): bool + { + return \in_array($license, $this->allowedLicenses, true); + } } diff --git a/src/Licenses.php b/src/Licenses.php new file mode 100644 index 00000000..54c9b749 --- /dev/null +++ b/src/Licenses.php @@ -0,0 +1,39 @@ + + */ +final class Licenses implements \IteratorAggregate +{ + /** + * @param list $licenses + */ + public function __construct( + private readonly array $licenses, + ) { + } + + public function toString(): string + { + return \implode(', ', $this->licenses); + } + + public function isEmpty(): bool + { + return $this->licenses === []; + } + + public function getIterator(): \Traversable + { + return new \ArrayIterator($this->licenses); + } + + public function isDisjunctive(): bool + { + return \count($this->licenses) > 1; + } +} diff --git a/src/Output/Display.php b/src/Output/Display.php index 58ab73c3..2122cecf 100644 --- a/src/Output/Display.php +++ b/src/Output/Display.php @@ -19,10 +19,7 @@ public function onOutcomeFailure(): void; public function onOutcomeSuccess(): void; - /** - * @param non-empty-string $license - */ - public function onPackageWithViolatingLicense(Package $package, string $license): void; + public function onPackageWithViolatingLicense(Package $package): void; public function onUnlicensedPackageNotExplicitlyAllowed(Package $package): void; diff --git a/src/Output/DisplayOutputSubscriber.php b/src/Output/DisplayOutputSubscriber.php index d2a98262..a343a277 100644 --- a/src/Output/DisplayOutputSubscriber.php +++ b/src/Output/DisplayOutputSubscriber.php @@ -53,7 +53,7 @@ private function onOutcomeSuccess(OutcomeSuccess $event): void private function onPackageWithViolatingLicense(PackageWithViolatingLicense $event): void { - $this->display->onPackageWithViolatingLicense($event->package, $event->license); + $this->display->onPackageWithViolatingLicense($event->package); } private function onUnlicensedPackageNotExplicitlyAllowed(UnlicensedPackageNotExplicitlyAllowed $event): void diff --git a/src/Output/HumanReadableDisplay.php b/src/Output/HumanReadableDisplay.php index c9f8f45e..94e577c1 100644 --- a/src/Output/HumanReadableDisplay.php +++ b/src/Output/HumanReadableDisplay.php @@ -37,15 +37,25 @@ public function onOutcomeSuccess(): void $this->style->success('All dependencies have allowed licenses.'); } - public function onPackageWithViolatingLicense(Package $package, string $license): void + public function onPackageWithViolatingLicense(Package $package): void { - $this->style->error( - \sprintf( - 'Dependency "%s" has license "%s" which is not in the allowed list.', - $package->name->toString(), - $license, - ), - ); + if ($package->licenses->isDisjunctive()) { + $this->style->error( + \sprintf( + 'Dependency "%s" is licensed under any of "%s", none of which are allowed.', + $package->name->toString(), + $package->licenses->toString(), + ), + ); + } else { + $this->style->error( + \sprintf( + 'Dependency "%s" is licensed under "%s" which is not in the allowed list.', + $package->name->toString(), + $package->licenses->toString(), + ), + ); + } } public function onUnlicensedPackageNotExplicitlyAllowed(Package $package): void diff --git a/src/Output/JsonDisplay.php b/src/Output/JsonDisplay.php index 34bc045a..74a44de7 100644 --- a/src/Output/JsonDisplay.php +++ b/src/Output/JsonDisplay.php @@ -43,9 +43,11 @@ public function onFatalError(string $message): void ); } - public function onPackageWithViolatingLicense(Package $package, string $license): void + public function onPackageWithViolatingLicense(Package $package): void { - $this->violations[$license][] = $package->name->toString(); + /** @var non-empty-string $licences */ + $licences = $package->licenses->toString(); + $this->violations[$licences][] = $package->name->toString(); } public function onUnlicensedPackageNotExplicitlyAllowed(Package $package): void diff --git a/src/Package.php b/src/Package.php index a188679e..ba76c54f 100644 --- a/src/Package.php +++ b/src/Package.php @@ -6,12 +6,14 @@ final class Package { - /** - * @param list $licenses - */ public function __construct( public readonly PackageName $name, - public readonly array $licenses, + public readonly Licenses $licenses, ) { } + + public function isUnlicensed(): bool + { + return $this->licenses->isEmpty(); + } } diff --git a/src/PackagesProvider/ComposerInstalledJsonPackagesProvider.php b/src/PackagesProvider/ComposerInstalledJsonPackagesProvider.php index 63451c64..a85b920a 100644 --- a/src/PackagesProvider/ComposerInstalledJsonPackagesProvider.php +++ b/src/PackagesProvider/ComposerInstalledJsonPackagesProvider.php @@ -5,6 +5,7 @@ namespace Lendable\ComposerLicenseChecker\PackagesProvider; use Lendable\ComposerLicenseChecker\Exception\FailedProvidingPackages; +use Lendable\ComposerLicenseChecker\Licenses; use Lendable\ComposerLicenseChecker\Package; use Lendable\ComposerLicenseChecker\PackageName; use Lendable\ComposerLicenseChecker\Packages; @@ -84,11 +85,10 @@ static function (mixed $package) use ($skipPackages): ?Package { throw FailedProvidingPackages::withReason('Key "license" is not a list'); } - /** @var list $licenses */ /** @var PackageData $package */ return new Package( new PackageName($package['name']), - $licenses, + new Licenses($licenses), ); }, $dependencies, diff --git a/src/PackagesProvider/ComposerLicensesPackagesProvider.php b/src/PackagesProvider/ComposerLicensesPackagesProvider.php index 52553538..b465f1e3 100644 --- a/src/PackagesProvider/ComposerLicensesPackagesProvider.php +++ b/src/PackagesProvider/ComposerLicensesPackagesProvider.php @@ -7,6 +7,7 @@ use Lendable\ComposerLicenseChecker\ComposerRunner; use Lendable\ComposerLicenseChecker\Exception\FailedProvidingPackages; use Lendable\ComposerLicenseChecker\Exception\FailedRunningComposer; +use Lendable\ComposerLicenseChecker\Licenses; use Lendable\ComposerLicenseChecker\Package; use Lendable\ComposerLicenseChecker\PackageName; use Lendable\ComposerLicenseChecker\Packages; @@ -45,7 +46,7 @@ public function provide(string $projectPath, bool $ignoreDev): Packages return new Packages( \array_map( - static fn (string $name, array $package): Package => new Package(new PackageName($name), $package['license']), + static fn (string $name, array $package): Package => new Package(new PackageName($name), new Licenses($package['license'])), \array_keys($data['dependencies']), $data['dependencies'], ), diff --git a/tests/e2e/LicenseCheckerCase.php b/tests/e2e/LicenseCheckerCase.php index ae13bc46..167b9d5c 100644 --- a/tests/e2e/LicenseCheckerCase.php +++ b/tests/e2e/LicenseCheckerCase.php @@ -155,7 +155,6 @@ public function test_report_not_allowed_licenses(): void ->foundLicensingIssues( [ 'lendable/apache' => 'Apache-2.0', - 'lendable/bsd3_mit' => 'BSD-3-Clause', 'package/bsd3' => 'BSD-3-Clause', ], ); diff --git a/tests/e2e/Output/DisplayOutputSubscriberTest.php b/tests/e2e/Output/DisplayOutputSubscriberTest.php index 16e25b9e..db15aff1 100644 --- a/tests/e2e/Output/DisplayOutputSubscriberTest.php +++ b/tests/e2e/Output/DisplayOutputSubscriberTest.php @@ -12,6 +12,7 @@ use Lendable\ComposerLicenseChecker\Event\Started; use Lendable\ComposerLicenseChecker\Event\TraceInformation; use Lendable\ComposerLicenseChecker\Event\UnlicensedPackageNotExplicitlyAllowed; +use Lendable\ComposerLicenseChecker\Licenses; use Lendable\ComposerLicenseChecker\Output\Display; use Lendable\ComposerLicenseChecker\Output\DisplayOutputSubscriber; use Lendable\ComposerLicenseChecker\Package; @@ -75,20 +76,19 @@ public function test_delegates_on_outcome_success(): void public function test_delegates_on_package_with_violating_license(): void { - $package = new Package(new PackageName('foo/bar'), ['MIT']); - $license = 'MIT'; + $package = new Package(new PackageName('foo/bar'), new Licenses(['MIT'])); $this->display ->expects(self::once()) ->method('onPackageWithViolatingLicense') - ->with($package, $license); + ->with($package); - $this->dispatcher->dispatch(new PackageWithViolatingLicense($package, $license)); + $this->dispatcher->dispatch(new PackageWithViolatingLicense($package)); } public function test_delegates_on_unlicensed_package_not_explicitly_allowed(): void { - $package = new Package(new PackageName('foo/bar'), ['MIT']); + $package = new Package(new PackageName('foo/bar'), new Licenses(['MIT'])); $this->display ->expects(self::once()) diff --git a/tests/support/CommandTesterAsserter.php b/tests/support/CommandTesterAsserter.php index 2da88654..89af191a 100644 --- a/tests/support/CommandTesterAsserter.php +++ b/tests/support/CommandTesterAsserter.php @@ -81,11 +81,18 @@ public function foundLicensingIssues(array $issues): self } if (\is_array($license)) { - foreach ($license as $element) { + if (\count($license) > 1) { $expectedOutput[] = \sprintf( - ' [ERROR] Dependency "%s" has license "%s" which is not in the allowed list.', + ' [ERROR] Dependency "%s" is licensed under any of "%s", none of which are allowed.', $package, - $element, + \implode(', ', $license), + ); + $expectedOutput[] = ''; + } else { + $expectedOutput[] = \sprintf( + ' [ERROR] Dependency "%s" is licensed under "%s" which is not in the allowed list.', + $package, + \implode(', ', $license), ); $expectedOutput[] = ''; } diff --git a/tests/support/PackageAsserter.php b/tests/support/PackageAsserter.php index 2b47b130..248e05bb 100644 --- a/tests/support/PackageAsserter.php +++ b/tests/support/PackageAsserter.php @@ -4,6 +4,7 @@ namespace Tests\Support\Lendable\ComposerLicenseChecker; +use Lendable\ComposerLicenseChecker\Licenses; use Lendable\ComposerLicenseChecker\Package; use Lendable\ComposerLicenseChecker\PackageName; use PHPUnit\Framework\Assert; @@ -30,32 +31,25 @@ public function hasName(string|PackageName $name): self return $this; } - public function hasLicense(string $license): self + public function equals(Package $package): self { - Assert::assertContains($license, $this->package->licenses); + $this->hasName($package->name); + $this->hasExactLicenses($package->licenses); return $this; } - /** - * @param list $licenses - */ - public function hasExactLicenses(array $licenses): self + private function hasExactLicenses(Licenses $licenses): void { Assert::assertSameSize($licenses, $this->package->licenses); foreach ($licenses as $license) { $this->hasLicense($license); } - - return $this; } - public function equals(Package $package): self + private function hasLicense(string $license): void { - $this->hasName($package->name); - $this->hasExactLicenses($package->licenses); - - return $this; + Assert::assertContains($license, \explode(', ', $this->package->licenses->toString())); } } diff --git a/tests/support/PackagesAsserter.php b/tests/support/PackagesAsserter.php index 0696ae1a..f4b2e39f 100644 --- a/tests/support/PackagesAsserter.php +++ b/tests/support/PackagesAsserter.php @@ -4,10 +4,8 @@ namespace Tests\Support\Lendable\ComposerLicenseChecker; -use Lendable\ComposerLicenseChecker\Package; use Lendable\ComposerLicenseChecker\Packages; use PHPUnit\Framework\Assert; -use PHPUnit\Framework\AssertionFailedError; final class PackagesAsserter { @@ -20,43 +18,6 @@ public static function assertThat(Packages $packages): self return new self($packages); } - public function hasCount(int $count): self - { - Assert::assertCount($count, $this->packages); - - return $this; - } - - /** - * @param \Countable|array $countable - */ - public function sameSize(\Countable|array $countable): self - { - Assert::assertSameSize($countable, $this->packages); - - return $this; - } - - public function containsPackage(Package $package): self - { - foreach ($this->packages as $existing) { - try { - PackageAsserter::assertThat($existing)->equals($package); - - return $this; - } catch (AssertionFailedError) { - } - } - - Assert::fail( - \sprintf( - 'Failed to find a package with name "%s" and licenses [%s].', - $package->name->toString(), - \implode(', ', $package->licenses), - ), - ); - } - public function equals(Packages $packages): self { $this->sameSize($packages); @@ -71,4 +32,14 @@ public function equals(Packages $packages): self return $this; } + + /** + * @param \Countable|array $countable + */ + private function sameSize(\Countable|array $countable): self + { + Assert::assertSameSize($countable, $this->packages); + + return $this; + } } diff --git a/tests/unit/PackagesProvider/ComposerInstalledJsonPackagesProviderTest.php b/tests/unit/PackagesProvider/ComposerInstalledJsonPackagesProviderTest.php index 0ee4a0ac..6e454ac2 100644 --- a/tests/unit/PackagesProvider/ComposerInstalledJsonPackagesProviderTest.php +++ b/tests/unit/PackagesProvider/ComposerInstalledJsonPackagesProviderTest.php @@ -5,6 +5,7 @@ namespace Tests\Unit\Lendable\ComposerLicenseChecker\PackagesProvider; use Lendable\ComposerLicenseChecker\Exception\FailedProvidingPackages; +use Lendable\ComposerLicenseChecker\Licenses; use Lendable\ComposerLicenseChecker\Package; use Lendable\ComposerLicenseChecker\PackageName; use Lendable\ComposerLicenseChecker\Packages; @@ -51,7 +52,7 @@ public static function provideInstalledJsonAndExpectedPackages(): iterable 'dev-package-names' => [], 'packages' => [['name' => 'foo/bar', 'version' => '1.0.0', 'license' => ['MIT']]], ], - new Packages([new Package(new PackageName('foo/bar'), ['MIT'])]), + new Packages([new Package(new PackageName('foo/bar'), new Licenses(['MIT']))]), true, ]; yield 'ignoring dev with 1 dev dependency' => [ @@ -69,7 +70,7 @@ public static function provideInstalledJsonAndExpectedPackages(): iterable 'dev-package-names' => ['foo/bar'], 'packages' => [['name' => 'foo/bar', 'version' => '1.0.0', 'license' => ['MIT']]], ], - new Packages([new Package(new PackageName('foo/bar'), ['MIT'])]), + new Packages([new Package(new PackageName('foo/bar'), new Licenses(['MIT']))]), false, ]; yield 'not ignoring dev with 1 dev dependency and 1 runtime' => [ @@ -82,8 +83,8 @@ public static function provideInstalledJsonAndExpectedPackages(): iterable ], ], new Packages([ - new Package(new PackageName('foo/bar'), ['MIT']), - new Package(new PackageName('foo/baz'), ['MIT', 'Apache-2.0']), + new Package(new PackageName('foo/bar'), new Licenses(['MIT'])), + new Package(new PackageName('foo/baz'), new Licenses(['MIT', 'Apache-2.0'])), ]), false, ]; @@ -97,7 +98,7 @@ public static function provideInstalledJsonAndExpectedPackages(): iterable ], ], new Packages([ - new Package(new PackageName('foo/baz'), ['MIT', 'Apache-2.0']), + new Package(new PackageName('foo/baz'), new Licenses(['MIT', 'Apache-2.0'])), ]), true, ]; @@ -111,7 +112,7 @@ public static function provideInstalledJsonAndExpectedPackages(): iterable ], ], new Packages([ - new Package(new PackageName('foo/bar'), []), + new Package(new PackageName('foo/bar'), new Licenses([])), ]), true, ]; diff --git a/tests/unit/PackagesProvider/ComposerLicensesPackagesProviderTest.php b/tests/unit/PackagesProvider/ComposerLicensesPackagesProviderTest.php index 00f3be1b..65237bc7 100644 --- a/tests/unit/PackagesProvider/ComposerLicensesPackagesProviderTest.php +++ b/tests/unit/PackagesProvider/ComposerLicensesPackagesProviderTest.php @@ -35,10 +35,10 @@ public function test_returns_parsed_packages(): void self::assertCount(2, $packages); self::assertSame('vendor/project', $packages[0]->name->toString()); - self::assertSame(['MIT', 'LGPL'], $packages[0]->licenses); + self::assertSame('MIT, LGPL', $packages[0]->licenses->toString()); self::assertSame('vendor2/project', $packages[1]->name->toString()); - self::assertSame(['WTFPL'], $packages[1]->licenses); + self::assertSame('WTFPL', $packages[1]->licenses->toString()); } public function test_wraps_and_throws_composer_runner_failure(): void diff --git a/tests/unit/PackagesTest.php b/tests/unit/PackagesTest.php index af1ee6c5..c94c951e 100644 --- a/tests/unit/PackagesTest.php +++ b/tests/unit/PackagesTest.php @@ -4,6 +4,7 @@ namespace Tests\Unit\Lendable\ComposerLicenseChecker; +use Lendable\ComposerLicenseChecker\Licenses; use Lendable\ComposerLicenseChecker\Package; use Lendable\ComposerLicenseChecker\PackageName; use Lendable\ComposerLicenseChecker\Packages; @@ -14,9 +15,9 @@ final class PackagesTest extends TestCase public function test_returns_instance_with_sorted_packages(): void { $packages = new Packages([ - new Package(new PackageName('c/d'), []), - new Package(new PackageName('x/y'), []), - new Package(new PackageName('c/b'), []), + new Package(new PackageName('c/d'), new Licenses([])), + new Package(new PackageName('x/y'), new Licenses([])), + new Package(new PackageName('c/b'), new Licenses([])), ]); $sorted = $packages->sort(); @@ -31,10 +32,10 @@ public function test_returns_instance_with_sorted_packages(): void public function test_returns_count(): void { self::assertCount(0, new Packages([])); - self::assertCount(1, new Packages([new Package(new PackageName('a/b'), [])])); + self::assertCount(1, new Packages([new Package(new PackageName('a/b'), new Licenses([]))])); self::assertCount(2, new Packages([ - new Package(new PackageName('a/a'), []), - new Package(new PackageName('a/b'), []), + new Package(new PackageName('a/a'), new Licenses([])), + new Package(new PackageName('a/b'), new Licenses([])), ])); } }