Changes suggested in code review:

- Use a data provider to provide various valid forms of `!important` (uppercase, lowercase, etc.) for testing; - Add tests confirming `!important` is removed from `style` attributes; - Remove `importantIsCaseInsensitive` test which is now superseded by the use of the above data provider; - Fix additional bug in which `! important` would not be removed from `style` attribute; The final item has meant replacing a `str_ireplace` with a `preg_replace`. Psalm picks up that in exceptional circumstances the latter may return `null`. Thus, a wrapper for `preg_replace` is required, which will not return `null`: - In debug mode it will throw an exception; - In non-debug mode it will log an error and return the original input string.
MyIntervals · Jun 23, 2020 · 9ec49ff · 9ec49ff
1 parent 1728d01
commit 9ec49ff
Show file tree

Hide file tree

Showing 2 changed files with 131 additions and 12 deletions.
diff --git a/src/CssInliner.php b/src/CssInliner.php
@@ -181,7 +181,8 @@ class CssInliner extends AbstractHtmlProcessor
      *
      * @return self fluent interface
      *
-     * @throws ParseException
+     * @throws ParseException in debug mode, if an invalid selector is encountered
+     * @throws \RuntimeException in debug mode, if an internal PCRE error occurs
      */
     public function inlineCss(string $css = ''): self
     {
@@ -995,7 +996,7 @@ private function generateStyleStringFromDeclarationsArrays(array $oldStyles, arr
      */
     private function attributeValueIsImportant(string $attributeValue): bool
     {
-        return \strtolower(\substr(\trim($attributeValue), -10)) === '!important';
+        return (bool)\preg_match('/!\\s*+important$/i', $attributeValue);
     }
 
     /**
@@ -1019,6 +1020,8 @@ private function fillStyleAttributesWithMergedStyles(): void
     /**
      * Searches for all nodes with a style attribute and removes the "!important" annotations out of
      * the inline style declarations, eventually by rearranging declarations.
+     *
+     * @throws \RuntimeException
      */
     private function removeImportantAnnotationFromAllInlineStyles(): void
     {
@@ -1036,6 +1039,8 @@ private function removeImportantAnnotationFromAllInlineStyles(): void
      * to "font-size: 13px; font: 12px serif;" in order to remain correct.
      *
      * @param \DOMElement $node
+     *
+     * @throws \RuntimeException
      */
     private function removeImportantAnnotationFromNodeInlineStyle(\DOMElement $node): void
     {
@@ -1044,7 +1049,7 @@ private function removeImportantAnnotationFromNodeInlineStyle(\DOMElement $node)
         $importantStyleDeclarations = [];
         foreach ($inlineStyleDeclarations as $property => $value) {
             if ($this->attributeValueIsImportant($value)) {
-                $importantStyleDeclarations[$property] = \trim(\str_ireplace('!important', '', $value));
+                $importantStyleDeclarations[$property] = $this->pregReplace('/\\s*+!\\s*+important$/i', '', $value);
             } else {
                 $regularStyleDeclarations[$property] = $value;
             }
@@ -1289,4 +1294,60 @@ private function getHeadElement(): \DOMElement
     {
         return $this->domDocument->getElementsByTagName('head')->item(0);
     }
+
+    /**
+     * Wraps `preg_replace`.  If an error occurs (which is highly unlikely), either it is logged and the original
+     * `$subject` is returned, or in debug mode an exception is thrown.
+     *
+     * This method does not currently allow `$subject` (and return value) to be an array, because a means of telling
+     * Psalm that a method returns the same type a particular parameter has not been found (though it knows this for
+     * `preg_replace`); nor does it currently support the optional parameters.
+     *
+     * @param string|string[] $pattern
+     * @param string|string[] $replacement
+     * @param string $subject
+     *
+     * @return string
+     *
+     * @throws \RuntimeException
+     */
+    private function pregReplace($pattern, $replacement, string $subject): string
+    {
+        $result = \preg_replace($pattern, $replacement, $subject);
+
+        if ($result === null) {
+            $this->logOrThrowPregLastError();
+            $result = $subject;
+        }
+
+        return $result;
+    }
+
+    /**
+     * Obtains the name of the error constant for `preg_last_error` (based on code posted at
+     * {@see https://www.php.net/manual/en/function.preg-last-error.php#124124}) and puts it into an error message
+     * which is either passed to `trigger_error` (in non-debug mode) or an exception which is thrown (in debug mode).
+     *
+     * @throws \RuntimeException
+     */
+    private function logOrThrowPregLastError(): void
+    {
+        $pcreConstants = \get_defined_constants(true)['pcre'];
+        $pcreErrorConstantNames = \is_array($pcreConstants) ? \array_flip(\array_filter(
+            $pcreConstants,
+            function (string $key): bool {
+                return \substr($key, -6) === '_ERROR';
+            },
+            ARRAY_FILTER_USE_KEY
+        )) : [];
+
+        $pregLastError = \preg_last_error();
+        $message = 'PCRE regex execution error `' . (string)($pcreErrorConstantNames[$pregLastError] ?? $pregLastError)
+            . '`';
+
+        if ($this->debug) {
+            throw new \RuntimeException($message, 1592870147);
+        }
+        \trigger_error($message);
+    }
 }
diff --git a/tests/Unit/CssInlinerTest.php b/tests/Unit/CssInlinerTest.php
@@ -2752,42 +2752,100 @@ private function buildSubjectWithBoilerplateHtml(string $style = ''): CssInliner
         return $this->buildDebugSubject($html);
     }
 
+    /**
+     * @return string[][]
+     */
+    public function provideImportantDeclarationMarker(): array
+    {
+        return [
+            'lower case' => [' !important'],
+            'mixed case' => [' !ImPorTant'],
+            'upper case' => [' !IMPORTANT'],
+            'with space within' => [' ! important'],
+            'without space preceding' => ['!important'],
+        ];
+    }
+
     /**
      * @test
+     *
+     * @param string $importantMarker
+     *
+     * @dataProvider provideImportantDeclarationMarker
      */
-    public function importantInExternalCssOverwritesInlineCss(): void
+    public function inlineCssRemovesImportantFromStyleAttribute(string $importantMarker): void
     {
-        $subject = $this->buildSubjectWithBoilerplateHtml('margin: 2px;');
+        $subject = $this->buildSubjectWithBoilerplateHtml('margin: 1px' . $importantMarker . ';');
 
-        $subject->inlineCss('p { margin: 1px !important; }');
+        $subject->inlineCss();
 
         self::assertStringContainsString('<p style="margin: 1px;">', $subject->renderBodyContent());
     }
 
     /**
      * @test
+     *
+     * @param string $importantMarker
+     *
+     * @dataProvider provideImportantDeclarationMarker
      */
-    public function importantInExternalCssKeepsInlineCssForOtherAttributes(): void
+    public function inlineCssRemovesImportantFromInlinedStyleAttribute(string $importantMarker): void
     {
-        $subject = $this->buildSubjectWithBoilerplateHtml('margin: 2px; text-align: center;');
+        $subject = $this->buildSubjectWithBoilerplateHtml();
 
-        $subject->inlineCss('p { margin: 1px !important; }');
+        $subject->inlineCss('p { margin: 1px' . $importantMarker . '; }');
 
-        self::assertStringContainsString('<p style="text-align: center; margin: 1px;">', $subject->renderBodyContent());
+        self::assertStringContainsString('<p style="margin: 1px;">', $subject->renderBodyContent());
     }
 
     /**
      * @test
+     *
+     * @param string $importantMarker
+     *
+     * @dataProvider provideImportantDeclarationMarker
      */
-    public function importantIsCaseInsensitive(): void
+    public function importantInExternalCssOverwritesInlineCss(string $importantMarker): void
     {
         $subject = $this->buildSubjectWithBoilerplateHtml('margin: 2px;');
 
-        $subject->inlineCss('p { margin: 1px !ImPorTant; }');
+        $subject->inlineCss('p { margin: 1px' . $importantMarker . '; }');
 
         self::assertStringContainsString('<p style="margin: 1px;">', $subject->renderBodyContent());
     }
 
+    /**
+     * @test
+     *
+     * @param string $importantMarker
+     *
+     * @dataProvider provideImportantDeclarationMarker
+     */
+    public function importantInExternalCssNotOverwritesImportantInInlineCss(string $importantMarker): void
+    {
+        $subject = $this->buildSubjectWithBoilerplateHtml('margin: 2px' . $importantMarker . ';');
+
+        $subject->inlineCss('p { margin: 1px' . $importantMarker . '; }');
+
+        self::assertStringContainsString('<p style="margin: 2px;">', $subject->renderBodyContent());
+    }
+
+    /**
+     * @test
+     *
+     * @param string $importantMarker
+     *
+     * @dataProvider provideImportantDeclarationMarker
+     */
+    public function importantInExternalCssKeepsInlineCssForOtherAttributes(string $importantMarker): void
+    {
+        $subject = $this->buildSubjectWithBoilerplateHtml('margin: 2px; text-align: center;');
+
+        $subject->inlineCss('p { margin: 1px' . $importantMarker . '; }');
+
+        self::assertStringContainsString('<p style="text-align: center; margin: 1px;">', $subject->renderBodyContent());
+    }
+
     /**
      * @test
      */