[BUGFIX] Remove uppercase !important from attributes (#911)

Also fix additional bug in which `! important` would not be removed from `style` attribute; This 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 · dcd7650 · dcd7650
1 parent e9ddac2
commit dcd7650
Show file tree

Hide file tree

Showing 4 changed files with 140 additions and 14 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -36,6 +36,9 @@ This project adheres to [Semantic Versioning](https://semver.org/).
   ([#880](https://github.com/MyIntervals/emogrifier/pull/880))
 
 ### Fixed
+- Remove `!important` from `style` attributes also when uppercase, mixed case or
+  having whitespace after `!`
+  ([#911](https://github.com/MyIntervals/emogrifier/pull/911))
 - Copy rules using `:...of-type` without a type to the `<style>` element
   ([#904](https://github.com/MyIntervals/emogrifier/pull/904))
 - Support combinator followed by dynamic pseudo-class in minified CSS

diff --git a/phpcs.xml b/phpcs.xml
@@ -56,7 +56,11 @@
         <exclude name="Squiz.Commenting.FunctionComment.ThrowsNotCapital"/>
         <exclude name="Squiz.Commenting.FunctionComment.ThrowsNoFullStop"/>
     </rule>
-    <rule ref="Squiz.Commenting.FunctionCommentThrowTag"/>
+    <rule ref="Squiz.Commenting.FunctionCommentThrowTag">
+        <!-- If we want to document exceptions thrown by called methods for a method that also throws its own
+             exceptions, this sniff simply does not work. -->
+        <exclude name="Squiz.Commenting.FunctionCommentThrowTag.WrongNumber"/>
+    </rule>
     <rule ref="Squiz.Commenting.PostStatementComment"/>
 
     <!-- Control structures -->

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_replace('!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,40 +2752,98 @@ 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;');
 
-        self::assertStringContainsString('<p style="margin: 1px !ImPorTant;">', $subject->renderBodyContent());
+        $subject->inlineCss('p { margin: 1px' . $importantMarker . '; }');
+
+        self::assertStringContainsString('<p style="text-align: center; margin: 1px;">', $subject->renderBodyContent());
     }
 
     /**