diff --git a/CHANGELOG.md b/CHANGELOG.md index d500e668..4c11c756 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ This project adheres to [Semantic Versioning](https://semver.org/). ### Added ### Changed +- Throw exception with invalid CSS in debug mode (#1142) - Only support up to 69 atomic expressions in a selector (#1113) - Require `sabberworm/php-css-parser:^8.4.0` (#1134) - Upgrade to PHPUnit 9 (#1112) diff --git a/src/Css/CssDocument.php b/src/Css/CssDocument.php index 145510bd..4639d585 100644 --- a/src/Css/CssDocument.php +++ b/src/Css/CssDocument.php @@ -13,6 +13,7 @@ use Sabberworm\CSS\Renderable as CssRenderable; use Sabberworm\CSS\RuleSet\DeclarationBlock as CssDeclarationBlock; use Sabberworm\CSS\RuleSet\RuleSet as CssRuleSet; +use Sabberworm\CSS\Settings as ParserSettings; /** * Parses and stores a CSS document from a string of CSS, and provides methods to obtain the CSS in parts or as data @@ -37,10 +38,30 @@ class CssDocument /** * @param string $css + * @param bool $debug + * If this is `true`, an exception will be thrown if invalid CSS is encountered. + * Otherwise the parser will try to do the best it can. */ - public function __construct(string $css) + public function __construct(string $css, bool $debug) { - $this->sabberwormCssDocument = (new CssParser($css))->parse(); + // CSS Parser currently throws exception with nested at-rules (like `@media`) in strict parsing mode + $parserSettings = ParserSettings::create()->withLenientParsing(!$debug || $this->hasNestedAtRule($css)); + + // CSS Parser currently throws exception with non-empty whitespace-only CSS in strict parsing mode, so `trim()` + // @see https://github.com/sabberworm/PHP-CSS-Parser/issues/349 + $this->sabberwormCssDocument = (new CssParser(\trim($css), $parserSettings))->parse(); + } + + /** + * Tests if a string of CSS appears to contain an at-rule with nested rules + * (`@media`, `@supports`, `@keyframes`, `@document`, + * the latter two additionally with vendor prefixes that may commonly be used). + * + * @see https://github.com/sabberworm/PHP-CSS-Parser/issues/127 + */ + private function hasNestedAtRule(string $css): bool + { + return \preg_match('/@(?:media|supports|(?:-webkit-|-moz-|-ms-|-o-)?+(keyframes|document))\\b/', $css) === 1; } /** diff --git a/src/CssInliner.php b/src/CssInliner.php index bf0b9118..0e071292 100644 --- a/src/CssInliner.php +++ b/src/CssInliner.php @@ -179,7 +179,7 @@ public function inlineCss(string $css = ''): self if ($this->isStyleBlocksParsingEnabled) { $combinedCss .= $this->getCssFromAllStyleNodes(); } - $parsedCss = new CssDocument($combinedCss); + $parsedCss = new CssDocument($combinedCss, $this->debug); $excludedNodes = $this->getNodesToExclude(); $cssRules = $this->collateCssRules($parsedCss); diff --git a/tests/Unit/Css/CssDocumentTest.php b/tests/Unit/Css/CssDocumentTest.php index 75f91add..96688279 100644 --- a/tests/Unit/Css/CssDocumentTest.php +++ b/tests/Unit/Css/CssDocumentTest.php @@ -7,6 +7,8 @@ use Pelago\Emogrifier\Css\CssDocument; use Pelago\Emogrifier\Tests\Support\Traits\AssertCss; use PHPUnit\Framework\TestCase; +use Sabberworm\CSS\Parsing\UnexpectedEOFException; +use Sabberworm\CSS\Parsing\UnexpectedTokenException; /** * @covers \Pelago\Emogrifier\Css\CssDocument @@ -22,6 +24,11 @@ final class CssDocumentTest extends TestCase . ' font-family: "Foo Sans";' . "\n" . ' src: url("/foo-sans.woff2") format("woff2");' . "\n}"; + private function createDebugSubject(string $css): CssDocument + { + return new CssDocument($css, true); + } + /** * @test * @@ -33,7 +40,7 @@ final class CssDocumentTest extends TestCase public function parsesSelector(string $selector): void { $css = $selector . '{ color: green; }'; - $subject = new CssDocument($css); + $subject = $this->createDebugSubject($css); $result = $subject->getStyleRulesData([]); @@ -47,7 +54,7 @@ public function parsesSelector(string $selector): void public function canParseMultipleSelectors(): void { $css = 'h1, h2 { color: green; }'; - $subject = new CssDocument($css); + $subject = $this->createDebugSubject($css); $result = $subject->getStyleRulesData([]); @@ -115,7 +122,7 @@ public function provideSelectorWithVariedWhitespace(): array public function parsesDeclarations(string $declarations): void { $css = 'p {' . $declarations . '}'; - $subject = new CssDocument($css); + $subject = $this->createDebugSubject($css); $result = $subject->getStyleRulesData([]); @@ -177,7 +184,7 @@ public function parsesAtMediaRule(string $mediaQuery): void { $atMediaAndQuery = '@media ' . $mediaQuery; $css = $atMediaAndQuery . ' { p { color: green; } }'; - $subject = new CssDocument($css); + $subject = $this->createDebugSubject($css); $result = $subject->getStyleRulesData(['screen']); @@ -218,7 +225,7 @@ public function parsesAtMediaRuleWithVariedWhitespace( $atMediaAndQuery = '@media' . $whitespaceAfterAtMedia . 'screen'; $css = $atMediaAndQuery . $optionalWhitespaceWithinRule . '{' . $optionalWhitespaceWithinRule . 'p { color: green; }' . $optionalWhitespaceWithinRule . '}'; - $subject = new CssDocument($css); + $subject = $this->createDebugSubject($css); $result = $subject->getStyleRulesData(['screen']); @@ -252,7 +259,7 @@ public function provideVariedWhitespaceForAtMediaRule(): array */ public function discardsMediaRuleWithTypeNotInAllowlist(string $mediaQuery): void { - $subject = new CssDocument('@media ' . $mediaQuery . ' { p { color: red; } }'); + $subject = $this->createDebugSubject('@media ' . $mediaQuery . ' { p { color: red; } }'); $result = $subject->getStyleRulesData(['screen']); @@ -296,7 +303,9 @@ public function provideMediaQueryWithTvTypeAndVariedWhitespace(): array */ public function parsesMultipleStyleRulesWithOtherCssBetween(string $cssBetween): void { - $subject = new CssDocument('p { color: green; }' . $cssBetween . '@media screen { h1 { color: green; } }'); + $subject = $this->createDebugSubject( + 'p { color: green; }' . $cssBetween . '@media screen { h1 { color: green; } }' + ); $result = $subject->getStyleRulesData(['screen']); @@ -314,7 +323,7 @@ public function parsesMultipleStyleRulesWithOtherCssBetween(string $cssBetween): */ public function parsesMultipleStyleRulesWithOtherCssBefore(string $cssBefore): void { - $subject = new CssDocument( + $subject = $this->createDebugSubject( $cssBefore . 'p { color: green; } @media screen { h1 { color: green; } }' ); @@ -362,7 +371,7 @@ public function provideCssThatMustPrecedeStyleRules(): array */ public function rendersValidNonConditionalAtRule(string $atRuleCss, string $cssBefore = ''): void { - $subject = new CssDocument($cssBefore . $atRuleCss); + $subject = $this->createDebugSubject($cssBefore . $atRuleCss); $result = $subject->renderNonConditionalAtRules(); @@ -403,7 +412,7 @@ public function provideValidNonConditionalAtRule(): array */ public function rendersMultipleNonConditionalAtRules(string $cssBetween): void { - $subject = new CssDocument('@import "foo.css";' . $cssBetween . self::VALID_AT_FONT_FACE_RULE); + $subject = $this->createDebugSubject('@import "foo.css";' . $cssBetween . self::VALID_AT_FONT_FACE_RULE); $result = $subject->renderNonConditionalAtRules(); @@ -432,24 +441,56 @@ public function provideCssWithoutNonConditionalAtRules(): array /** * @test * - * @param string $css + * @dataProvider provideValidAtCharsetRule + * @dataProvider provideInvalidAtCharsetRuleWhichCausesException + * @dataProvider provideInvalidAtCharsetRuleWhichDoesNotCauseException + */ + public function discardsValidOrInvalidAtCharsetRuleNotInDebugMode(string $css): void + { + $subject = new CssDocument($css, false); + + $result = $subject->renderNonConditionalAtRules(); + + self::assertSame('', $result); + } + + /** + * @test * - * @dataProvider provideValidAtCharsetRules - * @dataProvider provideInvalidAtCharsetRules + * @dataProvider provideValidAtCharsetRule */ - public function discardsValidOrInvalidAtCharsetRule(string $css): void + public function discardsValidAtCharsetRuleInDebugMode(string $css): void { - $subject = new CssDocument($css); + $subject = $this->createDebugSubject($css); $result = $subject->renderNonConditionalAtRules(); self::assertSame('', $result); } + /** + * @test + * + * @dataProvider provideInvalidAtCharsetRuleWhichCausesException + * + * Invalid `@charset` rules which do not currently cause an exception not yet tested. + */ + public function throwsExceptionForInvalidAtCharsetRuleInDebugMode(string $css): void + { + $this->expectException(UnexpectedEOFException::class); + + $this->createDebugSubject($css); + + self::markTestSkipped( + 'This test is disabled as currently the CSS parser does not throw an exception in all cases.' + . ' Discarding of invalid rules in non-debug mode is already covered by other tests.' + ); + } + /** * @return array */ - public function provideValidAtCharsetRules(): array + public function provideValidAtCharsetRule(): array { return [ 'UTF-8' => ['@charset "UTF-8";'], @@ -460,26 +501,33 @@ public function provideValidAtCharsetRules(): array /** * @return array */ - public function provideInvalidAtCharsetRules(): array + public function provideInvalidAtCharsetRuleWhichCausesException(): array + { + return [ + 'with unquoted value' => ['@charset UTF-8;'], + ]; + } + + /** + * @return array + */ + public function provideInvalidAtCharsetRuleWhichDoesNotCauseException(): array { return [ 'with uppercase identifier' => ['@CHARSET "UTF-8";'], 'with extra space' => ['@charset "UTF-8";'], - 'with unquoted value' => ['@charset UTF-8;'], ]; } /** * @test * - * @param string $atRuleCss - * @param string $cssBefore - * - * @dataProvider provideInvalidNonConditionalAtRule + * @dataProvider provideInvalidNonConditionalAtRuleWhichCausesException + * @dataProvider provideInvalidNonConditionalAtRuleWhichDoesNotCauseException */ - public function notRendersInvalidNonConditionalAtRule(string $atRuleCss, string $cssBefore = ''): void + public function discardsInvalidNonConditionalAtRuleNotInDebugMode(string $atRuleCss, string $cssBefore = ''): void { - $subject = new CssDocument($cssBefore . $atRuleCss); + $subject = new CssDocument($cssBefore . $atRuleCss, false); $result = $subject->renderNonConditionalAtRules(); @@ -488,10 +536,37 @@ public function notRendersInvalidNonConditionalAtRule(string $atRuleCss, string self::assertStringNotContainsString($atAndRuleName, $result); } + /** + * @test + * + * @dataProvider provideInvalidNonConditionalAtRuleWhichCausesException + * + * Invalid non-conditional at-rules which do not currently cause an exception not yet tested. + */ + public function throwsExceptionForInvalidNonConditionalAtRuleInDebugMode( + string $atRuleCss, + string $cssBefore = '' + ): void { + $this->expectException(UnexpectedTokenException::class); + + $this->createDebugSubject($cssBefore . $atRuleCss); + } + + /** + * @return array> + */ + public function provideInvalidNonConditionalAtRuleWhichCausesException(): array + { + return [ + '`@charset` after style rule' => ['@charset "UTF-8";', 'p { color: red; }'], + '`@charset` after `@import` rule' => ['@charset "UTF-8";', '@import "foo.css";'], + ]; + } + /** * @return array> */ - public function provideInvalidNonConditionalAtRule(): array + public function provideInvalidNonConditionalAtRuleWhichDoesNotCauseException(): array { return [ '`@font-face` without `font-family`' => [' @@ -504,8 +579,6 @@ public function provideInvalidNonConditionalAtRule(): array font-family: "Foo Sans"; } '], - '`@charset` after style rule' => ['@charset "UTF-8";', 'p { color: red; }'], - '`@charset` after `@import` rule' => ['@charset "UTF-8";', '@import "foo.css";'], '`@import` after style rule' => ['@import "foo.css";', 'p { color: red; }'], '`@import` after `@font-face` rule' => ['@import "foo.css";', self::VALID_AT_FONT_FACE_RULE], ]; @@ -516,7 +589,7 @@ public function provideInvalidNonConditionalAtRule(): array */ public function notRendersAtMediaRuleInNonConditionalAtRules(): void { - $subject = new CssDocument('@media screen { p { color: red; } }'); + $subject = $this->createDebugSubject('@media screen { p { color: red; } }'); $result = $subject->renderNonConditionalAtRules(); diff --git a/tests/Unit/CssInlinerTest.php b/tests/Unit/CssInlinerTest.php index 76d259af..07a63fa0 100644 --- a/tests/Unit/CssInlinerTest.php +++ b/tests/Unit/CssInlinerTest.php @@ -10,6 +10,7 @@ use Pelago\Emogrifier\Tests\Support\Traits\AssertCss; use PHPUnit\Framework\Constraint\Constraint; use PHPUnit\Framework\TestCase; +use Sabberworm\CSS\Parsing\UnexpectedTokenException; use Symfony\Component\CssSelector\CssSelectorConverter; use Symfony\Component\CssSelector\Exception\SyntaxErrorException; use TRegx\DataProvider\DataProviders; @@ -1407,34 +1408,60 @@ static function (string $styleAttributeContent): Constraint { } /** - * @return string[][] + * @return array> */ - public function invalidDeclarationDataProvider(): array + public function provideInvalidDeclaration(): array { return [ 'missing dash in property name' => ['font weight: bold;'], - 'invalid character in property name' => ['-9webkit-text-size-adjust:none;'], 'missing :' => ['-webkit-text-size-adjust none'], 'missing value' => ['-webkit-text-size-adjust :'], ]; } + /** + * @return array> + */ + public function provideDeclarationWithInvalidPropertyName(): array + { + return [ + 'invalid character in property name' => ['-9webkit-text-size-adjust:none;'], + ]; + } + /** * @test * * @param string $cssDeclarationBlock the CSS declaration block (without the curly braces) * - * @dataProvider invalidDeclarationDataProvider + * @dataProvider provideInvalidDeclaration + * @dataProvider provideDeclarationWithInvalidPropertyName */ - public function inlineCssDropsInvalidCssDeclaration(string $cssDeclarationBlock): void + public function inlineCssNotInDebugModeDropsInvalidCssDeclaration(string $cssDeclarationBlock): void { - $subject = $this->buildDebugSubject(''); + $subject = CssInliner::fromHtml(''); $subject->inlineCss('html {' . $cssDeclarationBlock . '}'); self::assertStringContainsString('', $subject->render()); } + /** + * @test + * + * @param string $cssDeclarationBlock the CSS declaration block (without the curly braces) + * + * @dataProvider provideInvalidDeclaration + */ + public function inlineCssInDebugModeForInvalidCssDeclarationThrowsException(string $cssDeclarationBlock): void + { + $this->expectException(UnexpectedTokenException::class); + + $subject = $this->buildDebugSubject(''); + + $subject->inlineCss('html {' . $cssDeclarationBlock . '}'); + } + /** * @test */ @@ -1557,10 +1584,7 @@ public function inlineCssRemovesStyleNodes(): void */ public function inlineCssInDebugModeForInvalidCssSelectorThrowsException(): void { - // @see https://github.com/sabberworm/PHP-CSS-Parser/issues/347 - self::markTestSkipped('This test is disabled as it currently is failing due to a bug in the CSS parser.'); - - $this->expectException(SyntaxErrorException::class); + $this->expectException(UnexpectedTokenException::class); $subject = CssInliner::fromHtml( '' @@ -3758,7 +3782,7 @@ public function copyUninlinableCssToStyleNodeHasNoSideEffects(): void $copyUninlinableCssToStyleNode->setAccessible(true); $domDocument = $subject->getDomDocument(); - $cssDocument = new CssDocument(''); + $cssDocument = new CssDocument('', true); $copyUninlinableCssToStyleNode->invoke($subject, $cssDocument); $expectedHtml = $subject->render();