From 71d719fd3231a09f3235ce6dced7ae8220e64f57 Mon Sep 17 00:00:00 2001 From: Jake Hotson Date: Mon, 24 Mar 2025 02:22:32 +0000 Subject: [PATCH 1/2] [TASK] Add (and use) a `CSSListItem` type This allows a single type to be used for the contents of a `CSSList`, instead of a long list of orred types, and helps with static analysis. Various `assertInstanceOf()` tests are added to the test cases to confirm that the list items are of the type expected. Some `implements` and `exetends` lists are now alphabetically sorted. --- config/phpstan-baseline.neon | 12 ++++---- src/CSSList/CSSList.php | 39 ++++++++++---------------- src/CSSList/CSSListItem.php | 18 ++++++++++++ src/Property/AtRule.php | 3 +- src/RuleSet/RuleSet.php | 3 +- tests/CSSList/AtRuleBlockListTest.php | 3 ++ tests/ParserTest.php | 36 ++++++++++++++++++++++++ tests/RuleSet/DeclarationBlockTest.php | 6 +++- 8 files changed, 87 insertions(+), 33 deletions(-) create mode 100644 src/CSSList/CSSListItem.php diff --git a/config/phpstan-baseline.neon b/config/phpstan-baseline.neon index 47b197a1..007667a5 100644 --- a/config/phpstan-baseline.neon +++ b/config/phpstan-baseline.neon @@ -18,6 +18,12 @@ parameters: count: 1 path: ../src/CSSList/CSSList.php + - + message: '#^Parameters should have "Sabberworm\\CSS\\CSSList\\CSSListItem\|array" types as the only types passed to this method$#' + identifier: typePerfect.narrowPublicClassMethodParamType + count: 1 + path: ../src/CSSList/CSSList.php + - message: '#^Short ternary operator is not allowed\. Use null coalesce operator if applicable or consider using long ternary\.$#' identifier: ternary.shortNotAllowed @@ -54,12 +60,6 @@ parameters: count: 2 path: ../src/RuleSet/RuleSet.php - - - message: '#^Parameters should have "Sabberworm\\CSS\\Rule\\Rule" types as the only types passed to this method$#' - identifier: typePerfect.narrowPublicClassMethodParamType - count: 1 - path: ../src/RuleSet/RuleSet.php - - message: '#^Parameters should have "string" types as the only types passed to this method$#' identifier: typePerfect.narrowPublicClassMethodParamType diff --git a/src/CSSList/CSSList.php b/src/CSSList/CSSList.php index 3cd76206..83ab9735 100644 --- a/src/CSSList/CSSList.php +++ b/src/CSSList/CSSList.php @@ -31,7 +31,7 @@ * * It can also contain `Import` and `Charset` objects stemming from at-rules. */ -abstract class CSSList implements Renderable, Commentable +abstract class CSSList implements Commentable, CSSListItem, Renderable { /** * @var list @@ -41,7 +41,7 @@ abstract class CSSList implements Renderable, Commentable protected $comments = []; /** - * @var array, RuleSet|CSSList|Import|Charset> + * @var array, CSSListItem> * * @internal since 8.8.0 */ @@ -105,7 +105,7 @@ public static function parseList(ParserState $parserState, CSSList $list): void } /** - * @return AtRuleBlockList|KeyFrame|Charset|CSSNamespace|Import|AtRuleSet|DeclarationBlock|false|null + * @return CSSListItem|false|null * If `null` is returned, it means the end of the list has been reached. * If `false` is returned, it means an invalid item has been encountered, * but parsing of the next item should proceed. @@ -156,13 +156,11 @@ private static function parseListItem(ParserState $parserState, CSSList $list) } /** - * @return AtRuleBlockList|KeyFrame|Charset|CSSNamespace|Import|AtRuleSet|null - * * @throws SourceException * @throws UnexpectedTokenException * @throws UnexpectedEOFException */ - private static function parseAtRule(ParserState $parserState) + private static function parseAtRule(ParserState $parserState): ?CSSListItem { $parserState->consume('@'); $identifier = $parserState->parseIdentifier(); @@ -265,20 +263,16 @@ public function getLineNo(): int /** * Prepends an item to the list of contents. - * - * @param RuleSet|CSSList|Import|Charset $item */ - public function prepend($item): void + public function prepend(CSSListItem $item): void { \array_unshift($this->contents, $item); } /** * Appends an item to the list of contents. - * - * @param RuleSet|CSSList|Import|Charset $item */ - public function append($item): void + public function append(CSSListItem $item): void { $this->contents[] = $item; } @@ -286,7 +280,7 @@ public function append($item): void /** * Splices the list of contents. * - * @param array $replacement + * @param array $replacement */ public function splice(int $offset, ?int $length = null, ?array $replacement = null): void { @@ -296,11 +290,8 @@ public function splice(int $offset, ?int $length = null, ?array $replacement = n /** * Inserts an item in the CSS list before its sibling. If the desired sibling cannot be found, * the item is appended at the end. - * - * @param RuleSet|CSSList|Import|Charset $item - * @param RuleSet|CSSList|Import|Charset $sibling */ - public function insertBefore($item, $sibling): void + public function insertBefore(CSSListItem $item, CSSListItem $sibling): void { if (\in_array($sibling, $this->contents, true)) { $this->replace($sibling, [$item, $sibling]); @@ -312,13 +303,13 @@ public function insertBefore($item, $sibling): void /** * Removes an item from the CSS list. * - * @param RuleSet|Import|Charset|CSSList $itemToRemove + * @param CSSListItem $itemToRemove * May be a `RuleSet` (most likely a `DeclarationBlock`), an `Import`, * a `Charset` or another `CSSList` (most likely a `MediaQuery`) * * @return bool whether the item was removed */ - public function remove($itemToRemove): bool + public function remove(CSSListItem $itemToRemove): bool { $key = \array_search($itemToRemove, $this->contents, true); if ($key !== false) { @@ -332,12 +323,12 @@ public function remove($itemToRemove): bool /** * Replaces an item from the CSS list. * - * @param RuleSet|Import|Charset|CSSList $oldItem + * @param CSSListItem $oldItem * May be a `RuleSet` (most likely a `DeclarationBlock`), an `Import`, a `Charset` * or another `CSSList` (most likely a `MediaQuery`) - * @param RuleSet|Import|Charset|CSSList|array $newItem + * @param CSSListItem|array $newItem */ - public function replace($oldItem, $newItem): bool + public function replace(CSSListItem $oldItem, $newItem): bool { $key = \array_search($oldItem, $this->contents, true); if ($key !== false) { @@ -353,7 +344,7 @@ public function replace($oldItem, $newItem): bool } /** - * @param array $contents + * @param array $contents */ public function setContents(array $contents): void { @@ -444,7 +435,7 @@ abstract public function isRootList(): bool; /** * Returns the stored items. * - * @return array, RuleSet|Import|Charset|CSSList> + * @return array, CSSListItem> */ public function getContents(): array { diff --git a/src/CSSList/CSSListItem.php b/src/CSSList/CSSListItem.php new file mode 100644 index 00000000..3cf2509b --- /dev/null +++ b/src/CSSList/CSSListItem.php @@ -0,0 +1,18 @@ +parse()->getContents(); $atRuleBlockList = $contents[0]; + self::assertInstanceOf(AtRuleBlockList::class, $atRuleBlockList); self::assertSame('media', $atRuleBlockList->atRuleName()); } @@ -73,6 +75,7 @@ public function parsesArgumentsOfMediaQueries(string $css): void $contents = (new Parser($css))->parse()->getContents(); $atRuleBlockList = $contents[0]; + self::assertInstanceOf(AtRuleBlockList::class, $atRuleBlockList); self::assertSame('(min-width: 768px)', $atRuleBlockList->atRuleArgs()); } diff --git a/tests/ParserTest.php b/tests/ParserTest.php index f2608fb3..7d1b528c 100644 --- a/tests/ParserTest.php +++ b/tests/ParserTest.php @@ -5,6 +5,8 @@ namespace Sabberworm\CSS\Tests; use PHPUnit\Framework\TestCase; +use Sabberworm\CSS\Comment\Commentable; +use Sabberworm\CSS\CSSList\CSSList; use Sabberworm\CSS\CSSList\Document; use Sabberworm\CSS\CSSList\KeyFrame; use Sabberworm\CSS\OutputFormat; @@ -973,9 +975,30 @@ public function lineNumbersParsing(): void $actual = []; foreach ($document->getContents() as $contentItem) { + // PHPStan can see what `assertInstanceOf()` does, + // but does not understand `LogicalOr` with multiple `IsIntanceOf` constraints. + // So a more explicit type check is required. + // TODO: tidy this up when an interface with `getLineNo()` is added. + if ( + !$contentItem instanceof Charset && + !$contentItem instanceof CSSList && + !$contentItem instanceof CSSNamespace && + !$contentItem instanceof Import && + !$contentItem instanceof RuleSet + ) { + self::fail('Content item is not of an expected type. It\'s a `' . \get_class($contentItem) . '`.'); + } $actual[$contentItem->getLineNo()] = [\get_class($contentItem)]; if ($contentItem instanceof KeyFrame) { foreach ($contentItem->getContents() as $block) { + if ( + !$block instanceof CSSList && + !$block instanceof RuleSet + ) { + self::fail( + 'KeyFrame content item is not of an expected type. It\'s a `' . \get_class($block) . '`.' + ); + } $actual[$contentItem->getLineNo()][] = $block->getLineNo(); } } @@ -1037,6 +1060,7 @@ public function commentExtracting(): void $nodes = $document->getContents(); // Import property. + self::assertInstanceOf(Commentable::class, $nodes[0]); $importComments = $nodes[0]->getComments(); self::assertCount(2, $importComments); self::assertSame("*\n * Comments\n ", $importComments[0]->getComment()); @@ -1044,6 +1068,7 @@ public function commentExtracting(): void // Declaration block. $fooBarBlock = $nodes[1]; + self::assertInstanceOf(Commentable::class, $fooBarBlock); $fooBarBlockComments = $fooBarBlock->getComments(); // TODO Support comments in selectors. // $this->assertCount(2, $fooBarBlockComments); @@ -1051,6 +1076,7 @@ public function commentExtracting(): void // $this->assertSame("* Number 5 *", $fooBarBlockComments[1]->getComment()); // Declaration rules. + self::assertInstanceOf(RuleSet::class, $fooBarBlock); $fooBarRules = $fooBarBlock->getRules(); $fooBarRule = $fooBarRules[0]; $fooBarRuleComments = $fooBarRule->getComments(); @@ -1058,16 +1084,20 @@ public function commentExtracting(): void self::assertSame(' Number 6 ', $fooBarRuleComments[0]->getComment()); // Media property. + self::assertInstanceOf(Commentable::class, $nodes[2]); $mediaComments = $nodes[2]->getComments(); self::assertCount(0, $mediaComments); // Media children. + self::assertInstanceOf(CSSList::class, $nodes[2]); $mediaRules = $nodes[2]->getContents(); + self::assertInstanceOf(Commentable::class, $mediaRules[0]); $fooBarComments = $mediaRules[0]->getComments(); self::assertCount(1, $fooBarComments); self::assertSame('* Number 10 *', $fooBarComments[0]->getComment()); // Media -> declaration -> rule. + self::assertInstanceOf(RuleSet::class, $mediaRules[0]); $fooBarRules = $mediaRules[0]->getRules(); $fooBarChildComments = $fooBarRules[0]->getComments(); self::assertCount(1, $fooBarChildComments); @@ -1083,6 +1113,7 @@ public function flatCommentExtractingOneComment(): void $document = $parser->parse(); $contents = $document->getContents(); + self::assertInstanceOf(RuleSet::class, $contents[0]); $divRules = $contents[0]->getRules(); $comments = $divRules[0]->getComments(); @@ -1099,6 +1130,7 @@ public function flatCommentExtractingTwoConjoinedCommentsForOneRule(): void $document = $parser->parse(); $contents = $document->getContents(); + self::assertInstanceOf(RuleSet::class, $contents[0]); $divRules = $contents[0]->getRules(); $comments = $divRules[0]->getComments(); @@ -1116,6 +1148,7 @@ public function flatCommentExtractingTwoSpaceSeparatedCommentsForOneRule(): void $document = $parser->parse(); $contents = $document->getContents(); + self::assertInstanceOf(RuleSet::class, $contents[0]); $divRules = $contents[0]->getRules(); $comments = $divRules[0]->getComments(); @@ -1133,6 +1166,7 @@ public function flatCommentExtractingCommentsForTwoRules(): void $document = $parser->parse(); $contents = $document->getContents(); + self::assertInstanceOf(RuleSet::class, $contents[0]); $divRules = $contents[0]->getRules(); $rule1Comments = $divRules[0]->getComments(); $rule2Comments = $divRules[1]->getComments(); @@ -1151,6 +1185,7 @@ public function topLevelCommentExtracting(): void $parser = new Parser('/*Find Me!*/div {left:10px; text-align:left;}'); $document = $parser->parse(); $contents = $document->getContents(); + self::assertInstanceOf(Commentable::class, $contents[0]); $comments = $contents[0]->getComments(); self::assertCount(1, $comments); self::assertSame('Find Me!', $comments[0]->getComment()); @@ -1216,6 +1251,7 @@ public function escapedSpecialCaseTokens(): void { $document = self::parsedStructureForFile('escaped-tokens'); $contents = $document->getContents(); + self::assertInstanceOf(RuleSet::class, $contents[0]); $rules = $contents[0]->getRules(); $urlRule = $rules[0]; $calcRule = $rules[1]; diff --git a/tests/RuleSet/DeclarationBlockTest.php b/tests/RuleSet/DeclarationBlockTest.php index 128048e8..5aaf0662 100644 --- a/tests/RuleSet/DeclarationBlockTest.php +++ b/tests/RuleSet/DeclarationBlockTest.php @@ -8,6 +8,7 @@ use Sabberworm\CSS\OutputFormat; use Sabberworm\CSS\Parser; use Sabberworm\CSS\Rule\Rule; +use Sabberworm\CSS\RuleSet\RuleSet; use Sabberworm\CSS\Settings as ParserSettings; use Sabberworm\CSS\Value\Size; @@ -30,8 +31,9 @@ public function overrideRules(): void $contents = $document->getContents(); $wrapper = $contents[0]; + self::assertInstanceOf(RuleSet::class, $wrapper); self::assertCount(2, $wrapper->getRules()); - $contents[0]->setRules([$rule]); + $wrapper->setRules([$rule]); $rules = $wrapper->getRules(); self::assertCount(1, $rules); @@ -50,6 +52,8 @@ public function ruleInsertion(): void $contents = $document->getContents(); $wrapper = $contents[0]; + self::assertInstanceOf(RuleSet::class, $wrapper); + $leftRules = $wrapper->getRules('left'); self::assertCount(1, $leftRules); $firstLeftRule = $leftRules[0]; From 691cfb518d5bf64b3c53882f3505d45a3ef17fdc Mon Sep 17 00:00:00 2001 From: Jake Hotson Date: Tue, 25 Mar 2025 01:02:14 +0000 Subject: [PATCH 2/2] Don't implement interfaces extended by another that is also implemented PHP<7.4 does not allow this. Instead, for clarity, add a DocBlock comment stating which additional interfaces should be implemented that are not explicitly listed in the `implements` section. When our minimum PHP version becomes 7.4 or above, we can revisit this. --- src/CSSList/CSSList.php | 5 ++++- src/Property/AtRule.php | 6 +++++- src/RuleSet/RuleSet.php | 5 ++++- 3 files changed, 13 insertions(+), 3 deletions(-) diff --git a/src/CSSList/CSSList.php b/src/CSSList/CSSList.php index 83ab9735..e6cb900f 100644 --- a/src/CSSList/CSSList.php +++ b/src/CSSList/CSSList.php @@ -30,8 +30,11 @@ * `RuleSet`s as well as other `CSSList` objects. * * It can also contain `Import` and `Charset` objects stemming from at-rules. + * + * Note that `CSSListItem` extends both `Commentable` and `Renderable`, + * so those interfaces must also be implemented by concrete subclasses. */ -abstract class CSSList implements Commentable, CSSListItem, Renderable +abstract class CSSList implements CSSListItem { /** * @var list diff --git a/src/Property/AtRule.php b/src/Property/AtRule.php index d09b8a6b..8b0d41a1 100644 --- a/src/Property/AtRule.php +++ b/src/Property/AtRule.php @@ -8,7 +8,11 @@ use Sabberworm\CSS\CSSList\CSSListItem; use Sabberworm\CSS\Renderable; -interface AtRule extends Commentable, CSSListItem, Renderable +/** + * Note that `CSSListItem` extends both `Commentable` and `Renderable`, + * so concrete classes implementing this interface must also implement those. + */ +interface AtRule extends CSSListItem { /** * Since there are more set rules than block rules, diff --git a/src/RuleSet/RuleSet.php b/src/RuleSet/RuleSet.php index f749de69..871aa757 100644 --- a/src/RuleSet/RuleSet.php +++ b/src/RuleSet/RuleSet.php @@ -22,8 +22,11 @@ * * If you want to manipulate a `RuleSet`, use the methods `addRule(Rule $rule)`, `getRules()` and `removeRule($rule)` * (which accepts either a `Rule` or a rule name; optionally suffixed by a dash to remove all related rules). + * + * Note that `CSSListItem` extends both `Commentable` and `Renderable`, + * so those interfaces must also be implemented by concrete subclasses. */ -abstract class RuleSet implements Commentable, CSSListItem, Renderable +abstract class RuleSet implements CSSListItem { /** * the rules in this rule set, using the property name as the key,