From 73a59878305233af9255adaadd94064b042ec774 Mon Sep 17 00:00:00 2001 From: Jake Hotson Date: Tue, 21 Dec 2021 01:42:26 +0000 Subject: [PATCH] [TASK] Pass debug setting on to CSS parser This partially addresses both #1139 and #1140. However, if the CSS contains nested at-rules, the debug setting won't currently be passed on, due to https://github.com/sabberworm/PHP-CSS-Parser/issues/127. Some tests have been adapted to cater for an exception in debug mode with only some of the data. --- CHANGELOG.md | 1 + src/Css/CssDocument.php | 16 ++++- src/CssInliner.php | 2 +- tests/Unit/Css/CssDocumentTest.php | 102 ++++++++++++++++++++++++----- tests/Unit/CssInlinerTest.php | 46 +++++++++---- 5 files changed, 138 insertions(+), 29 deletions(-) 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..3d792a8f 100644 --- a/src/Css/CssDocument.php +++ b/src/Css/CssDocument.php @@ -37,10 +37,22 @@ 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 + // @see https://github.com/sabberworm/PHP-CSS-Parser/issues/127 + $parserSettings = \Sabberworm\CSS\Settings::create()->withLenientParsing( + !$debug || + \preg_match('/@(?:media|supports|(?:-webkit-|-moz-|-ms-|-o-)?+(keyframes|document))\\b/', $css) === 1 + ); + + // 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(); } /** 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..5a099db3 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 @@ -33,7 +35,7 @@ final class CssDocumentTest extends TestCase public function parsesSelector(string $selector): void { $css = $selector . '{ color: green; }'; - $subject = new CssDocument($css); + $subject = self::createDebugSubject($css); $result = $subject->getStyleRulesData([]); @@ -47,7 +49,7 @@ public function parsesSelector(string $selector): void public function canParseMultipleSelectors(): void { $css = 'h1, h2 { color: green; }'; - $subject = new CssDocument($css); + $subject = self::createDebugSubject($css); $result = $subject->getStyleRulesData([]); @@ -115,7 +117,7 @@ public function provideSelectorWithVariedWhitespace(): array public function parsesDeclarations(string $declarations): void { $css = 'p {' . $declarations . '}'; - $subject = new CssDocument($css); + $subject = self::createDebugSubject($css); $result = $subject->getStyleRulesData([]); @@ -177,7 +179,7 @@ public function parsesAtMediaRule(string $mediaQuery): void { $atMediaAndQuery = '@media ' . $mediaQuery; $css = $atMediaAndQuery . ' { p { color: green; } }'; - $subject = new CssDocument($css); + $subject = self::createDebugSubject($css); $result = $subject->getStyleRulesData(['screen']); @@ -218,7 +220,7 @@ public function parsesAtMediaRuleWithVariedWhitespace( $atMediaAndQuery = '@media' . $whitespaceAfterAtMedia . 'screen'; $css = $atMediaAndQuery . $optionalWhitespaceWithinRule . '{' . $optionalWhitespaceWithinRule . 'p { color: green; }' . $optionalWhitespaceWithinRule . '}'; - $subject = new CssDocument($css); + $subject = self::createDebugSubject($css); $result = $subject->getStyleRulesData(['screen']); @@ -252,7 +254,7 @@ public function provideVariedWhitespaceForAtMediaRule(): array */ public function discardsMediaRuleWithTypeNotInAllowlist(string $mediaQuery): void { - $subject = new CssDocument('@media ' . $mediaQuery . ' { p { color: red; } }'); + $subject = self::createDebugSubject('@media ' . $mediaQuery . ' { p { color: red; } }'); $result = $subject->getStyleRulesData(['screen']); @@ -296,7 +298,9 @@ public function provideMediaQueryWithTvTypeAndVariedWhitespace(): array */ public function parsesMultipleStyleRulesWithOtherCssBetween(string $cssBetween): void { - $subject = new CssDocument('p { color: green; }' . $cssBetween . '@media screen { h1 { color: green; } }'); + $subject = self::createDebugSubject( + 'p { color: green; }' . $cssBetween . '@media screen { h1 { color: green; } }' + ); $result = $subject->getStyleRulesData(['screen']); @@ -314,7 +318,7 @@ public function parsesMultipleStyleRulesWithOtherCssBetween(string $cssBetween): */ public function parsesMultipleStyleRulesWithOtherCssBefore(string $cssBefore): void { - $subject = new CssDocument( + $subject = self::createDebugSubject( $cssBefore . 'p { color: green; } @media screen { h1 { color: green; } }' ); @@ -362,7 +366,7 @@ public function provideCssThatMustPrecedeStyleRules(): array */ public function rendersValidNonConditionalAtRule(string $atRuleCss, string $cssBefore = ''): void { - $subject = new CssDocument($cssBefore . $atRuleCss); + $subject = self::createDebugSubject($cssBefore . $atRuleCss); $result = $subject->renderNonConditionalAtRules(); @@ -403,7 +407,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 = self::createDebugSubject('@import "foo.css";' . $cssBetween . self::VALID_AT_FONT_FACE_RULE); $result = $subject->renderNonConditionalAtRules(); @@ -437,15 +441,52 @@ public function provideCssWithoutNonConditionalAtRules(): array * @dataProvider provideValidAtCharsetRules * @dataProvider provideInvalidAtCharsetRules */ - public function discardsValidOrInvalidAtCharsetRule(string $css): void + public function discardsValidOrInvalidAtCharsetRuleNotInDebugMode(string $css): void { - $subject = new CssDocument($css); + $subject = new CssDocument($css, false); $result = $subject->renderNonConditionalAtRules(); self::assertSame('', $result); } + /** + * @test + * + * @param string $css + * + * @dataProvider provideValidAtCharsetRules + */ + public function discardsValidAtCharsetRuleInDebugMode(string $css): void + { + $subject = self::createDebugSubject($css); + + $result = $subject->renderNonConditionalAtRules(); + + self::assertSame('', $result); + } + + /** + * @test + * + * @param string $css + * + * @dataProvider provideInvalidAtCharsetRules + */ + public function throwsExceptionForOrDiscardsInvalidAtCharsetRuleInDebugMode(string $css): void + { + try { + $subject = self::createDebugSubject($css); + + $result = $subject->renderNonConditionalAtRules(); + + self::assertSame('', $result); + } catch (UnexpectedEOFException $exception) { + // Passed test + self::expectNotToPerformAssertions(); + } + } + /** * @return array */ @@ -477,9 +518,9 @@ public function provideInvalidAtCharsetRules(): array * * @dataProvider provideInvalidNonConditionalAtRule */ - 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,6 +529,32 @@ public function notRendersInvalidNonConditionalAtRule(string $atRuleCss, string self::assertStringNotContainsString($atAndRuleName, $result); } + /** + * @test + * + * @param string $atRuleCss + * @param string $cssBefore + * + * @dataProvider provideInvalidNonConditionalAtRule + */ + public function throwsExceptionForOrDiscardsInvalidNonConditionalAtRuleInDebugMode( + string $atRuleCss, + string $cssBefore = '' + ): void { + try { + $subject = self::createDebugSubject($cssBefore . $atRuleCss); + + $result = $subject->renderNonConditionalAtRules(); + + \preg_match('/@[\\w\\-]++/', $atRuleCss, $atAndRuleNameMatches); + $atAndRuleName = $atAndRuleNameMatches[0]; + self::assertStringNotContainsString($atAndRuleName, $result); + } catch (UnexpectedTokenException $exception) { + // Passed test + self::expectNotToPerformAssertions(); + } + } + /** * @return array> */ @@ -516,13 +583,18 @@ public function provideInvalidNonConditionalAtRule(): array */ public function notRendersAtMediaRuleInNonConditionalAtRules(): void { - $subject = new CssDocument('@media screen { p { color: red; } }'); + $subject = self::createDebugSubject('@media screen { p { color: red; } }'); $result = $subject->renderNonConditionalAtRules(); self::assertSame('', $result); } + private static function createDebugSubject(string $css): CssDocument + { + return new CssDocument($css, true); + } + /** * Asserts that two strings are the same after `trim`ming both of them. * 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();