[TASK] Split copyUninlinableCssToStyleNode()

Added new method `determineMatchingUninlinableCssRules` to do the first part of the job (filtering CSS rules for those relevant to the document) and store the result as a class property. Part of #380.
MyIntervals · Sep 12, 2019 · 82869fe · 82869fe
1 parent ee831be
commit 82869fe
Show file tree

Hide file tree

Showing 2 changed files with 34 additions and 9 deletions.
diff --git a/src/Emogrifier/CssInliner.php b/src/Emogrifier/CssInliner.php
@@ -128,6 +128,14 @@ class CssInliner extends AbstractHtmlProcessor
         '(?:(?<![="\':\\w\\-])|::)' => 1,
     ];
 
+    /**
+     * array of data describing CSS rules which apply to the document but cannot be inlined, in the format returned by
+     * `parseCssRules`
+     *
+     * @var string[][]
+     */
+    private $matchingUninlinableCssRules = [];
+
     /**
      * Emogrifier will throw Exceptions when it encounters an error instead of silently ignoring them.
      *
@@ -210,7 +218,8 @@ public function inlineCss($css = '')
 
         $this->removeImportantAnnotationFromAllInlineStyles();
 
-        $this->copyUninlinableCssToStyleNode($cssRules['uninlinable']);
+        $this->determineMatchingUninlinableCssRules($cssRules['uninlinable']);
+        $this->copyUninlinableCssToStyleNode();
 
         return $this;
     }
@@ -618,15 +627,16 @@ private function attributeValueIsImportant($attributeValue)
     }
 
     /**
-     * Applies $cssRules to $this->domDocument, limited to the rules that actually apply to the document.
+     * Determines which of `$cssRules` actually apply to `$this->domDocument`, and sets them in
+     * `$this->matchingUninlinableCssRules`.
      *
-     * @param string[][] $cssRules The "uninlinable" array of CSS rules returned by `parseCssRules`
+     * @param string[][] $cssRules the "uninlinable" array of CSS rules returned by `parseCssRules`
      *
      * @return void
      */
-    private function copyUninlinableCssToStyleNode(array $cssRules)
+    private function determineMatchingUninlinableCssRules(array $cssRules)
     {
-        $cssRulesRelevantForDocument = \array_filter(
+        $this->matchingUninlinableCssRules = \array_filter(
             $cssRules,
             function (array $cssRule) {
                 $selector = $cssRule['selector'];
@@ -636,14 +646,23 @@ function (array $cssRule) {
                 return $this->existsMatchForCssSelector($selector);
             }
         );
+    }
 
-        if ($cssRulesRelevantForDocument === []) {
+    /**
+     * Applies `$this->matchingUninlinableCssRules` to `$this->domDocument` by placing them as CSS in a `<style>`
+     * element.
+     *
+     * @return void
+     */
+    private function copyUninlinableCssToStyleNode()
+    {
+        if ($this->matchingUninlinableCssRules === []) {
             // avoid adding empty style element (or including unneeded class dependency)
             return;
         }
 
         $cssConcatenator = new CssConcatenator();
-        foreach ($cssRulesRelevantForDocument as $cssRule) {
+        foreach ($this->matchingUninlinableCssRules as $cssRule) {
             $cssConcatenator->append([$cssRule['selector']], $cssRule['declarationsBlock'], $cssRule['media']);
         }
 

diff --git a/tests/Unit/CssInlinerTest.php b/tests/Unit/CssInlinerTest.php
@@ -2641,19 +2641,25 @@ public function copyUninlinableCssToStyleNodeHasNoSideEffects()
                 'line' => 1,
             ],
         ];
+        $matchingUninlinableCssRulesProperty = new \ReflectionProperty(
+            CssInliner::class,
+            'matchingUninlinableCssRules'
+        );
+        $matchingUninlinableCssRulesProperty->setAccessible(true);
+        $matchingUninlinableCssRulesProperty->setValue($subject, $uninlinableCssRules);
 
         $copyUninlinableCssToStyleNode = new \ReflectionMethod(CssInliner::class, 'copyUninlinableCssToStyleNode');
         $copyUninlinableCssToStyleNode->setAccessible(true);
 
         $domDocument = $subject->getDomDocument();
 
-        $copyUninlinableCssToStyleNode->invoke($subject, $uninlinableCssRules);
+        $copyUninlinableCssToStyleNode->invoke($subject);
         $expectedHtml = $subject->render();
 
         $styleElement = $domDocument->getElementsByTagName('style')->item(0);
         $styleElement->parentNode->removeChild($styleElement);
 
-        $copyUninlinableCssToStyleNode->invoke($subject, $uninlinableCssRules);
+        $copyUninlinableCssToStyleNode->invoke($subject);
 
         self::assertSame($expectedHtml, $subject->render());
     }