[TASK] Pass debug setting on to CSS parser

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)

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
@@ -36,11 +37,31 @@ class CssDocument
  private $isImportRuleAllowed = true;
 
  /**
- * @param string $css
+ * @param string $css {@see https://github.com/squizlabs/PHP_CodeSniffer/issues/3521}
+ * @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 || static::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 static function hasNestedAtRule(string $css): bool
+ {
+ return \preg_match('/@(?:media|supports|(?:-webkit-|-moz-|-ms-|-o-)?+(keyframes|document))\\b/', $css) === 1;
  }
 
  /**

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);

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 static 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 = self::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 = self::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 = self::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 = self::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 = self::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 = self::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 = self::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 = self::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 = self::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 = self::createDebugSubject('@import "foo.css";' . $cssBetween . self::VALID_AT_FONT_FACE_RULE);
 
  $result = $subject->renderNonConditionalAtRules();
 
@@ -432,20 +441,51 @@ public function provideCssWithoutNonConditionalAtRules(): array
  /**
  * @test
  *
- * @param string $css
- *
  * @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
+ *
+ * @dataProvider provideValidAtCharsetRules
+ */
+ public function discardsValidAtCharsetRuleInDebugMode(string $css): void
+ {
+ $subject = self::createDebugSubject($css);
+
+ $result = $subject->renderNonConditionalAtRules();
+
+ self::assertSame('', $result);
+ }
+
+ /**
+ * @test
+ *
+ * @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<string, array{0: string}>
  */
@@ -472,14 +512,11 @@ public function provideInvalidAtCharsetRules(): array
  /**
  * @test
  *
- * @param string $atRuleCss
- * @param string $cssBefore
- *
  * @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 +525,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<string, array<int, string>>
  */
@@ -516,7 +579,7 @@ 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();
 

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<string, array<int, string>>
  */
- 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<string, array<int, string>>
+ */
+ 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('<html></html>');
+ $subject = CssInliner::fromHtml('<html></html>');
 
  $subject->inlineCss('html {' . $cssDeclarationBlock . '}');
 
  self::assertStringContainsString('<html>', $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('<html></html>');
+
+ $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(
  '<html><style type="text/css">p{color:red;} <style data-x="1">html{cursor:text;}</style></html>'
@@ -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();