Fix the follow-ups from the #87–#89 review (#91–#96)

CHANGELOG.md

@@ -26,7 +26,12 @@ QueryPath Changelog
 - Fix `QPXML::createElement()` emitting an "Only variables should be passed by reference" notice for a namespace-prefixed element name without an explicit namespace URI (issue #82)
 - Fix the `:text` pseudo-class so it matches jQuery: it selects `input` elements whose `type` attribute is absent or is `text` (case-insensitively). It never indicated, and still does not indicate, whether a node is a text node
 - `find('*')` now matches the nodes in the match set as well as their descendants, so that a selector can be tested against an element already in hand
+- Fix `QPXML::createElement()` throwing "Undefined namespace" for a prefixed element name whose prefix is declared on the matched element or an ancestor below the document element. The prefix is now resolved against the namespaces in scope on the first matched element (issue #94)
 - `writeHTML5()` now returns the `DOMQuery` like the other `write*()` methods, and throws `QueryPath\IOException` instead of a `TypeError` when the file cannot be written (issue #80)
+- Fix `QueryPath\ParseException` and `QueryPath\IOException` losing the PHP warning they were raised from (issue #91). The message was the error number, as in `2 (/path/to/src/DOMQuery.php: 1343)`, and the code was always `0`. The message now carries the warning text and the code is the PHP error level (for example `E_WARNING`), so code that matches on the old message text will need updating
+- Fix `writeHTML()` throwing `QueryPath\ParseException` instead of `QueryPath\IOException` when the file cannot be written, as `writeXML()` and `writeHTML5()` do (issue #92). `IOException` extends `ParseException`, so existing `catch (ParseException $e)` blocks still catch it
+- Fix QueryPath's error handler staying installed for the rest of the request when an exception other than a `QueryPath\Exception` (a `TypeError`, or one thrown by a stream wrapper, for example) escaped from parsing a document, inserting markup, or `writeXML()`/`writeHTML()` (issue #95)
+- Fix `tag()` emitting an "Undefined property" warning and returning `NULL` when the first match is not an element, such as a text, comment or processing instruction node returned by `contents()`. It now returns an empty string, as it does for an empty match set (issue #93)
 
 # 4.1.0
 

src/DOM.php

@@ -177,9 +177,7 @@ private function parseXMLString($string, $flags = 0)
 	{
 		$document = self::createDocument('1.0');
 		$lead     = strtolower(substr($string, 0, 5)); // <?xml
-		try {
-			set_error_handler([ParseException::class, 'initializeFromError'], $this->errTypes);
-
+		self::withErrorHandler(ParseException::class, function () use ($document, $string, $flags, $lead) {
 			if (isset($this->options['convert_to_encoding'])) {
 				// Is there another way to do this?
 
@@ -217,12 +215,7 @@ private function parseXMLString($string, $flags = 0)
 			else {
 				self::loadHTMLString($document, $string);
 			}
-		} // Emulate 'finally' behavior.
-		catch (Exception $e) {
-			restore_error_handler();
-			throw $e;
-		}
-		restore_error_handler();
+		}, $this->errTypes);
 
 		if (empty($document)) {
 			throw new ParseException('Unknown parser exception.');
@@ -453,15 +446,9 @@ protected function prepareInsert($item)
 			}
 
 			$frag = $this->document->createDocumentFragment();
-			try {
-				set_error_handler([ParseException::class, 'initializeFromError'], $this->errTypes);
+			self::withErrorHandler(ParseException::class, function () use ($frag, $item) {
 				$frag->appendXML($item);
-			} // Simulate a finally block.
-			catch (Exception $e) {
-				restore_error_handler();
-				throw $e;
-			}
-			restore_error_handler();
+			}, $this->errTypes);
 
 			return $frag;
 		}
@@ -498,6 +485,28 @@ protected function prepareInsert($item)
 		throw new Exception('Cannot prepare item of unsupported type: ' . gettype($item));
 	}
 
+	/**
+	 * Run a callback with PHP errors converted into exceptions.
+	 *
+	 * @param string   $exceptionClass
+	 *  ParseException or a subclass, whose initializeFromError() handles the errors.
+	 * @param callable $callback
+	 * @param int      $types
+	 *  The error levels to convert.
+	 *
+	 * @return mixed
+	 *  Whatever the callback returns.
+	 */
+	protected static function withErrorHandler($exceptionClass, callable $callback, $types = E_ALL)
+	{
+		set_error_handler([$exceptionClass, 'initializeFromError'], $types);
+		try {
+			return $callback();
+		} finally {
+			restore_error_handler();
+		}
+	}
+
 	/**
 	 * Convenience function for getNthMatch(0).
 	 */
@@ -537,17 +546,9 @@ private function parseXMLFile($filename, $flags = 0, $context = null)
 		// If a context is specified, we basically have to do the reading in
 		// two steps:
 		if (! empty($context)) {
-			try {
-				set_error_handler(['\QueryPath\ParseException', 'initializeFromError'], $this->errTypes);
-				$contents = file_get_contents($filename, false, $context);
-			}
-				// Apparently there is no 'finally' in PHP, so we have to restore the error
-				// handler this way:
-			catch (Exception $e) {
-				restore_error_handler();
-				throw $e;
-			}
-			restore_error_handler();
+			$contents = self::withErrorHandler(ParseException::class, function () use ($filename, $context) {
+				return file_get_contents($filename, false, $context);
+			}, $this->errTypes);
 
 			if ($contents == false) {
 				throw new ParseException(sprintf(
@@ -576,27 +577,16 @@ private function parseXMLFile($filename, $flags = 0, $context = null)
 
 		$ext = $lastDot !== false ? strtolower(substr($filename, $lastDot)) : '';
 
-		try {
-			set_error_handler([ParseException::class, 'initializeFromError'], $this->errTypes);
+		// An explicit parser setting wins; otherwise go by the file extension, defaulting to XML.
+		$asHtml = $useParser === 'html' || ($useParser !== 'xml' && isset($htmlExtensions[$ext]));
 
-			// If the parser is explicitly set to XML, use that parser.
-			if ($useParser === 'xml') {
-				$document->load($filename, $flags);
-			} // Otherwise, see if it looks like HTML.
-			elseif ($useParser === 'html' || isset($htmlExtensions[$ext])) {
-				// Try parsing it as HTML.
+		self::withErrorHandler(ParseException::class, function () use ($document, $filename, $flags, $asHtml) {
+			if ($asHtml) {
 				self::loadHTMLFile($document, $filename);
-			} // Default to XML.
-			else {
+			} else {
 				$document->load($filename, $flags);
 			}
-
-		} // Emulate 'finally' behavior.
-		catch (Exception $e) {
-			restore_error_handler();
-			throw $e;
-		}
-		restore_error_handler();
+		}, $this->errTypes);
 
 		return $document;
 	}

src/DOMQuery.php

@@ -14,6 +14,7 @@
 namespace QueryPath;
 
 use DOMDocument;
+use DOMElement;
 use DOMNode;
 use DOMXPath;
 use QueryPath\CSS\DOMTraverser;
@@ -547,7 +548,9 @@ public function index($subject)
 	 * The tag name of the first element in the list.
 	 *
 	 * This returns the tag name of the first element in the list of matches. If
-	 * the list is empty, an empty string will be used.
+	 * the list is empty, or the first match is not an element (for example, a
+	 * text, comment or processing instruction node returned by contents()), an
+	 * empty string will be used.
 	 *
 	 * @return string
 	 *  The tag name of the first element in the list.
@@ -556,7 +559,9 @@ public function index($subject)
 	 */
 	public function tag()
 	{
-		return ($this->matches->count() > 0) ? $this->getFirstMatch()->tagName : '';
+		$first = ($this->matches->count() > 0) ? $this->getFirstMatch() : null;
+
+		return ($first instanceof DOMElement) ? $first->tagName : '';
 	}
 
 	/**
@@ -1338,14 +1343,9 @@ public function writeXML($path = null, $options = 0)
 		if ($path === null) {
 			print $this->document->saveXML(null, $options);
 		} else {
-			try {
-				set_error_handler([IOException::class, 'initializeFromError']);
+			self::withErrorHandler(IOException::class, function () use ($path, $options) {
 				$this->document->save($path, $options);
-			} catch (Exception $e) {
-				restore_error_handler();
-				throw $e;
-			}
-			restore_error_handler();
+			});
 		}
 
 		return $this;
@@ -1376,14 +1376,9 @@ public function writeHTML($path = null)
 		if ($path === null) {
 			print $this->saveDocumentHTML();
 		} else {
-			try {
-				set_error_handler(['\QueryPath\ParseException', 'initializeFromError']);
+			self::withErrorHandler(IOException::class, function () use ($path) {
 				$this->saveDocumentHTML($path);
-			} catch (Exception $e) {
-				restore_error_handler();
-				throw $e;
-			}
-			restore_error_handler();
+			});
 		}
 
 		return $this;
@@ -1413,12 +1408,9 @@ public function writeHTML5($path = null)
 			// Print the document to stdout.
 			print $html5->saveHTML($this->document);
 		} else {
-			set_error_handler([IOException::class, 'initializeFromError']);
-			try {
+			self::withErrorHandler(IOException::class, function () use ($html5, $path) {
 				$html5->save($this->document, $path);
-			} finally {
-				restore_error_handler();
-			}
+			});
 		}
 
 		return $this;

src/Extension/QPXML.php

@@ -73,18 +73,7 @@ public function cdata($text = null)
 			return $this->qp;
 		}
 
-		// Look for CDATA sections.
-		foreach ($this->qp->get() as $ele) {
-			foreach ($ele->childNodes as $node) {
-				if ($node->nodeType === XML_CDATA_SECTION_NODE) {
-					// Return first match.
-					return $node->textContent;
-				}
-			}
-		}
-
-		return null;
-		// Nothing found
+		return $this->firstChildText(XML_CDATA_SECTION_NODE);
 	}
 
 	/**
@@ -117,18 +106,32 @@ public function comment($text = null)
 
 			return $this->qp;
 		}
-		foreach ($this->qp->get() as $ele) {
-			foreach ($ele->childNodes as $node) {
-				if ($node->nodeType == XML_COMMENT_NODE) {
-					// Return first match.
-					return $node->textContent;
-				}
-			}
-		}
+
+		return $this->firstChildText(XML_COMMENT_NODE);
 	}
 
 	/**
-	 * Get or set a processor instruction.
+	 * Get or set a processing instruction.
+	 *
+	 * If $text is passed in (and is not NULL), this will add a processing instruction
+	 * with the target $prefix and the data $text to every match in the set.
+	 *
+	 * If no text is passed in, this will return the data of the first processing
+	 * instruction in the set of matches, limited to those with the target $prefix
+	 * if one is given. If none is found, NULL will be returned.
+	 *
+	 * @param string $prefix
+	 *  The target of the processing instruction (the `xml-stylesheet` in
+	 *  `<?xml-stylesheet ...?>`). When getting, NULL matches any target.
+	 * @param string $text
+	 *  The data of the processing instruction. If set, a new processing instruction
+	 *  will be created in every item wrapped by the current {@link QueryPath}.
+	 *
+	 * @return mixed
+	 *  If $text is set, this will return a {@link QueryPath}. If no text is set, this
+	 *  will return the string value of the first matching processing instruction, or
+	 *  NULL if none is found.
+	 * @see comment()
 	 */
 	public function pi($prefix = null, $text = null)
 	{
@@ -140,13 +143,31 @@ public function pi($prefix = null, $text = null)
 
 			return $this->qp;
 		}
+
+		return $this->firstChildText(XML_PI_NODE, $prefix);
+	}
+
+	/**
+	 * Get the text of the first child node of the given type in the set of matches.
+	 *
+	 * @param int $nodeType
+	 *  One of the XML_*_NODE constants.
+	 * @param string $target
+	 *  If set, only processing instructions with this target match.
+	 *
+	 * @return string|null
+	 */
+	private function firstChildText($nodeType, $target = null)
+	{
 		foreach ($this->qp->get() as $ele) {
 			foreach ($ele->childNodes as $node) {
-				if ($node->nodeType == XML_PI_NODE && ($prefix === null || $node->target === $prefix)) {
+				if ($node->nodeType === $nodeType && ($target === null || $node->target === $target)) {
 					return $node->textContent;
 				}
-			} // foreach
-		} // foreach
+			}
+		}
+
+		return null;
 	}
 
 	public function toXml()
@@ -174,40 +195,41 @@ public function createNilElement($text, $value)
 	/**
 	 * Create an element with the given namespace.
 	 *
+	 * The element is created in the document of the first matched element. If
+	 * $text has a prefix and no $nsUri is given, the prefix is resolved against
+	 * the namespaces in scope on that element.
+	 *
 	 * @param string $text
 	 * @param string $nsUri
 	 *   The namespace URI for the given element.
 	 *
 	 * @return DOMQuery
+	 *   The new element, or an empty DOMQuery if there are no matches.
+	 * @throws Exception
+	 *   If the prefix of $text is not bound to a namespace on the first match.
 	 */
 	public function createElement($text, $nsUri = null)
 	{
-		if (isset($text)) {
-			foreach ($this->qp->get() as $element) {
-				if ($nsUri === null && strpos($text, ':') !== false) {
-					$ns    = strstr($text, ':', true);
-					$nsUri = $element->ownerDocument->lookupNamespaceURI($ns);
-
-					if ($nsUri === null) {
-						throw new Exception("Undefined namespace for: " . $text);
-					}
-				}
+		$element = $this->qp->get(0);
+		if (! isset($text) || $element === null) {
+			return new DOMQuery();
+		}
 
-				$node = null;
-				if ($nsUri !== null) {
-					$node = $element->ownerDocument->createElementNS(
-						$nsUri,
-						$text
-					);
-				} else {
-					$node = $element->ownerDocument->createElement($text);
-				}
+		if ($nsUri === null && strpos($text, ':') !== false) {
+			$nsUri = $element->lookupNamespaceURI(strstr($text, ':', true));
 
-				return QueryPath::with($node);
+			if ($nsUri === null) {
+				throw new Exception("Undefined namespace for: " . $text);
 			}
 		}
 
-		return new DOMQuery();
+		if ($nsUri !== null) {
+			$node = $element->ownerDocument->createElementNS($nsUri, $text);
+		} else {
+			$node = $element->ownerDocument->createElement($text);
+		}
+
+		return QueryPath::with($node);
 	}
 
 	/**

src/IOException.php

@@ -14,10 +14,4 @@
  */
 class IOException extends ParseException
 {
-
-	public static function initializeFromError($errno, $errstr, $errfile, $errline, $context = null)
-	{
-		$class = __CLASS__;
-		throw new $class($errno, (int) $errstr, $errfile, $errline);
-	}
 }

src/ParseException.php

@@ -44,7 +44,6 @@ public function __construct($msg = '', $code = 0, $file = null, $line = null)
 
 	public static function initializeFromError($errno, $errstr, $errfile, $errline, $context = null)
 	{
-		$class = __CLASS__;
-		throw new $class($errno, (int) $errstr, $errfile, $errline);
+		throw new static($errstr, $errno, $errfile, $errline);
 	}
 }