From 5109c768be8fff53304143ff2c9b639fb34a0afe Mon Sep 17 00:00:00 2001 From: Jake Hotson Date: Fri, 2 May 2025 01:25:54 +0100 Subject: [PATCH 1/4] [TASK] Add `RuleSet::removeMatchingRules()`, with deprecation Passing `string` or `null` to `removeRule()` is deprecated. The new method handles that functionality. Relates to #1247. --- CHANGELOG.md | 5 ++++ src/RuleSet/RuleSet.php | 51 +++++++++++++++++++++++------------------ 2 files changed, 34 insertions(+), 22 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f4b2fc5e..9c15d0e2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,8 @@ Please also have a look at our ### Added +- `RuleSet::removeMatchingRules()` method for implementing classes `AtRuleSet` + and `DeclarationBlock` (#1249) - Add Interface `CSSElement` (#1231) - Methods `getLineNumber` and `getColumnNumber` which return a nullable `int` for the following classes: @@ -46,6 +48,9 @@ Please also have a look at our ### Deprecated +- Passing a `string` or `null` to `RuleSet::removeRule()` is deprecated + (implementing classes are `AtRuleSet` and `DeclarationBlock`); + use `removeMatchingRules()` instead (#1249) - Passing a `Rule` to `RuleSet::getRules()` or `getRulesAssoc()` is deprecated, affecting the implementing classes `AtRuleSet` and `DeclarationBlock` (call e.g. `getRules($rule->getRule())` instead) (#1248) diff --git a/src/RuleSet/RuleSet.php b/src/RuleSet/RuleSet.php index d96787c5..2dd1d13b 100644 --- a/src/RuleSet/RuleSet.php +++ b/src/RuleSet/RuleSet.php @@ -212,18 +212,12 @@ public function getRulesAssoc($searchPattern = null): array } /** - * Removes a rule from this RuleSet. This accepts all the possible values that `getRules()` accepts. - * - * If given a Rule, it will only remove this particular rule (by identity). - * If given a name, it will remove all rules by that name. - * - * Note: this is different from pre-v.2.0 behaviour of PHP-CSS-Parser, where passing a Rule instance would - * remove all rules with the same name. To get the old behaviour, use `removeRule($rule->getRule())`. + * Removes a `Rule` from this `RuleSet` by identity. * * @param Rule|string|null $searchPattern - * pattern to remove. If null, all rules are removed. If the pattern ends in a dash, - * all rules starting with the pattern are removed as well as one matching the pattern with the dash - * excluded. Passing a Rule behaves matches by identity. + * `Rule` to remove. + * Passing a `string` or `null` is deprecated in version 8.9.0, and will no longer work from v9.0. + * Use `removeMatchingRules()` instead. */ public function removeRule($searchPattern): void { @@ -238,18 +232,31 @@ public function removeRule($searchPattern): void } } } else { - foreach ($this->rules as $propertyName => $rules) { - // Either no search rule is given or the search rule matches the found rule exactly - // or the search rule ends in “-” and the found rule starts with the search rule or equals it - // (without the trailing dash). - if ( - $searchPattern === null || $propertyName === $searchPattern - || (\strrpos($searchPattern, '-') === \strlen($searchPattern) - \strlen('-') - && (\strpos($propertyName, $searchPattern) === 0 - || $propertyName === \substr($searchPattern, 0, -1))) - ) { - unset($this->rules[$propertyName]); - } + $this->removeMatchingRules($searchPattern); + } + } + + /** + * Removes rules by property name or search pattern. + * + * @param string|null $searchPattern + * pattern to remove. If null, all rules are removed. If the pattern ends in a dash, + * all rules starting with the pattern are removed as well as one matching the pattern with the dash + * excluded. + */ + public function removeMatchingRules(?string $searchPattern): void + { + foreach ($this->rules as $propertyName => $rules) { + // Either no search rule is given or the search rule matches the found rule exactly + // or the search rule ends in “-” and the found rule starts with the search rule or equals it + // (without the trailing dash). + if ( + $searchPattern === null || $propertyName === $searchPattern + || (\strrpos($searchPattern, '-') === \strlen($searchPattern) - \strlen('-') + && (\strpos($propertyName, $searchPattern) === 0 + || $propertyName === \substr($searchPattern, 0, -1))) + ) { + unset($this->rules[$propertyName]); } } } From 0352420f9d55645f236616fe76df9ada79f4c8ef Mon Sep 17 00:00:00 2001 From: Jake Hotson Date: Fri, 2 May 2025 23:33:03 +0100 Subject: [PATCH 2/4] Clarify changelog 'Added' entry --- CHANGELOG.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9c15d0e2..0d3007ec 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,8 +10,8 @@ Please also have a look at our ### Added -- `RuleSet::removeMatchingRules()` method for implementing classes `AtRuleSet` - and `DeclarationBlock` (#1249) +- `RuleSet::removeMatchingRules()` method + (for the implementing classes `AtRuleSet` and `DeclarationBlock`) (#1249) - Add Interface `CSSElement` (#1231) - Methods `getLineNumber` and `getColumnNumber` which return a nullable `int` for the following classes: From b15a3ab9ea9ce8db6c8285af3c6640aadb69b5b3 Mon Sep 17 00:00:00 2001 From: Jake Hotson Date: Sat, 3 May 2025 00:20:57 +0100 Subject: [PATCH 3/4] Add `removeAllRules()` method --- CHANGELOG.md | 4 +++- src/RuleSet/RuleSet.php | 22 +++++++++++++++------- 2 files changed, 18 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0d3007ec..023117c6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,8 @@ Please also have a look at our - `RuleSet::removeMatchingRules()` method (for the implementing classes `AtRuleSet` and `DeclarationBlock`) (#1249) +- `RuleSet::removeAllRules()` method + (for the implementing classes `AtRuleSet` and `DeclarationBlock`) (#1249) - Add Interface `CSSElement` (#1231) - Methods `getLineNumber` and `getColumnNumber` which return a nullable `int` for the following classes: @@ -50,7 +52,7 @@ Please also have a look at our - Passing a `string` or `null` to `RuleSet::removeRule()` is deprecated (implementing classes are `AtRuleSet` and `DeclarationBlock`); - use `removeMatchingRules()` instead (#1249) + use `removeMatchingRules()` or `removeAllRules()` instead (#1249) - Passing a `Rule` to `RuleSet::getRules()` or `getRulesAssoc()` is deprecated, affecting the implementing classes `AtRuleSet` and `DeclarationBlock` (call e.g. `getRules($rule->getRule())` instead) (#1248) diff --git a/src/RuleSet/RuleSet.php b/src/RuleSet/RuleSet.php index 2dd1d13b..3ed5cf0f 100644 --- a/src/RuleSet/RuleSet.php +++ b/src/RuleSet/RuleSet.php @@ -217,7 +217,7 @@ public function getRulesAssoc($searchPattern = null): array * @param Rule|string|null $searchPattern * `Rule` to remove. * Passing a `string` or `null` is deprecated in version 8.9.0, and will no longer work from v9.0. - * Use `removeMatchingRules()` instead. + * Use `removeMatchingRules()` or `removeAllRules()` instead. */ public function removeRule($searchPattern): void { @@ -231,27 +231,30 @@ public function removeRule($searchPattern): void unset($this->rules[$nameOfPropertyToRemove][$key]); } } - } else { + } elseif ($searchPattern !== null) { $this->removeMatchingRules($searchPattern); + } else { + $this->removeAllRules(); } } /** * Removes rules by property name or search pattern. * - * @param string|null $searchPattern - * pattern to remove. If null, all rules are removed. If the pattern ends in a dash, + * @param string $searchPattern + * pattern to remove. + * If the pattern ends in a dash, * all rules starting with the pattern are removed as well as one matching the pattern with the dash * excluded. */ - public function removeMatchingRules(?string $searchPattern): void + public function removeMatchingRules(string $searchPattern): void { foreach ($this->rules as $propertyName => $rules) { - // Either no search rule is given or the search rule matches the found rule exactly + // Either the search rule matches the found rule exactly // or the search rule ends in “-” and the found rule starts with the search rule or equals it // (without the trailing dash). if ( - $searchPattern === null || $propertyName === $searchPattern + $propertyName === $searchPattern || (\strrpos($searchPattern, '-') === \strlen($searchPattern) - \strlen('-') && (\strpos($propertyName, $searchPattern) === 0 || $propertyName === \substr($searchPattern, 0, -1))) @@ -261,6 +264,11 @@ public function removeMatchingRules(?string $searchPattern): void } } + public function removeAllRules(): void + { + $this->rules = []; + } + protected function renderRules(OutputFormat $outputFormat): string { $result = ''; From eab36587bab8bd06b7134cd9d3648f58796d961a Mon Sep 17 00:00:00 2001 From: Jake Hotson Date: Sun, 4 May 2025 00:22:47 +0100 Subject: [PATCH 4/4] Add tests for the new methods For consistency with current nomenclature, `Rule` is used in the test method names, despite that they are now called 'declarations'. (Renaming to match the current spec is a separate project.) --- tests/Unit/RuleSet/RuleSetTest.php | 170 +++++++++++++++++++++++++++++ 1 file changed, 170 insertions(+) diff --git a/tests/Unit/RuleSet/RuleSetTest.php b/tests/Unit/RuleSet/RuleSetTest.php index 0b1fd816..774f37e7 100644 --- a/tests/Unit/RuleSet/RuleSetTest.php +++ b/tests/Unit/RuleSet/RuleSetTest.php @@ -7,6 +7,7 @@ use PHPUnit\Framework\TestCase; use Sabberworm\CSS\CSSElement; use Sabberworm\CSS\CSSList\CSSListItem; +use Sabberworm\CSS\Rule\Rule; use Sabberworm\CSS\Tests\Unit\RuleSet\Fixtures\ConcreteRuleSet; /** @@ -39,4 +40,173 @@ public function implementsCSSListItem(): void { self::assertInstanceOf(CSSListItem::class, $this->subject); } + + /** + * @return array, 1: string, 2: list}> + */ + public static function provideRulesAndPropertyNameToRemoveAndExpectedRemainingPropertyNames(): array + { + return [ + 'removing single rule' => [ + [new Rule('color')], + 'color', + [], + ], + 'removing first rule' => [ + [new Rule('color'), new Rule('display')], + 'color', + ['display'], + ], + 'removing last rule' => [ + [new Rule('color'), new Rule('display')], + 'display', + ['color'], + ], + 'removing middle rule' => [ + [new Rule('color'), new Rule('display'), new Rule('width')], + 'display', + ['color', 'width'], + ], + 'removing multiple rules' => [ + [new Rule('color'), new Rule('color')], + 'color', + [], + ], + 'removing multiple rules with another kept' => [ + [new Rule('color'), new Rule('color'), new Rule('display')], + 'color', + ['display'], + ], + 'removing nonexistent rule from empty list' => [ + [], + 'color', + [], + ], + 'removing nonexistent rule from nonempty list' => [ + [new Rule('color'), new Rule('display')], + 'width', + ['color', 'display'], + ], + ]; + } + + /** + * @test + * + * @param list $rules + * @param list $expectedRemainingPropertyNames + * + * @dataProvider provideRulesAndPropertyNameToRemoveAndExpectedRemainingPropertyNames + */ + public function removeMatchingRulesRemovesRulesByPropertyNameAndKeepsOthers( + array $rules, + string $propertyName, + array $expectedRemainingPropertyNames + ): void { + $this->subject->setRules($rules); + + $this->subject->removeMatchingRules($propertyName); + + $remainingRules = $this->subject->getRulesAssoc(); + self::assertArrayNotHasKey($propertyName, $remainingRules); + foreach ($expectedRemainingPropertyNames as $expectedPropertyName) { + self::assertArrayHasKey($expectedPropertyName, $remainingRules); + } + } + + /** + * @return array, 1: string, 2: list}> + */ + public static function provideRulesAndPropertyNamePrefixToRemoveAndExpectedRemainingPropertyNames(): array + { + return [ + 'removing shorthand rule' => [ + [new Rule('font')], + 'font', + [], + ], + 'removing longhand rule' => [ + [new Rule('font-size')], + 'font', + [], + ], + 'removing shorthand and longhand rule' => [ + [new Rule('font'), new Rule('font-size')], + 'font', + [], + ], + 'removing shorthand rule with another kept' => [ + [new Rule('font'), new Rule('color')], + 'font', + ['color'], + ], + 'removing longhand rule with another kept' => [ + [new Rule('font-size'), new Rule('color')], + 'font', + ['color'], + ], + 'keeping other rules whose property names begin with the same characters' => [ + [new Rule('contain'), new Rule('container'), new Rule('container-type')], + 'contain', + ['container', 'container-type'], + ], + ]; + } + + /** + * @test + * + * @param list $rules + * @param list $expectedRemainingPropertyNames + * + * @dataProvider provideRulesAndPropertyNamePrefixToRemoveAndExpectedRemainingPropertyNames + */ + public function removeMatchingRulesRemovesRulesByPropertyNamePrefixAndKeepsOthers( + array $rules, + string $propertyNamePrefix, + array $expectedRemainingPropertyNames + ): void { + $propertyNamePrefixWithHyphen = $propertyNamePrefix . '-'; + $this->subject->setRules($rules); + + $this->subject->removeMatchingRules($propertyNamePrefixWithHyphen); + + $remainingRules = $this->subject->getRulesAssoc(); + self::assertArrayNotHasKey($propertyNamePrefix, $remainingRules); + foreach (\array_keys($remainingRules) as $remainingPropertyName) { + self::assertStringStartsNotWith($propertyNamePrefixWithHyphen, $remainingPropertyName); + } + foreach ($expectedRemainingPropertyNames as $expectedPropertyName) { + self::assertArrayHasKey($expectedPropertyName, $remainingRules); + } + } + + /** + * @return array}> + */ + public static function provideRulesToRemove(): array + { + return [ + 'no rules' => [[]], + 'one rule' => [[new Rule('color')]], + 'two rules for different properties' => [[new Rule('color'), new Rule('display')]], + 'two rules for the same property' => [[new Rule('color'), new Rule('color')]], + ]; + } + + /** + * @test + * + * @param list $rules + * + * @dataProvider provideRulesToRemove + */ + public function removeAllRulesRemovesAllRules(array $rules): void + { + $this->subject->setRules($rules); + + $this->subject->removeAllRules(); + + self::assertSame([], $this->subject->getRules()); + } }