Move xml_free_parser out of destructor.
authorBrian Wolff <bawolff+wn@gmail.com>
Mon, 25 May 2015 21:21:59 +0000 (15:21 -0600)
committerBryanDavis <bdavis@wikimedia.org>
Tue, 2 Jun 2015 17:45:33 +0000 (17:45 +0000)
Attempt to reduce error log spam. I'm not sure about the exact
cause of this error, nor have I really reproduced locally, so
I don't know if this will help, but getting it out of destructor
seems like a good idea.

I also do not know if calling xml_free_parser is really needed
(Would php automatically destroy parser during garbage collection?)

Bug: T89532
Change-Id: I5709b6cf26cf4a9032a79d2b5f48ed3b4238eaee

includes/media/XMP.php

index 042f749..6b36e37 100644 (file)
@@ -155,16 +155,27 @@ class XMPReader implements LoggerAwareInterface {
                $this->logger = $logger;
        }
 
+       /**
+        * free the XML parser.
+        *
+        * @note It is unclear to me if we really need to do this ourselves
+        *  or if php garbage collection will automatically free the xmlParser
+        *  when it is no longer needed.
+        */
+       private function destroyXMLParser() {
+               if ( $this->xmlParser ) {
+                       xml_parser_free( $this->xmlParser );
+                       $this->xmlParser = null;
+               }
+       }
+
        /**
         * Main use is if a single item has multiple xmp documents describing it.
         * For example in jpeg's with extendedXMP
         */
        private function resetXMLParser() {
 
-               if ( $this->xmlParser ) {
-                       //is this needed?
-                       xml_parser_free( $this->xmlParser );
-               }
+               $this->destroyXMLParser();
 
                $this->xmlParser = xml_parser_create_ns( 'UTF-8', ' ' );
                xml_parser_set_option( $this->xmlParser, XML_OPTION_CASE_FOLDING, 0 );
@@ -180,15 +191,6 @@ class XMPReader implements LoggerAwareInterface {
                $this->xmlParsableBuffer = '';
        }
 
-       /** Destroy the xml parser
-        *
-        * Not sure if this is actually needed.
-        */
-       function __destruct() {
-               // not sure if this is needed.
-               xml_parser_free( $this->xmlParser );
-       }
-
        /**
         * Check if this instance supports using this class
         */
@@ -294,12 +296,11 @@ class XMPReader implements LoggerAwareInterface {
         *
         * @param string $content XMP data
         * @param bool $allOfIt If this is all the data (true) or if its split up (false). Default true
-        * @param bool $reset Does xml parser need to be reset. Default false
         * @throws RuntimeException
         * @return bool Success.
         */
-       public function parse( $content, $allOfIt = true, $reset = false ) {
-               if ( $reset ) {
+       public function parse( $content, $allOfIt = true ) {
+               if ( !$this->xmlParser ) {
                        $this->resetXMLParser();
                }
                try {
@@ -373,14 +374,21 @@ class XMPReader implements LoggerAwareInterface {
 
                                $this->logger->info( "XMPReader::parse : Error reading XMP content: $error ($where)" );
                                $this->results = array(); // blank if error.
+                               $this->destroyXMLParser();
                                return false;
                        }
                } catch ( Exception $e ) {
                        $this->logger->info( 'XMP parse error: ' . $e );
                        $this->results = array();
 
+                       if ( $allOfIt ) {
+                               $this->destroyXMLParser();
+                       }
                        return false;
                }
+               if ( $allOfIt ) {
+                       $this->destroyXMLParser();
+               }
 
                return true;
        }