Skip to content

Commit

Permalink
[TASK] Pass debug setting on to CSS parser
Browse files Browse the repository at this point in the history 
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 MyIntervals/PHP-CSS-Parser#127.

Some tests have been adapted to cater for an exception in debug mode with only
some of the data.
  • Loading branch information
@JakeQZ
JakeQZ committed Jan 12, 2022
1 parent 8942cfe commit 73a5987
Show file tree
Hide file tree
Showing 5 changed files with 138 additions and 29 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
16 changes: 14 additions & 2 deletions src/Css/CssDocument.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}

/**
Expand Down
2 changes: 1 addition & 1 deletion src/CssInliner.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
102 changes: 87 additions & 15 deletions tests/Unit/Css/CssDocumentTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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([]);

Expand All @@ -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([]);

Expand Down Expand Up @@ -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([]);

Expand Down Expand Up @@ -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']);

Expand Down Expand Up @@ -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']);

Expand Down Expand Up @@ -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']);

Expand Down Expand Up @@ -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']);

Expand All @@ -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; } }'
);

Expand Down Expand Up @@ -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();

Expand Down Expand Up @@ -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();

Expand Down Expand Up @@ -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<string, array{0: string}>
*/
Expand Down Expand Up @@ -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();

Expand All @@ -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<string, array<int, string>>
*/
Expand Down Expand Up @@ -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.
*
Expand Down
46 changes: 35 additions & 11 deletions tests/Unit/CssInlinerTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
*/
Expand Down Expand Up @@ -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>'
Expand Down Expand Up @@ -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();
Expand Down

0 comments on commit 73a5987

Please sign in to comment.