From 2109cc15c6098a0e49cbdb2c9457e3e2f714e150 Mon Sep 17 00:00:00 2001 From: Oliver Klee Date: Fri, 9 Apr 2021 20:32:54 +0200 Subject: [PATCH 1/3] [CLEANUP] Fix Psalm warnings in AbstractHtmlProcessor --- psalm.baseline.xml | 9 ---- src/CssInliner.php | 12 ++--- src/HtmlProcessor/AbstractHtmlProcessor.php | 45 ++++++++++++------- src/HtmlProcessor/CssToAttributeConverter.php | 2 +- src/HtmlProcessor/HtmlPruner.php | 4 +- 5 files changed, 40 insertions(+), 32 deletions(-) diff --git a/psalm.baseline.xml b/psalm.baseline.xml index 26b6f9fd..fc01cd42 100644 --- a/psalm.baseline.xml +++ b/psalm.baseline.xml @@ -82,15 +82,6 @@ string[] - - - null - - - new static() - new static() - - $mapping['attribute'] diff --git a/src/CssInliner.php b/src/CssInliner.php index da7e0775..5964153a 100644 --- a/src/CssInliner.php +++ b/src/CssInliner.php @@ -200,7 +200,8 @@ public function inlineCss(string $css = ''): self $cssSelectorConverter = $this->getCssSelectorConverter(); foreach ($cssRules['inlinable'] as $cssRule) { try { - $nodesMatchingCssSelectors = $this->xPath->query($cssSelectorConverter->toXPath($cssRule['selector'])); + $nodesMatchingCssSelectors = $this->getXPath() + ->query($cssSelectorConverter->toXPath($cssRule['selector'])); } catch (ParseException $e) { if ($this->debug) { throw $e; @@ -398,7 +399,7 @@ private function normalizeStyleAttributesOfAllNodes(): void */ private function getAllNodesWithStyleAttribute(): \DOMNodeList { - return $this->xPath->query('//*[@style]'); + return $this->getXPath()->query('//*[@style]'); } /** @@ -475,7 +476,7 @@ private function parseCssDeclarationsBlock(string $cssDeclarationsBlock): array */ private function getCssFromAllStyleNodes(): string { - $styleNodes = $this->xPath->query('//style'); + $styleNodes = $this->getXPath()->query('//style'); if ($styleNodes === false) { return ''; } @@ -608,7 +609,8 @@ private function getNodesToExclude(): array $excludedNodes = []; foreach (\array_keys($this->excludedSelectors) as $selectorToExclude) { try { - $matchingNodes = $this->xPath->query($this->getCssSelectorConverter()->toXPath($selectorToExclude)); + $matchingNodes = $this->getXPath() + ->query($this->getCssSelectorConverter()->toXPath($selectorToExclude)); } catch (ParseException $e) { if ($this->debug) { throw $e; @@ -1110,7 +1112,7 @@ private function existsMatchForSelectorInCssRule(array $cssRule): bool private function existsMatchForCssSelector(string $cssSelector): bool { try { - $nodesMatchingSelector = $this->xPath->query($this->getCssSelectorConverter()->toXPath($cssSelector)); + $nodesMatchingSelector = $this->getXPath()->query($this->getCssSelectorConverter()->toXPath($cssSelector)); } catch (ParseException $e) { if ($this->debug) { throw $e; diff --git a/src/HtmlProcessor/AbstractHtmlProcessor.php b/src/HtmlProcessor/AbstractHtmlProcessor.php index f29b9f3e..5a636d7f 100644 --- a/src/HtmlProcessor/AbstractHtmlProcessor.php +++ b/src/HtmlProcessor/AbstractHtmlProcessor.php @@ -60,16 +60,16 @@ abstract class AbstractHtmlProcessor protected $domDocument = null; /** - * @var \DOMXPath + * @var \DOMXPath|null */ - protected $xPath = null; + private $xPath = null; /** * The constructor. * * Please use `::fromHtml` or `::fromDomDocument` instead. */ - private function __construct() + final private function __construct() { } @@ -128,15 +128,9 @@ private function setHtml(string $html): void */ public function getDomDocument(): \DOMDocument { - if ($this->domDocument === null) { - throw new \UnexpectedValueException( - ( - self::class . - '::setDomDocument() has not yet been called on ' . - static::class - ), - 1570472239 - ); + if (!$this->domDocument instanceof \DOMDocument) { + $message = self::class . '::setDomDocument() has not yet been called on ' . static::class; + throw new \UnexpectedValueException($message, 1570472239); } return $this->domDocument; @@ -151,6 +145,20 @@ private function setDomDocument(\DOMDocument $domDocument): void $this->xPath = new \DOMXPath($this->domDocument); } + /** + * @return \DOMXPath + * + * @throws \BadMethodCallException + */ + protected function getXPath(): \DOMXPath + { + if (!$this->xPath instanceof \DOMXPath) { + throw new \BadMethodCallException('$this->xPath has not been initialized yet.', 1617819086); + } + + return $this->xPath; + } + /** * Renders the normalized and processed HTML. * @@ -194,10 +202,17 @@ private function removeSelfClosingTagsClosingTags(string $html): string * This method assumes that there always is a BODY element. * * @return \DOMElement + * + * @throws \RuntimeException */ private function getBodyElement(): \DOMElement { - return $this->getDomDocument()->getElementsByTagName('body')->item(0); + $node = $this->getDomDocument()->getElementsByTagName('body')->item(0); + if (!$node instanceof \DOMElement) { + throw new \RuntimeException('There is no body element.', 1617922607); + } + + return $node; } /** @@ -441,12 +456,12 @@ private function ensurePhpUnrecognizedSelfClosingTagsAreXml(string $html): strin */ private function ensureExistenceOfBodyElement(): void { - if ($this->getDomDocument()->getElementsByTagName('body')->item(0) !== null) { + if ($this->getDomDocument()->getElementsByTagName('body')->item(0) instanceof \DOMElement) { return; } $htmlElement = $this->getDomDocument()->getElementsByTagName('html')->item(0); - if ($htmlElement === null) { + if (!$htmlElement instanceof \DOMElement) { throw new \UnexpectedValueException('There is no HTML element although there should be one.', 1569930853); } $htmlElement->appendChild($this->getDomDocument()->createElement('body')); diff --git a/src/HtmlProcessor/CssToAttributeConverter.php b/src/HtmlProcessor/CssToAttributeConverter.php index a7b26072..c1464de2 100644 --- a/src/HtmlProcessor/CssToAttributeConverter.php +++ b/src/HtmlProcessor/CssToAttributeConverter.php @@ -70,7 +70,7 @@ public function convertCssToVisualAttributes(): self */ private function getAllNodesWithStyleAttribute(): \DOMNodeList { - return $this->xPath->query('//*[@style]'); + return $this->getXPath()->query('//*[@style]'); } /** diff --git a/src/HtmlProcessor/HtmlPruner.php b/src/HtmlProcessor/HtmlPruner.php index a322179e..5330a9bb 100644 --- a/src/HtmlProcessor/HtmlPruner.php +++ b/src/HtmlProcessor/HtmlPruner.php @@ -31,7 +31,7 @@ class HtmlPruner extends AbstractHtmlProcessor */ public function removeElementsWithDisplayNone(): self { - $elementsWithStyleDisplayNone = $this->xPath->query(self::DISPLAY_NONE_MATCHER); + $elementsWithStyleDisplayNone = $this->getXPath()->query(self::DISPLAY_NONE_MATCHER); if ($elementsWithStyleDisplayNone->length === 0) { return $this; } @@ -61,7 +61,7 @@ public function removeElementsWithDisplayNone(): self */ public function removeRedundantClasses(array $classesToKeep = []): self { - $elementsWithClassAttribute = $this->xPath->query('//*[@class]'); + $elementsWithClassAttribute = $this->getXPath()->query('//*[@class]'); if ($classesToKeep !== []) { $this->removeClassesFromElements($elementsWithClassAttribute, $classesToKeep); From eb40b6e49f642564296429d77cedc6e2169de37f Mon Sep 17 00:00:00 2001 From: Oliver Klee Date: Sun, 11 Apr 2021 10:49:56 +0200 Subject: [PATCH 2/3] Changes after review - use `?ClassName` instead of `ClassName|null` in annotations - mention class names in exception messages from `getXpath()` - make the constructor protected --- src/HtmlProcessor/AbstractHtmlProcessor.php | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/HtmlProcessor/AbstractHtmlProcessor.php b/src/HtmlProcessor/AbstractHtmlProcessor.php index 5a636d7f..6a4f0d67 100644 --- a/src/HtmlProcessor/AbstractHtmlProcessor.php +++ b/src/HtmlProcessor/AbstractHtmlProcessor.php @@ -55,12 +55,12 @@ abstract class AbstractHtmlProcessor = '%][^<]*+(?:<(?!/template>)[^<]*+)*+(?:|$)%i'; /** - * @var \DOMDocument|null + * @var ?\DOMDocument */ protected $domDocument = null; /** - * @var \DOMXPath|null + * @var ?\DOMXPath */ private $xPath = null; @@ -69,7 +69,7 @@ abstract class AbstractHtmlProcessor * * Please use `::fromHtml` or `::fromDomDocument` instead. */ - final private function __construct() + final protected function __construct() { } @@ -148,12 +148,13 @@ private function setDomDocument(\DOMDocument $domDocument): void /** * @return \DOMXPath * - * @throws \BadMethodCallException + * @throws \UnexpectedValueException */ protected function getXPath(): \DOMXPath { if (!$this->xPath instanceof \DOMXPath) { - throw new \BadMethodCallException('$this->xPath has not been initialized yet.', 1617819086); + $message = self::class . '::setDomDocument() has not yet been called on ' . static::class; + throw new \UnexpectedValueException($message, 1617819086); } return $this->xPath; From cc45c29156866b90b28d06aa86d447f78a7d1b8c Mon Sep 17 00:00:00 2001 From: Oliver Klee Date: Sun, 11 Apr 2021 17:30:23 +0200 Subject: [PATCH 3/3] Add a psalm annotation for the `AbstractHtmlProcessor` constructor --- src/HtmlProcessor/AbstractHtmlProcessor.php | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/HtmlProcessor/AbstractHtmlProcessor.php b/src/HtmlProcessor/AbstractHtmlProcessor.php index 6a4f0d67..2e203642 100644 --- a/src/HtmlProcessor/AbstractHtmlProcessor.php +++ b/src/HtmlProcessor/AbstractHtmlProcessor.php @@ -8,6 +8,8 @@ * Base class for HTML processor that e.g., can remove, add or modify nodes or attributes. * * The "vanilla" subclass is the HtmlNormalizer. + * + * @psalm-consistent-constructor */ abstract class AbstractHtmlProcessor { @@ -69,7 +71,7 @@ abstract class AbstractHtmlProcessor * * Please use `::fromHtml` or `::fromDomDocument` instead. */ - final protected function __construct() + private function __construct() { }