Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[CLEANUP] Refactor parseCssRules: parse all CSS #528

Merged
merged 5 commits into from
Mar 31, 2018
Merged

[CLEANUP] Refactor parseCssRules: parse all CSS #528

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
cda0d91
[CLEANUP] Refactor `parseCssRules`: parse all CSS
JakeQZ Feb 22, 2018
59c31f0
Merge branch 'master' into refactor/parseCssRules
JakeQZ Mar 29, 2018
3ca3726
Reverted changes to CssInliner.php
JakeQZ Mar 29, 2018
53c169f
Changes following review:
JakeQZ Mar 29, 2018
5618067
Update Emogrifier.php
oliverklee Mar 31, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
142 changes: 94 additions & 48 deletions src/Emogrifier.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -375,12 +375,9 @@ protected function process(\DOMDocument $xmlDocument)
$allCss .= $this->getCssFromAllStyleNodes($xPath);
}

$splitCss = $this->splitCssAndMediaQuery($allCss);
$cssParts = $this->combineSplitCssAndMediaQueryParts($splitCss);

$excludedNodes = $this->getNodesToExclude($xPath);
$cssRules = $this->parseCssRules($cssParts['css']);
foreach ($cssRules as $cssRule) {
$cssRules = $this->parseCssRules($allCss);
foreach ($cssRules['inlineable'] as $cssRule) {
// There's no real way to test "PHP Warning" output generated by the following XPath query unless PHPUnit
// converts it to an exception. Unfortunately, this would only apply to tests and not work for production
// executions, which can still flood logs/output unnecessarily. Instead, Emogrifier's error handler should
Expand Down Expand Up @@ -429,7 +426,11 @@ protected function process(\DOMDocument $xmlDocument)

$this->removeImportantAnnotationFromAllInlineStyles($xPath);

$this->copyCssWithMediaToStyleNode($xmlDocument, $xPath, $cssParts['media']);
$this->copyCssWithMediaToStyleNode(
$xmlDocument,
$xPath,
$this->combineCssMediaRules($cssRules['uninlineable'])
);

restore_error_handler();
}
Expand Down Expand Up @@ -658,7 +659,11 @@ private function parseCssShorthandValue($value)
*
* @param string $css a string of raw CSS code
*
* @return string[][] an array of string sub-arrays with the keys
* @return string[][][] A 2-entry array with the key "inlineable" containing rules which can be inlined as `style`
* attributes and the key "uninlineable" containing rules which cannot. Each value is an array of string
* sub-arrays with the keys
* "media" (the media query string, e.g. "@media screen and (max-width: 480px)",
* or an empty string if not from a `@media` rule),
* "selector" (the CSS selector(s), e.g., "*" or "h1"),
* "declarationsBLock" (the semicolon-separated CSS declarations for that selector(s),
* e.g., "color: red; height: 4px;"),
Expand All @@ -668,19 +673,21 @@ private function parseCssRules($css)
{
$cssKey = md5($css);
if (!isset($this->caches[static::CACHE_KEY_CSS][$cssKey])) {
// process the CSS file for selectors and definitions
preg_match_all('/(?:^|[\\s^{}]*)([^{]+){([^}]*)}/mi', $css, $matches, PREG_SET_ORDER);
$matches = $this->getCssRuleMatches($css);

$cssRules = [];
$cssRules = [
'inlineable' => [],
'uninlineable' => [],
];
/** @var string[][] $matches */
/** @var string[] $cssRule */
foreach ($matches as $key => $cssRule) {
$cssDeclaration = trim($cssRule[2]);
$cssDeclaration = trim($cssRule['declarations']);
if ($cssDeclaration === '') {
continue;
}

$selectors = explode(',', $cssRule[1]);
$selectors = explode(',', $cssRule['selectors']);
foreach ($selectors as $selector) {
// don't process pseudo-elements and behavioral (dynamic) pseudo-classes;
// only allow structural pseudo-classes
Expand All @@ -694,23 +701,59 @@ private function parseCssRules($css)
continue;
}

$cssRules[] = [
$parsedCssRule = [
'media' => $cssRule['media'],
'selector' => trim($selector),
'declarationsBlock' => $cssDeclaration,
// keep track of where it appears in the file, since order is important
'line' => $key,
];
$ruleType = ($cssRule['media'] === '') ? 'inlineable' : 'uninlineable';
$cssRules[$ruleType][] = $parsedCssRule;
}
}

usort($cssRules, [$this, 'sortBySelectorPrecedence']);
usort($cssRules['inlineable'], [$this, 'sortBySelectorPrecedence']);

$this->caches[static::CACHE_KEY_CSS][$cssKey] = $cssRules;
}

return $this->caches[static::CACHE_KEY_CSS][$cssKey];
}

/**
* Parses a string of CSS into the media query, selectors and declarations for each ruleset in order.
*
* @param string $css
*
* @return string[][] Array of string sub-arrays with the keys
* "media" (the media query string, e.g. "@media screen and (max-width: 480px)",
* or an empty string if not from an `@media` rule),
* "selectors" (the CSS selector(s), e.g., "*" or "h1, h2"),
* "declarations" (the semicolon-separated CSS declarations for that/those selector(s),
* e.g., "color: red; height: 4px;"),
*/
private function getCssRuleMatches($css)
{
$ruleMatches = [];

$splitCss = $this->splitCssAndMediaQuery($css);
foreach ($splitCss as $cssPart) {
// process each part for selectors and definitions
preg_match_all('/(?:^|[\\s^{}]*)([^{]+){([^}]*)}/mi', $cssPart['css'], $matches, PREG_SET_ORDER);

foreach ($matches as $cssRule) {
$ruleMatches[] = [
'media' => $cssPart['media'],
'selectors' => $cssRule[1],
'declarations' => $cssRule[2],
];
}
}

return $ruleMatches;
}

/**
* Disables the parsing of inline styles.
*
Expand Down Expand Up @@ -1067,6 +1110,41 @@ private function attributeValueIsImportant($attributeValue)
return strtolower(substr(trim($attributeValue), -10)) === '!important';
}

/**
* Reconstructs the CSS containing only allowed @media rules from the "uninlineable" CSS rules array returned by
* `parseCssRules`.
*
* @param string[][] $cssRules
*
* @return string
*/
private function combineCssMediaRules(array $cssRules)
Copy link
Contributor

@oliverklee oliverklee Feb 22, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also please split up this method? It's far too long. This would also allow to get rid of the comments (if the methods are well-named).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is why I'd like to add a CssConcatenator class.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now using CssConcatenator.

{
// filter out any rules with no media query (shouldn't be any at present)
$cssMediaRules = array_filter(
$cssRules,
function (array $cssRule) {
return $cssRule['media'] !== '';
}
);

if ($cssMediaRules === []) {
// avoid including class dependency if it's not actually needed
return '';
}

// support use without autoload
if (!class_exists('Pelago\\Emogrifier\\CssConcatenator')) {
require_once __DIR__ . '/Emogrifier/CssConcatenator.php';
}

$cssConcatenator = new Emogrifier\CssConcatenator();
foreach ($cssMediaRules as $cssRule) {
$cssConcatenator->append([$cssRule['selector']], $cssRule['declarationsBlock'], $cssRule['media']);
}
return $cssConcatenator->getCss();
}

/**
* Applies $css to $xmlDocument, limited to the media queries that actually apply to the document.
*
Expand All @@ -1085,7 +1163,8 @@ private function copyCssWithMediaToStyleNode(\DOMDocument $xmlDocument, \DOMXPat
$mediaQueriesRelevantForDocument = [];

foreach ($this->extractMediaQueriesFromCss($css) as $mediaQuery) {
foreach ($this->parseCssRules($mediaQuery['css']) as $selector) {
// as only the @media rule body is being parsed, the rules will be parsed as "inlineable"
foreach ($this->parseCssRules($mediaQuery['css'])['inlineable'] as $selector) {
if ($this->existsMatchForCssSelector($xPath, $selector['selector'])) {
$mediaQueriesRelevantForDocument[] = $mediaQuery['query'];
break;
Expand Down Expand Up @@ -1313,39 +1392,6 @@ private function splitCssAndMediaQuery($css)
return $splitCss;
}

/**
* Combines the parts returned by `splitCssAndMediaQuery` into just two strings.
*
* @param string[][] $splitCss as returned by `splitCssAndMediaQuery`
*
* @return string[] Array with the following keys:
* "css" => the cleaned CSS without any @media rules
* "media" => the allowed @media rules from the original CSS
*/
private function combineSplitCssAndMediaQueryParts(array $splitCss)
{
return [
'css' => implode(
'',
array_map(
function (array $cssParts) {
return $cssParts['media'] === '' ? $cssParts['css'] : '';
},
$splitCss
)
),
'media' => implode(
'',
array_map(
function (array $cssParts) {
return $cssParts['media'] !== '' ? $cssParts['media'] . '{' . $cssParts['css'] . '}' : '';
},
$splitCss
)
),
];
}

/**
* Creates a DOMDocument instance with the current HTML.
*
Expand Down