From e4a15f308911a344345d0f4cd5408893851f2179 Mon Sep 17 00:00:00 2001 From: Oliver Klee Date: Tue, 1 Oct 2019 19:50:17 +0200 Subject: [PATCH] [BUGFIX] Fix PhpStorm code inspection warnings (#770) [BUGFIX] Fix PhpStorm code inspection warnings Fixes #729 Also update regular expression for CSS rule names --- CHANGELOG.md | 3 ++ src/Emogrifier.php | 40 +++++++++---------- src/Emogrifier/CssInliner.php | 17 ++++---- .../HtmlProcessor/AbstractHtmlProcessor.php | 5 +++ .../HtmlProcessor/CssToAttributeConverter.php | 9 ++--- src/Emogrifier/HtmlProcessor/HtmlPruner.php | 2 + tests/Support/Traits/AssertCss.php | 2 +- tests/Unit/CssInlinerTest.php | 3 +- .../AbstractHtmlProcessorTest.php | 1 + tests/Unit/EmogrifierTest.php | 15 +------ 10 files changed, 45 insertions(+), 52 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1afa6271..a23621bc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -105,6 +105,9 @@ This project adheres to [Semantic Versioning](https://semver.org/). ([#690](https://github.com/MyIntervals/emogrifier/pull/690)) ### Fixed +- Fix PhpStorm code inspection warnings + ([#729](https://github.com/MyIntervals/emogrifier/issues/729), + [#770](https://github.com/MyIntervals/emogrifier/pull/770)) - Uppercase type combined with class or ID in selector ([#590](https://github.com/MyIntervals/emogrifier/issues/590), [#769](https://github.com/MyIntervals/emogrifier/pull/769)) diff --git a/src/Emogrifier.php b/src/Emogrifier.php index 9ddf14dc..5f6c6c55 100644 --- a/src/Emogrifier.php +++ b/src/Emogrifier.php @@ -587,7 +587,7 @@ private function getAllNodesWithStyleAttribute() * "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, e.g., "*" or "header h1"), - * "hasUnmatchablePseudo" (true if that selector contains psuedo-elements or dynamic pseudo-classes + * "hasUnmatchablePseudo" (true if that selector contains pseudo-elements or dynamic pseudo-classes * such that the declarations cannot be applied inline), * "declarationsBlock" (the semicolon-separated CSS declarations for that selector, * e.g., "color: red; height: 4px;"), @@ -611,8 +611,7 @@ private function parseCssRules($css) continue; } - $selectors = \explode(',', $cssRule['selectors']); - foreach ($selectors as $selector) { + foreach (\explode(',', $cssRule['selectors']) as $selector) { // don't process pseudo-elements and behavioral (dynamic) pseudo-classes; // only allow structural pseudo-classes $hasPseudoElement = \strpos($selector, '::') !== false; @@ -752,6 +751,7 @@ public function removeUnprocessableHtmlTag($tagName) { $key = \array_search($tagName, $this->unprocessableHtmlTags, true); if ($key !== false) { + /** @var int|string $key */ unset($this->unprocessableHtmlTags[$key]); } } @@ -843,7 +843,7 @@ private function normalizeStyleAttributesOfAllNodes() private function normalizeStyleAttributes(\DOMElement $node) { $normalizedOriginalStyle = \preg_replace_callback( - '/[A-z\\-]+(?=\\:)/S', + '/-?+[_a-zA-Z][\\w\\-]*+(?=:)/S', static function (array $m) { return \strtolower($m[0]); }, @@ -1004,7 +1004,7 @@ private function copyUninlinableCssToStyleNode(array $cssRules, $cssImportRules) // avoid including unneeded class dependency if there are no rules if ($cssRulesRelevantForDocument !== []) { // support use without autoload - if (!\class_exists('Pelago\\Emogrifier\\Utilities\\CssConcatenator')) { + if (!\class_exists(CssConcatenator::class)) { require_once __DIR__ . '/Emogrifier/Utilities/CssConcatenator.php'; } @@ -1058,7 +1058,7 @@ private function removeUnmatchablePseudoComponents($selector) // The regex allows nested brackets via `(?2)`. // A space is temporarily prepended because the callback can't determine if the match was at the very start. $selectorWithoutNots = \ltrim(\preg_replace_callback( - '/(\\s?+):not(\\([^\\(\\)]*+(?:(?2)[^\\(\\)]*+)*+\\))/i', + '/(\\s?+):not(\\([^()]*+(?:(?2)[^()]*+)*+\\))/i', [$this, 'replaceUnmatchableNotComponent'], ' ' . $selector )); @@ -1169,6 +1169,8 @@ protected function addStyleElementToDocument($css) * Checks that $this->domDocument has a BODY element and adds it if it is missing. * * @return void + * + * @throws \UnexpectedValueException */ private function ensureExistenceOfBodyElement() { @@ -1177,6 +1179,9 @@ private function ensureExistenceOfBodyElement() } $htmlElement = $this->domDocument->getElementsByTagName('html')->item(0); + if ($htmlElement === null) { + throw new \UnexpectedValueException('There is no HTML element although there should be one.', 1569930874); + } $htmlElement->appendChild($this->domDocument->createElement('body')); } @@ -1230,7 +1235,7 @@ private function extractImportAndCharsetRules($css) } /** - * Splits input CSS code into an array of parts for different media querues, in order. + * Splits input CSS code into an array of parts for different media queries, in order. * Each part is an array where: * * - key "css" will contain clean CSS code (for @media rules this will be the group rule body within "{...}") @@ -1268,7 +1273,7 @@ private function splitCssAndMediaQuery($css) $mediaRuleBodyMatcher = '[^{]*+{(?:[^{}]*+{.*})?\\s*+}\\s*+'; $cssSplitForAllowedMediaTypes = \preg_split( - '#(@media\\s++(?:only\\s++)?+(?:(?=[{\\(])' . $mediaTypesExpression . ')' . $mediaRuleBodyMatcher + '#(@media\\s++(?:only\\s++)?+(?:(?=[{(])' . $mediaTypesExpression . ')' . $mediaRuleBodyMatcher . ')#misU', $css, -1, @@ -1318,8 +1323,7 @@ private function removeUnprocessableTags() } /** @var \DOMNode $node */ foreach ($nodes as $node) { - $hasContent = $node->hasChildNodes() || $node->hasChildNodes(); - if (!$hasContent) { + if (!$node->hasChildNodes()) { $node->parentNode->removeChild($node); } } @@ -1468,13 +1472,13 @@ static function (array $matches) { $trimmedLowercaseSelector, $matches ); - if (!$hasNotSelector) { - $xPath = '//' . $this->translateCssToXpathPass($trimmedLowercaseSelector); - } else { + if ($hasNotSelector) { /** @var string[] $matches */ list(, $partBeforeNot, $notContents) = $matches; $xPath = '//' . $this->translateCssToXpathPass($partBeforeNot) . '[not(' . $this->translateCssToXpathPassInline($notContents) . ')]'; + } else { + $xPath = '//' . $this->translateCssToXpathPass($trimmedLowercaseSelector); } $this->caches[self::CACHE_KEY_SELECTOR][$xPathKey] = $xPath; @@ -1579,10 +1583,8 @@ private function matchClassAttributes(array $match) private function matchClassAttributesInline(array $match) { return 'contains(concat(" ",@class," "),concat(" ","' . - \implode( - '"," "))][contains(concat(" ",@class," "),concat(" ","', - \explode('.', \substr($match[2], 1)) - ) . '"," "))'; + \str_replace('.', '"," "))][contains(concat(" ",@class," "),concat(" ","', \substr($match[2], 1)) . + '"," "))'; } /** @@ -1720,9 +1722,7 @@ private function parseCssDeclarationsBlock($cssDeclarationsBlock) } $properties = []; - $declarations = \preg_split('/;(?!base64|charset)/', $cssDeclarationsBlock); - - foreach ($declarations as $declaration) { + foreach (\preg_split('/;(?!base64|charset)/', $cssDeclarationsBlock) as $declaration) { $matches = []; if (!\preg_match('/^([A-Za-z\\-]+)\\s*:\\s*(.+)$/s', \trim($declaration), $matches)) { continue; diff --git a/src/Emogrifier/CssInliner.php b/src/Emogrifier/CssInliner.php index 38341439..681785aa 100644 --- a/src/Emogrifier/CssInliner.php +++ b/src/Emogrifier/CssInliner.php @@ -380,7 +380,7 @@ private function getAllNodesWithStyleAttribute() private function normalizeStyleAttributes(\DOMElement $node) { $normalizedOriginalStyle = \preg_replace_callback( - '/[A-z\\-]+(?=\\:)/S', + '/-?+[_a-zA-Z][\\w\\-]*+(?=:)/S', static function (array $m) { return \strtolower($m[0]); }, @@ -424,9 +424,7 @@ private function parseCssDeclarationsBlock($cssDeclarationsBlock) } $properties = []; - $declarations = \preg_split('/;(?!base64|charset)/', $cssDeclarationsBlock); - - foreach ($declarations as $declaration) { + foreach (\preg_split('/;(?!base64|charset)/', $cssDeclarationsBlock) as $declaration) { $matches = []; if (!\preg_match('/^([A-Za-z\\-]+)\\s*:\\s*(.+)$/s', \trim($declaration), $matches)) { continue; @@ -562,7 +560,7 @@ private function getCssSelectorConverter() * "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, e.g., "*" or "header h1"), - * "hasUnmatchablePseudo" (true if that selector contains psuedo-elements or dynamic pseudo-classes + * "hasUnmatchablePseudo" (true if that selector contains pseudo-elements or dynamic pseudo-classes * such that the declarations cannot be applied inline), * "declarationsBlock" (the semicolon-separated CSS declarations for that selector, * e.g., "color: red; height: 4px;"), @@ -589,8 +587,7 @@ private function parseCssRules($css) continue; } - $selectors = \explode(',', $cssRule['selectors']); - foreach ($selectors as $selector) { + foreach (\explode(',', $cssRule['selectors']) as $selector) { // don't process pseudo-elements and behavioral (dynamic) pseudo-classes; // only allow structural pseudo-classes $hasPseudoElement = \strpos($selector, '::') !== false; @@ -699,7 +696,7 @@ private function getCssRuleMatches($css) } /** - * Splits input CSS code into an array of parts for different media querues, in order. + * Splits input CSS code into an array of parts for different media queries, in order. * Each part is an array where: * * - key "css" will contain clean CSS code (for @media rules this will be the group rule body within "{...}") @@ -737,7 +734,7 @@ private function splitCssAndMediaQuery($css) $mediaRuleBodyMatcher = '[^{]*+{(?:[^{}]*+{.*})?\\s*+}\\s*+'; $cssSplitForAllowedMediaTypes = \preg_split( - '#(@media\\s++(?:only\\s++)?+(?:(?=[{\\(])' . $mediaTypesExpression . ')' . $mediaRuleBodyMatcher + '#(@media\\s++(?:only\\s++)?+(?:(?=[{(])' . $mediaTypesExpression . ')' . $mediaRuleBodyMatcher . ')#misU', $css, -1, @@ -1015,7 +1012,7 @@ private function removeUnmatchablePseudoComponents($selector) // The regex allows nested brackets via `(?2)`. // A space is temporarily prepended because the callback can't determine if the match was at the very start. $selectorWithoutNots = \ltrim(\preg_replace_callback( - '/(\\s?+):not(\\([^\\(\\)]*+(?:(?2)[^\\(\\)]*+)*+\\))/i', + '/(\\s?+):not(\\([^()]*+(?:(?2)[^()]*+)*+\\))/i', [$this, 'replaceUnmatchableNotComponent'], ' ' . $selector )); diff --git a/src/Emogrifier/HtmlProcessor/AbstractHtmlProcessor.php b/src/Emogrifier/HtmlProcessor/AbstractHtmlProcessor.php index 4dd92398..589898d5 100644 --- a/src/Emogrifier/HtmlProcessor/AbstractHtmlProcessor.php +++ b/src/Emogrifier/HtmlProcessor/AbstractHtmlProcessor.php @@ -295,6 +295,8 @@ private function ensurePhpUnrecognizedSelfClosingTagsAreXml($html) * Checks that $this->domDocument has a BODY element and adds it if it is missing. * * @return void + * + * @throws \UnexpectedValueException */ private function ensureExistenceOfBodyElement() { @@ -303,6 +305,9 @@ private function ensureExistenceOfBodyElement() } $htmlElement = $this->domDocument->getElementsByTagName('html')->item(0); + if ($htmlElement === null) { + throw new \UnexpectedValueException('There is no HTML element although there should be one.', 1569930853); + } $htmlElement->appendChild($this->domDocument->createElement('body')); } } diff --git a/src/Emogrifier/HtmlProcessor/CssToAttributeConverter.php b/src/Emogrifier/HtmlProcessor/CssToAttributeConverter.php index 8ff24289..3ea7937d 100644 --- a/src/Emogrifier/HtmlProcessor/CssToAttributeConverter.php +++ b/src/Emogrifier/HtmlProcessor/CssToAttributeConverter.php @@ -99,9 +99,7 @@ private function parseCssDeclarationsBlock($cssDeclarationsBlock) } $properties = []; - $declarations = \preg_split('/;(?!base64|charset)/', $cssDeclarationsBlock); - - foreach ($declarations as $declaration) { + foreach (\preg_split('/;(?!base64|charset)/', $cssDeclarationsBlock) as $declaration) { $matches = []; if (!\preg_match('/^([A-Za-z\\-]+)\\s*:\\s*(.+)$/s', \trim($declaration), $matches)) { continue; @@ -219,9 +217,9 @@ private function mapComplexCssProperty($property, $value, \DOMElement $node) private function mapBackgroundProperty(\DOMElement $node, $value) { // parse out the color, if any - $styles = \explode(' ', $value); + $styles = \explode(' ', $value, 2); $first = $styles[0]; - if (\is_numeric($first[0]) || \strpos($first, 'url') === 0) { + if (\is_numeric($first[0]) || \strncmp($first, 'url', 3) === 0) { return; } @@ -304,6 +302,7 @@ private function isTableOrImageNode(\DOMElement $node) */ private function parseCssShorthandValue($value) { + /** @var string[] $values */ $values = \preg_split('/\\s+/', $value); $css = []; diff --git a/src/Emogrifier/HtmlProcessor/HtmlPruner.php b/src/Emogrifier/HtmlProcessor/HtmlPruner.php index 10e1cbcd..59b8ef10 100644 --- a/src/Emogrifier/HtmlProcessor/HtmlPruner.php +++ b/src/Emogrifier/HtmlProcessor/HtmlPruner.php @@ -88,6 +88,7 @@ private function removeClassesFromElements(\DOMNodeList $elements, array $classe { $classesToKeepIntersector = new ArrayIntersector($classesToKeep); + /** @var \DOMNode $element */ foreach ($elements as $element) { $elementClasses = \preg_split('/\\s++/', \trim($element->getAttribute('class'))); $elementClassesToKeep = $classesToKeepIntersector->intersectWith($elementClasses); @@ -108,6 +109,7 @@ private function removeClassesFromElements(\DOMNodeList $elements, array $classe */ private function removeClassAttributeFromElements(\DOMNodeList $elements) { + /** @var \DOMNode $element */ foreach ($elements as $element) { $element->removeAttribute('class'); } diff --git a/tests/Support/Traits/AssertCss.php b/tests/Support/Traits/AssertCss.php index a99c7082..6ed804ba 100644 --- a/tests/Support/Traits/AssertCss.php +++ b/tests/Support/Traits/AssertCss.php @@ -11,7 +11,7 @@ trait AssertCss { /** * Processing of @media rules may involve removal of some unnecessary whitespace from the CSS placed in the