diff --git a/src/Config.php b/src/Config.php index 59ba38e..98f0e5d 100644 --- a/src/Config.php +++ b/src/Config.php @@ -230,14 +230,24 @@ public static function createFromViolinistConfigJsonString(string $data) public function setConfig($config) { + if (!$config instanceof \stdClass) { + return; + } + $config = $this->normalizeConfigKeys($config); foreach ($this->getDefaultConfig() as $key => $value) { if (isset($config->{$key})) { $this->config->{$key} = $config->{$key}; $this->configOptionsSet[$key] = true; } } - // Also make sure to set the block list config from the deprecated part. - // Plus alternative spelling from allow list. + if (!empty($config->rules)) { + $this->config->rules = $config->rules; + } + } + + private function normalizeConfigKeys(\stdClass $config) : \stdClass + { + $config = clone $config; $renamed_and_aliased = [ 'blacklist' => 'blocklist', 'block_list' => 'blocklist', @@ -245,12 +255,11 @@ public function setConfig($config) ]; foreach ($renamed_and_aliased as $not_real => $real) { if (isset($config->{$not_real})) { - $this->config->{$real} = $config->{$not_real}; + $config->{$real} = $config->{$not_real}; + unset($config->{$not_real}); } } - if (!empty($config->rules)) { - $this->config->rules = $config->rules; - } + return $config; } public function getComposerOutdatedFlag() : string @@ -544,37 +553,44 @@ public function getCommitMessageConvention() public function getConfigForRuleObject(\stdClass $rule_object) { - // @todo: This is a bit duplication from below actually. Specifically - // the method ::getConfigForPackage if (empty($rule_object->config)) { return $this; } - $new_config = clone $this->config; - $this->mergeConfigFromConfigObject($new_config, $rule_object->config); - return self::createFromViolinistConfig($new_config); + $clone = clone $this; + $clone->config = clone $this->config; + $clone->extendsStorage = clone $this->extendsStorage; + $normalized = $this->normalizeConfigKeys($rule_object->config); + foreach ($this->getDefaultConfig() as $key => $value) { + if (!isset($normalized->{$key})) { + continue; + } + $clone->config->{$key} = $normalized->{$key}; + $clone->configOptionsSet[$key] = true; + $clone->extendsStorage->removeItemsForKey($key); + } + return $clone; } public function getConfigForPackage(string $package_name) : self { - // @todo: Consider de-duplicating with the method above - // (::getConfigForRuleObject). $rules = $this->getRules(); if (empty($rules)) { return $this; } - $new_config = clone $this->config; - foreach ($this->config->rules as $rule) { - if (empty($rule->config)) { - continue; - } - $matches = $this->getMatcherFactory()->hasMatches($rule, $package_name); - if (!$matches) { + $result = $this; + foreach ($rules as $rule) { + if (!$this->getMatcherFactory()->hasMatches($rule, $package_name)) { continue; } - // Then merge the config for this rule. - $this->mergeConfigFromConfigObject($new_config, $rule->config); + $result = $result->getConfigForRuleObject($rule); } - return self::createFromViolinistConfig($new_config); + if ($result === $this) { + $clone = clone $this; + $clone->config = clone $this->config; + $clone->extendsStorage = clone $this->extendsStorage; + return $clone; + } + return $result; } public function getRules() : array @@ -589,8 +605,12 @@ protected function mergeConfig(Config $other, $extends_name, $parent) { $keys_and_values_affected = $this->mergeConfigFromConfigObject($this->getConfig(), $other->getConfig()); $this->extendsStorage->addExtendItems($other->getExtendsStorage()->getExtendItems()); + foreach ($this->getDefaultConfig() as $key => $value) { + if ($other->hasConfigForKey($key)) { + $this->configOptionsSet[$key] = true; + } + } foreach ($keys_and_values_affected as $key => $value) { - $this->configOptionsSet[$key] = true; $this->extendsStorage->addExtendItem(new ExtendsChainItem($extends_name, $key, $value)); } } diff --git a/src/ExtendsStorage.php b/src/ExtendsStorage.php index 5cb9001..a01acb7 100644 --- a/src/ExtendsStorage.php +++ b/src/ExtendsStorage.php @@ -24,4 +24,11 @@ public function addExtendItem(ExtendsChainItem $item) { $this->items[$item->getName()][$item->getKey()] = $item; } + + public function removeItemsForKey(string $key) + { + foreach ($this->items as $extend_name => $items) { + unset($this->items[$extend_name][$key]); + } + } } diff --git a/tests/RuleObjectConfigTest.php b/tests/RuleObjectConfigTest.php index dc6894e..d8311c0 100644 --- a/tests/RuleObjectConfigTest.php +++ b/tests/RuleObjectConfigTest.php @@ -7,6 +7,21 @@ class RuleObjectConfigTest extends TestCase { + public function testRuleWithDeprecatedAliasKeyIsNormalized() + { + $config = new Config(); + $rule = (object) [ + 'config' => (object) [ + 'blacklist' => ['vendor/package-a'], + ], + ]; + + $config_from_rule = $config->getConfigForRuleObject($rule); + + self::assertEquals(['vendor/package-a'], $config_from_rule->getBlockList()); + self::assertTrue($config_from_rule->hasConfigForKey('blocklist')); + } + public function testGetConfigForRuleObjectReturnsRuleConfig() { $config_data = json_decode(file_get_contents(__DIR__ . '/fixtures/violinist-test-drupal-config.json')); @@ -32,7 +47,7 @@ public function testRuleWithoutConfigReturnsOriginalConfig() self::assertSame($config, $result); } - public function testRuleDoesNotOverrideExistingConfig() + public function testRuleOverridesExistingConfig() { $config = Config::createFromViolinistConfig((object) [ 'update_dev_dependencies' => 0, @@ -47,6 +62,255 @@ public function testRuleDoesNotOverrideExistingConfig() self::assertNotSame($config, $config_from_rule); self::assertFalse($config->shouldUpdateDevDependencies()); - self::assertFalse($config_from_rule->shouldUpdateDevDependencies()); + self::assertTrue($config_from_rule->shouldUpdateDevDependencies()); + } + + public function testLaterRuleOverridesEarlierRuleForSamePackage() + { + // A catch-all rule sets security_updates_only for all matched packages. + // A more specific rule after it can override back to the default for + // specific packages, since rules can override each other. + $config_data = (object) [ + 'rules' => [ + (object) [ + 'name' => 'Security only for vendor packages', + 'matchRules' => [ + (object) ['type' => 'names', 'values' => ['vendor/*']], + ], + 'config' => (object) [ + 'security_updates_only' => 1, + ], + ], + (object) [ + 'name' => 'Always update these', + 'matchRules' => [ + (object) ['type' => 'names', 'values' => ['vendor/package-a', 'vendor/package-b']], + ], + 'config' => (object) [ + 'security_updates_only' => 0, + ], + ], + ], + ]; + $config = Config::createFromViolinistConfig($config_data); + + $config_for_package_a = $config->getConfigForPackage('vendor/package-a'); + self::assertFalse($config_for_package_a->shouldOnlyUpdateSecurityUpdates()); + + $config_for_package_b = $config->getConfigForPackage('vendor/package-b'); + self::assertFalse($config_for_package_b->shouldOnlyUpdateSecurityUpdates()); + + $config_for_other = $config->getConfigForPackage('vendor/package-c'); + self::assertTrue($config_for_other->shouldOnlyUpdateSecurityUpdates()); + } + + public function testRuleOverridesGlobalConfigForPackage() + { + $config_data = (object) [ + 'security_updates_only' => 1, + 'rules' => [ + (object) [ + 'name' => 'Override security_updates_only', + 'matchRules' => [ + (object) ['type' => 'names', 'values' => ['vendor/*']], + ], + 'config' => (object) [ + 'security_updates_only' => 0, + ], + ], + ], + ]; + $config = Config::createFromViolinistConfig($config_data); + + $config_for_package = $config->getConfigForPackage('vendor/package-a'); + self::assertFalse($config_for_package->shouldOnlyUpdateSecurityUpdates()); + } + + public function testRuleOverridesExplicitGlobalValueForPackage() + { + $config_data = (object) [ + 'update_dev_dependencies' => 1, + 'rules' => [ + (object) [ + 'name' => 'Override update_dev_dependencies', + 'matchRules' => [ + (object) ['type' => 'names', 'values' => ['vendor/*']], + ], + 'config' => (object) [ + 'update_dev_dependencies' => 0, + ], + ], + ], + ]; + $config = Config::createFromViolinistConfig($config_data); + + $config_for_package = $config->getConfigForPackage('vendor/package-a'); + self::assertFalse($config_for_package->shouldUpdateDevDependencies()); + } + + public function testRuleOverridesGlobalBundledPackages() + { + $config_data = (object) [ + 'bundled_packages' => (object) ['psr/log' => ['symfony/console']], + 'rules' => [ + (object) [ + 'name' => 'Override bundled_packages', + 'matchRules' => [ + (object) ['type' => 'names', 'values' => ['psr/*']], + ], + 'config' => (object) [ + 'bundled_packages' => (object) ['psr/log' => ['other/package']], + ], + ], + ], + ]; + $config = Config::createFromViolinistConfig($config_data); + + $config_for_package = $config->getConfigForPackage('psr/log'); + self::assertEquals(['other/package'], $config_for_package->getBundledPackagesForPackage('psr/log')); + } + + public function testRuleOverridesGlobalSecurityUpdatesOnlyForSpecificPackage() + { + $config_data = (object) [ + 'security_updates_only' => 1, + 'rules' => [ + (object) [ + 'name' => 'PSR Log', + 'matchRules' => [ + (object) ['type' => 'names', 'values' => ['psr/log']], + ], + 'config' => (object) [ + 'security_updates_only' => 0, + ], + ], + ], + ]; + $config = Config::createFromViolinistConfig($config_data); + + $config_for_package = $config->getConfigForPackage('psr/log'); + self::assertFalse($config_for_package->shouldOnlyUpdateSecurityUpdates()); + self::assertTrue($config->shouldOnlyUpdateSecurityUpdates()); + self::assertTrue($config->getConfigForPackage('not/exist')->shouldOnlyUpdateSecurityUpdates()); + } + + public function testGetConfigForPackageDoesNotMarkUnsetOptionsAsSet() + { + // automerge_method is explicitly set to 'squash'; automerge_method_security + // is intentionally left unset so it should fall back to automerge_method. + $config_data = (object) [ + 'automerge_method' => 'squash', + 'rules' => [ + (object) [ + 'name' => 'Enable automerge for vendor packages', + 'matchRules' => [ + (object) ['type' => 'names', 'values' => ['vendor/*']], + ], + 'config' => (object) [ + 'automerge' => 1, + ], + ], + ], + ]; + $config = Config::createFromViolinistConfig($config_data); + + // Sanity: on the global config, automerge_method_security is unset, so + // getAutomergeMethod(true) must fall back to automerge_method ('squash'). + self::assertFalse($config->hasConfigForKey('automerge_method_security')); + self::assertEquals('squash', $config->getAutomergeMethod(true)); + + $config_for_package = $config->getConfigForPackage('vendor/package-a'); + + // After getConfigForPackage(), automerge_method_security was still never + // explicitly configured, so hasConfigForKey() must still return false and + // getAutomergeMethod(true) must still fall back to 'squash'. + self::assertFalse($config_for_package->hasConfigForKey('automerge_method_security')); + self::assertEquals('squash', $config_for_package->getAutomergeMethod(true)); + } + + public function testGetConfigForRuleObjectAndForPackageProduceSameBundledPackages() + { + $config_data = (object) [ + 'bundled_packages' => (object) ['psr/log' => ['symfony/console']], + 'rules' => [ + (object) [ + 'name' => 'Override bundled_packages', + 'matchRules' => [ + (object) ['type' => 'names', 'values' => ['psr/*']], + ], + 'config' => (object) [ + 'bundled_packages' => (object) ['psr/log' => ['other/package']], + ], + ], + ], + ]; + $config = Config::createFromViolinistConfig($config_data); + $rule = $config_data->rules[0]; + + $config_for_package = $config->getConfigForPackage('psr/log'); + $config_from_rule = $config->getConfigForRuleObject($rule); + + self::assertEquals( + $config_for_package->getBundledPackagesForPackage('psr/log'), + $config_from_rule->getBundledPackagesForPackage('psr/log') + ); + } + + public function testRuleConfigWithUnknownKeyIsIgnored() + { + $config = new Config(); + $rule = (object) [ + 'config' => (object) [ + 'this_key_does_not_exist' => 'some_value', + 'security_updates_only' => 1, + ], + ]; + $config_from_rule = $config->getConfigForRuleObject($rule); + self::assertFalse($config_from_rule->hasConfigForKey('this_key_does_not_exist')); + // Known keys are still applied. + self::assertTrue($config_from_rule->shouldOnlyUpdateSecurityUpdates()); + } + + public function testRuleConfigWithNestedRulesIsNotPropagated() + { + $config = new Config(); + $rule = (object) [ + 'config' => (object) [ + 'security_updates_only' => 1, + 'rules' => [ + (object) [ + 'config' => (object) ['automerge' => 1], + ], + ], + ], + ]; + $config_from_rule = $config->getConfigForRuleObject($rule); + // Nested rules must not leak into the package-specific config. + self::assertEmpty($config_from_rule->getRules()); + // Other keys in the rule config are still applied. + self::assertTrue($config_from_rule->shouldOnlyUpdateSecurityUpdates()); + } + + public function testGetConfigForPackageReturnsIndependentConfigWhenNoRuleMatches() + { + $config = Config::createFromViolinistConfig((object) [ + 'security_updates_only' => 0, + 'rules' => [ + (object) [ + 'matchRules' => [ + (object) ['type' => 'names', 'values' => ['vendor/specific-package']], + ], + 'config' => (object) ['security_updates_only' => 1], + ], + ], + ]); + + $config_for_unmatched = $config->getConfigForPackage('vendor/other-package'); + + // Must be an independent object, not the global config instance. + self::assertNotSame($config, $config_for_unmatched); + // Mutating the returned config must not affect the global config. + $config_for_unmatched->setConfig((object) ['security_updates_only' => 1]); + self::assertFalse($config->shouldOnlyUpdateSecurityUpdates()); } } diff --git a/tests/VersionThree/OverrideVisibilityTest.php b/tests/VersionThree/OverrideVisibilityTest.php index c689d76..c84c2a6 100644 --- a/tests/VersionThree/OverrideVisibilityTest.php +++ b/tests/VersionThree/OverrideVisibilityTest.php @@ -26,4 +26,29 @@ public function testInvalidExtendName() self::expectException(\RuntimeException::class); $readable = $config->getReadableChainForExtendName('invalid_key'); } + + public function testRuleOverridingExtendsKeyResetsProvenance() + { + // automerge_method = 'squash' originates from violinist-base-config.json via extends. + self::assertEquals('violinist-base-config.json', $this->config->getExtendNameForKey('automerge_method')); + + $this->config->setConfig((object) [ + 'rules' => [ + (object) [ + 'matchRules' => [ + (object) ['type' => 'names', 'values' => ['vendor/package-a']], + ], + 'config' => (object) ['automerge_method' => 'merge'], + ], + ], + ]); + + $config_for_package = $this->config->getConfigForPackage('vendor/package-a'); + + // The rule overrode automerge_method; the extend file must no longer + // be reported as its source on the package-specific config. + self::assertEquals('', $config_for_package->getExtendNameForKey('automerge_method')); + // The global config's provenance must be unaffected. + self::assertEquals('violinist-base-config.json', $this->config->getExtendNameForKey('automerge_method')); + } }