[CLEANUP] Add CssInliner::querySelectorAll method

This `private` method provides functionality that is used in three separate places (albeit a one-liner), and includes taking care of exception handling according to the debug mode setting. The new method and parameter names match the equivalent in the [Web API]( https://developer.mozilla.org/en-US/docs/Web/API/Document/querySelectorAll ). There will be a tiny performance impact due to additional method calls. But the benefits include code re-use and common error handling. Also add `ensureNodeIsElement` method for use with the result from `querySelectorAll` to ensure type safety. `querySelectorAll` should always return a `DOMNodeList` of `DOMElement`s. But this cannot be guaranteed if there is a bug in CssSelector whereby it returns an XPath expression which may match non-element nodes. And it would (probably) be sub-optimal for `querySelectorAll` to have an extra loop checking this, when it can be done in the caller's loop over the returned data.
MyIntervals · Sep 18, 2024 · dbf26c9 · dbf26c9
1 parent 597eaf2
commit dbf26c9
Show file tree

Hide file tree

Showing 2 changed files with 77 additions and 37 deletions.
diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon
@@ -1,7 +1,7 @@
 parameters:
 	ignoreErrors:
 		-
-			message: "#^Argument of an invalid type DOMNodeList\\<DOMNode\\>\\|false supplied for foreach, only iterables are supported\\.$#"
+			message: "#^Argument of an invalid type array\\<int, string\\>\\|false supplied for foreach, only iterables are supported\\.$#"
 			count: 2
 			path: src/CssInliner.php
 

diff --git a/src/CssInliner.php b/src/CssInliner.php
@@ -53,6 +53,13 @@ class CssInliner extends AbstractHtmlProcessor
      */
     private const COMBINATOR_MATCHER = '(?:\\s++|\\s*+[>+~]\\s*+)(?=[[:alpha:]_\\-.#*:\\[])';
 
+    /**
+     * options array key for `querySelectorAll`
+     *
+     * @var string
+     */
+    private const QSA_ALWAYS_THROW_PARSE_EXCEPTION = 'alwaysThrowParseException';
+
     /**
      * @var array<string, bool>
      */
@@ -164,7 +171,11 @@ class CssInliner extends AbstractHtmlProcessor
      * @return $this
      *
      * @throws ParseException in debug mode, if an invalid selector is encountered
-     * @throws \RuntimeException in debug mode, if an internal PCRE error occurs
+     * @throws \RuntimeException
+     *         in debug mode, if an internal PCRE error occurs
+     *         or `CssSelectorConverter::toXPath` returns an invalid XPath expression
+     * @throws \UnexpectedValueException
+     *         if a selector query result includes a node which is not a `DOMElement`
      */
     public function inlineCss(string $css = ''): self
     {
@@ -183,23 +194,12 @@ public function inlineCss(string $css = ''): self
 
         $excludedNodes = $this->getNodesToExclude();
         $cssRules = $this->collateCssRules($parsedCss);
-        $cssSelectorConverter = $this->getCssSelectorConverter();
         foreach ($cssRules['inlinable'] as $cssRule) {
-            try {
-                $nodesMatchingCssSelectors = $this->getXPath()
-                    ->query($cssSelectorConverter->toXPath($cssRule['selector']));
-
-                /** @var \DOMElement $node */
-                foreach ($nodesMatchingCssSelectors as $node) {
-                    if (\in_array($node, $excludedNodes, true)) {
-                        continue;
-                    }
-                    $this->copyInlinableCssToStyleAttribute($node, $cssRule);
-                }
-            } catch (ParseException $e) {
-                if ($this->debug) {
-                    throw $e;
+            foreach ($this->querySelectorAll($cssRule['selector']) as $node) {
+                if (\in_array($node, $excludedNodes, true)) {
+                    continue;
                 }
+                $this->copyInlinableCssToStyleAttribute($this->ensureNodeIsElement($node), $cssRule);
             }
         }
 
@@ -496,34 +496,72 @@ private function getCssFromAllStyleNodes(): string
      *
      * @return list<\DOMElement>
      *
-     * @throws ParseException
-     * @throws \UnexpectedValueException
+     * @throws ParseException in debug mode, if an invalid selector is encountered
+     * @throws \RuntimeException in debug mode, if `CssSelectorConverter::toXPath` returns an invalid XPath expression
+     * @throws \UnexpectedValueException if the selector query result includes a node which is not a `DOMElement`
      */
     private function getNodesToExclude(): array
     {
         $excludedNodes = [];
         foreach (\array_keys($this->excludedSelectors) as $selectorToExclude) {
-            try {
-                $matchingNodes = $this->getXPath()
-                    ->query($this->getCssSelectorConverter()->toXPath($selectorToExclude));
-
-                foreach ($matchingNodes as $node) {
-                    if (!$node instanceof \DOMElement) {
-                        $path = $node->getNodePath() ?? '$node';
-                        throw new \UnexpectedValueException($path . ' is not a DOMElement.', 1617975914);
-                    }
-                    $excludedNodes[] = $node;
-                }
-            } catch (ParseException $e) {
-                if ($this->debug) {
-                    throw $e;
-                }
+            foreach ($this->querySelectorAll($selectorToExclude) as $node) {
+                $excludedNodes[] = $this->ensureNodeIsElement($node);
             }
         }
 
         return $excludedNodes;
     }
 
+    /**
+     * @param array{}|array{alwaysThrowParseException: bool} $options
+     *        This is an array of option values to control behaviour:
+     *        - `QSA_ALWAYS_THROW_PARSE_EXCEPTION` - `bool` - throw any `ParseException` regardless of debug setting.
+     *
+     * @return \DOMNodeList<\DOMNode> the HTML elements that match the provided CSS `$selectors`
+     *
+     * @throws ParseException
+     *         in debug mode (or with `QSA_ALWAYS_THROW_PARSE_EXCEPTION` option), if an invalid selector is encountered
+     * @throws \RuntimeException in debug mode, if `CssSelectorConverter::toXPath` returns an invalid XPath expression
+     */
+    private function querySelectorAll(string $selectors, array $options = []): \DOMNodeList
+    {
+        try {
+            $result = $this->getXPath()->query($this->getCssSelectorConverter()->toXPath($selectors));
+
+            if ($result === false) {
+                throw new \RuntimeException('query failed with selector \'' . $selectors . '\'', 1726533051);
+            }
+
+            return $result;
+        } catch (ParseException|\RuntimeException $exception) {
+            if (
+                $this->debug
+                || ($options[self::QSA_ALWAYS_THROW_PARSE_EXCEPTION] ?? false) && $exception instanceof ParseException
+            ) {
+                throw $exception;
+            }
+            // In non-debug mode, `ParseException`s are silently ignored, whereas `RuntimeException`s (indicating a bug
+            // in CssSelector) have their message passed to the error handler (for logging or custom handling).
+            if ($exception instanceof \RuntimeException) {
+                \trigger_error($exception->getMessage());
+            }
+            return new \DOMNodeList();
+        }
+    }
+
+    /**
+     * @throws \UnexpectedValueException if `$node` is not a `DOMElement`
+     */
+    private function ensureNodeIsElement(\DOMNode $node): \DOMElement
+    {
+        if (!$node instanceof \DOMElement) {
+            $path = $node->getNodePath() ?? '$node';
+            throw new \UnexpectedValueException($path . ' is not a DOMElement.', 1617975914);
+        }
+
+        return $node;
+    }
+
     /**
      * @return CssSelectorConverter
      */
@@ -952,20 +990,22 @@ private function existsMatchForSelectorInCssRule(array $cssRule): bool
      *
      * @return bool
      *
-     * @throws ParseException
+     * @throws ParseException in debug mode, if an invalid selector is encountered
+     * @throws \RuntimeException in debug mode, if `CssSelectorConverter::toXPath` returns an invalid XPath expression
      */
     private function existsMatchForCssSelector(string $cssSelector): bool
     {
         try {
-            $nodesMatchingSelector = $this->getXPath()->query($this->getCssSelectorConverter()->toXPath($cssSelector));
+            $nodesMatchingSelector
+                = $this->querySelectorAll($cssSelector, [self::QSA_ALWAYS_THROW_PARSE_EXCEPTION => true]);
         } catch (ParseException $e) {
             if ($this->debug) {
                 throw $e;
             }
             return true;
         }
 
-        return $nodesMatchingSelector !== false && $nodesMatchingSelector->length !== 0;
+        return $nodesMatchingSelector->length !== 0;
     }
 
     /**