jenkins-bot has submitted this change and it was merged. Change subject: XMP: Use structured logging instead of wfDebugLog ......................................................................
XMP: Use structured logging instead of wfDebugLog Co-Authored-By: Brian Wolff <[email protected]> Change-Id: I486192a718576a4d1e585ffb390e297b14dde087 --- M includes/media/BitmapMetadataHandler.php M includes/media/XMP.php M includes/media/XMPInfo.php M includes/media/XMPValidate.php M tests/phpunit/includes/media/XMPValidateTest.php 5 files changed, 103 insertions(+), 56 deletions(-) Approvals: BryanDavis: Looks good to me, approved jenkins-bot: Verified diff --git a/includes/media/BitmapMetadataHandler.php b/includes/media/BitmapMetadataHandler.php index c8d37bb..a5cddac 100644 --- a/includes/media/BitmapMetadataHandler.php +++ b/includes/media/BitmapMetadataHandler.php @@ -21,6 +21,8 @@ * @ingroup Media */ +use MediaWiki\Logger\LoggerFactory; + /** * Class to deal with reconciling and extracting metadata from bitmap images. * This is meant to comply with http://www.metadataworkinggroup.org/pdf/mwg_guidance.pdf @@ -167,7 +169,7 @@ } } if ( isset( $seg['XMP'] ) && $showXMP ) { - $xmp = new XMPReader(); + $xmp = new XMPReader( LoggerFactory::getInstance( 'XMP' ) ); $xmp->parse( $seg['XMP'] ); foreach ( $seg['XMP_ext'] as $xmpExt ) { /* Support for extended xmp in jpeg files @@ -203,7 +205,7 @@ if ( isset( $array['text']['xmp']['x-default'] ) && $array['text']['xmp']['x-default'] !== '' && $showXMP ) { - $xmp = new XMPReader(); + $xmp = new XMPReader( LoggerFactory::getInstance( 'XMP' ) ); $xmp->parse( $array['text']['xmp']['x-default'] ); $xmpRes = $xmp->getResults(); foreach ( $xmpRes as $type => $xmpSection ) { @@ -237,7 +239,7 @@ } if ( $baseArray['xmp'] !== '' && XMPReader::isSupported() ) { - $xmp = new XMPReader(); + $xmp = new XMPReader( LoggerFactory::getInstance( 'XMP' ) ); $xmp->parse( $baseArray['xmp'] ); $xmpRes = $xmp->getResults(); foreach ( $xmpRes as $type => $xmpSection ) { diff --git a/includes/media/XMP.php b/includes/media/XMP.php index 957ddd2..042f749 100644 --- a/includes/media/XMP.php +++ b/includes/media/XMP.php @@ -21,6 +21,10 @@ * @ingroup Media */ +use Psr\Log\LoggerAwareInterface; +use Psr\Log\LoggerInterface; +use Psr\Log\NullLogger; + /** * Class for reading xmp data containing properties relevant to * images, and spitting out an array that FormatMetadata accepts. @@ -46,7 +50,7 @@ * read rdf. * */ -class XMPReader { +class XMPReader implements LoggerAwareInterface { /** @var array XMP item configuration array */ protected $items; @@ -121,20 +125,34 @@ const PARSABLE_NO = 3; /** + * @var LoggerInterface + */ + private $logger; + + /** * Constructor. * * Primary job is to initialize the XMLParser */ - function __construct() { + function __construct( LoggerInterface $logger = null ) { if ( !function_exists( 'xml_parser_create_ns' ) ) { // this should already be checked by this point throw new RuntimeException( 'XMP support requires XML Parser' ); } + if ( $logger ) { + $this->setLogger( $logger ); + } else { + $this->setLogger( new NullLogger() ); + } $this->items = XMPInfo::getItems(); $this->resetXMLParser(); + } + + public function setLogger( LoggerInterface $logger ) { + $this->logger = $logger; } /** @@ -353,12 +371,12 @@ . ' column: ' . xml_get_current_column_number( $this->xmlParser ) . ' byte offset: ' . xml_get_current_byte_index( $this->xmlParser ); - wfDebugLog( 'XMP', "XMPReader::parse : Error reading XMP content: $error ($where)" ); + $this->logger->info( "XMPReader::parse : Error reading XMP content: $error ($where)" ); $this->results = array(); // blank if error. return false; } } catch ( Exception $e ) { - wfDebugLog( 'XMP', 'XMP parse error: ' . $e ); + $this->logger->info( 'XMP parse error: ' . $e ); $this->results = array(); return false; @@ -381,7 +399,7 @@ if ( !isset( $this->results['xmp-special']['HasExtendedXMP'] ) || $this->results['xmp-special']['HasExtendedXMP'] !== $guid ) { - wfDebugLog( 'XMP', __METHOD__ . + $this->logger->info( __METHOD__ . " Ignoring XMPExtended block due to wrong guid (guid= '$guid')" ); return false; @@ -389,7 +407,7 @@ $len = unpack( 'Nlength/Noffset', substr( $content, 32, 8 ) ); if ( !$len || $len['length'] < 4 || $len['offset'] < 0 || $len['offset'] > $len['length'] ) { - wfDebugLog( 'XMP', __METHOD__ . 'Error reading extended XMP block, invalid length or offset.' ); + $this->logger->info( __METHOD__ . 'Error reading extended XMP block, invalid length or offset.' ); return false; } @@ -406,7 +424,7 @@ // > 128k, and be in the wrong order is very low... if ( $len['offset'] !== $this->extendedXMPOffset ) { - wfDebugLog( 'XMP', __METHOD__ . 'Ignoring XMPExtended block due to wrong order. (Offset was ' + $this->logger->info( __METHOD__ . 'Ignoring XMPExtended block due to wrong order. (Offset was ' . $len['offset'] . ' but expected ' . $this->extendedXMPOffset . ')' ); return false; @@ -428,7 +446,7 @@ $atEnd = false; } - wfDebugLog( 'XMP', __METHOD__ . 'Parsing a XMPExtended block' ); + $this->logger->debug( __METHOD__ . 'Parsing a XMPExtended block' ); return $this->parse( $actualContent, $atEnd ); } @@ -617,23 +635,27 @@ $finalName = isset( $info['map_name'] ) ? $info['map_name'] : $tag; - $validate = is_array( $info['validate'] ) ? $info['validate'] - : array( 'XMPValidate', $info['validate'] ); + if ( is_array( $info['validate'] ) ) { + $validate = $info['validate']; + } else { + $validator = new XMPValidate( $this->logger ); + $validate = array( $validator, $info['validate'] ); + } if ( !isset( $this->results['xmp-' . $info['map_group']][$finalName] ) ) { // This can happen if all the members of the struct failed validation. - wfDebugLog( 'XMP', __METHOD__ . " <$ns:$tag> has no valid members." ); + $this->logger->debug( __METHOD__ . " <$ns:$tag> has no valid members." ); } elseif ( is_callable( $validate ) ) { $val =& $this->results['xmp-' . $info['map_group']][$finalName]; call_user_func_array( $validate, array( $info, &$val, false ) ); if ( is_null( $val ) ) { // the idea being the validation function will unset the variable if // its invalid. - wfDebugLog( 'XMP', __METHOD__ . " <$ns:$tag> failed validation." ); + $this->logger->info( __METHOD__ . " <$ns:$tag> failed validation." ); unset( $this->results['xmp-' . $info['map_group']][$finalName] ); } } else { - wfDebugLog( 'XMP', __METHOD__ . " Validation function for $finalName (" + $this->logger->warning( __METHOD__ . " Validation function for $finalName (" . $validate[0] . '::' . $validate[1] . '()) is not callable.' ); } } @@ -674,7 +696,7 @@ array_shift( $this->mode ); if ( !isset( $this->results['xmp-' . $info['map_group']][$finalName] ) ) { - wfDebugLog( 'XMP', __METHOD__ . " Empty compund element $finalName." ); + $this->logger->debug( __METHOD__ . " Empty compund element $finalName." ); return; } @@ -741,7 +763,7 @@ if ( $elm === self::NS_RDF . ' type' ) { // these aren't really supported properly yet. // However, it appears they almost never used. - wfDebugLog( 'XMP', __METHOD__ . ' encountered <rdf:type>' ); + $this->logger->info( __METHOD__ . ' encountered <rdf:type>' ); } if ( strpos( $elm, ' ' ) === false ) { @@ -749,7 +771,7 @@ // However, there is a bug in an adobe product // that forgets the namespace on some things. // (Luckily they are unimportant things). - wfDebugLog( 'XMP', __METHOD__ . " Encountered </$elm> which has no namespace. Skipping." ); + $this->logger->info( __METHOD__ . " Encountered </$elm> which has no namespace. Skipping." ); return; } @@ -795,7 +817,7 @@ $this->endElementModeQDesc( $elm ); break; default: - wfDebugLog( 'XMP', __METHOD__ . " no mode (elm = $elm)" ); + $this->logger->warning( __METHOD__ . " no mode (elm = $elm)" ); break; } } @@ -845,7 +867,7 @@ array_unshift( $this->mode, self::MODE_LI ); } elseif ( $elm === self::NS_RDF . ' Bag' ) { # bug 27105 - wfDebugLog( 'XMP', __METHOD__ . ' Expected an rdf:Seq, but got an rdf:Bag. Pretending' + $this->logger->info( __METHOD__ . ' Expected an rdf:Seq, but got an rdf:Bag. Pretending' . ' it is a Seq, since some buggy software is known to screw this up.' ); array_unshift( $this->mode, self::MODE_LI ); } else { @@ -908,7 +930,7 @@ throw new RuntimeException( __METHOD__ . ' Encountered <rdf:value> where it was unexpected.' ); } else { // something else we don't recognize, like a qualifier maybe. - wfDebugLog( 'XMP', __METHOD__ . + $this->logger->info( __METHOD__ . " Encountered element <$elm> where only expecting character data as value of " . $this->curItem[0] ); array_unshift( $this->mode, self::MODE_IGNORE ); @@ -962,7 +984,7 @@ // a child of a struct), then something weird is // happening, so ignore this element and its children. - wfDebugLog( 'XMP', "Encountered <$ns:$tag> outside" + $this->logger->warning( "Encountered <$ns:$tag> outside" . " of its expected parent. Ignoring." ); array_unshift( $this->mode, self::MODE_IGNORE ); @@ -984,7 +1006,7 @@ } } else { // This element is not on our list of allowed elements so ignore. - wfDebugLog( 'XMP', __METHOD__ . " Ignoring unrecognized element <$ns:$tag>." ); + $this->logger->debug( __METHOD__ . " Ignoring unrecognized element <$ns:$tag>." ); array_unshift( $this->mode, self::MODE_IGNORE ); array_unshift( $this->curItem, $ns . ' ' . $tag ); @@ -1164,12 +1186,12 @@ // // also it seems as if exiv2 and exiftool do not support // this either (That or I misunderstand the standard) - wfDebugLog( 'XMP', __METHOD__ . ' Encountered <rdf:type> which isn\'t currently supported' ); + $this->logger->info( __METHOD__ . ' Encountered <rdf:type> which isn\'t currently supported' ); } if ( strpos( $elm, ' ' ) === false ) { // This probably shouldn't happen. - wfDebugLog( 'XMP', __METHOD__ . " Encountered <$elm> which has no namespace. Skipping." ); + $this->logger->info( __METHOD__ . " Encountered <$elm> which has no namespace. Skipping." ); return; } @@ -1251,7 +1273,7 @@ if ( strpos( $name, ' ' ) === false ) { // This shouldn't happen, but so far some old software forgets namespace // on rdf:about. - wfDebugLog( 'XMP', __METHOD__ . ' Encountered non-namespaced attribute: ' + $this->logger->info( __METHOD__ . ' Encountered non-namespaced attribute: ' . " $name=\"$val\". Skipping. " ); continue; } @@ -1269,7 +1291,7 @@ } $this->saveValue( $ns, $tag, $val ); } else { - wfDebugLog( 'XMP', __METHOD__ . " Ignoring unrecognized element <$ns:$tag>." ); + $this->logger->debug( __METHOD__ . " Ignoring unrecognized element <$ns:$tag>." ); } } } @@ -1291,20 +1313,24 @@ $finalName = isset( $info['map_name'] ) ? $info['map_name'] : $tag; if ( isset( $info['validate'] ) ) { - $validate = is_array( $info['validate'] ) ? $info['validate'] - : array( 'XMPValidate', $info['validate'] ); + if ( is_array( $info['validate'] ) ) { + $validate = $info['validate']; + } else { + $validator = new XMPValidate( $this->logger ); + $validate = array( $validator, $info['validate'] ); + } if ( is_callable( $validate ) ) { call_user_func_array( $validate, array( $info, &$val, true ) ); // the reasoning behind using &$val instead of using the return value // is to be consistent between here and validating structures. if ( is_null( $val ) ) { - wfDebugLog( 'XMP', __METHOD__ . " <$ns:$tag> failed validation." ); + $this->logger->info( __METHOD__ . " <$ns:$tag> failed validation." ); return; } } else { - wfDebugLog( 'XMP', __METHOD__ . " Validation function for $finalName (" + $this->logger->warning( __METHOD__ . " Validation function for $finalName (" . $validate[0] . '::' . $validate[1] . '()) is not callable.' ); } } diff --git a/includes/media/XMPInfo.php b/includes/media/XMPInfo.php index 2a31001..97aa796 100644 --- a/includes/media/XMPInfo.php +++ b/includes/media/XMPInfo.php @@ -50,7 +50,7 @@ * * mode - What type of item (self::MODE_SIMPLE usually, see above for * all values). * * validate - Method to validate input. Could also post-process the - * input. A string value is assumed to be a static method of + * input. A string value is assumed to be a method of * XMPValidate. Can also take a array( 'className', 'methodName' ). * * choices - Array of potential values (format of 'value' => true ). * Only used with validateClosed. diff --git a/includes/media/XMPValidate.php b/includes/media/XMPValidate.php index 0fa6011..55e8ce7 100644 --- a/includes/media/XMPValidate.php +++ b/includes/media/XMPValidate.php @@ -21,6 +21,9 @@ * @ingroup Media */ +use Psr\Log\LoggerInterface; +use Psr\Log\LoggerAwareInterface; + /** * This contains some static methods for * validating XMP properties. See XMPInfo and XMPReader classes. @@ -40,7 +43,20 @@ * @see http://www.adobe.com/devnet/xmp/pdfs/XMPSpecificationPart1.pdf starting at pg 28 * @see http://www.adobe.com/devnet/xmp/pdfs/XMPSpecificationPart2.pdf starting at pg 11 */ -class XMPValidate { +class XMPValidate implements LoggerAwareInterface { + + /** + * @var LoggerInterface + */ + private $logger; + + public function __construct( LoggerInterface $logger ) { + $this->setLogger( $logger ); + } + + public function setLogger( LoggerInterface $logger ) { + $this->logger = $logger; + } /** * Function to validate boolean properties ( True or False ) * @@ -48,13 +64,13 @@ * @param mixed &$val Current value to validate * @param bool $standalone If this is a simple property or array */ - public static function validateBoolean( $info, &$val, $standalone ) { + public function validateBoolean( $info, &$val, $standalone ) { if ( !$standalone ) { // this only validates standalone properties, not arrays, etc return; } if ( $val !== 'True' && $val !== 'False' ) { - wfDebugLog( 'XMP', __METHOD__ . " Expected True or False but got $val" ); + $this->debug->info( __METHOD__ . " Expected True or False but got $val" ); $val = null; } } @@ -66,13 +82,13 @@ * @param mixed &$val Current value to validate * @param bool $standalone If this is a simple property or array */ - public static function validateRational( $info, &$val, $standalone ) { + public function validateRational( $info, &$val, $standalone ) { if ( !$standalone ) { // this only validates standalone properties, not arrays, etc return; } if ( !preg_match( '/^(?:-?\d+)\/(?:\d+[1-9]|[1-9]\d*)$/D', $val ) ) { - wfDebugLog( 'XMP', __METHOD__ . " Expected rational but got $val" ); + $this->logger->info( __METHOD__ . " Expected rational but got $val" ); $val = null; } } @@ -87,7 +103,7 @@ * @param mixed &$val Current value to validate * @param bool $standalone If this is a simple property or array */ - public static function validateRating( $info, &$val, $standalone ) { + public function validateRating( $info, &$val, $standalone ) { if ( !$standalone ) { // this only validates standalone properties, not arrays, etc return; @@ -95,7 +111,7 @@ if ( !preg_match( '/^[-+]?\d*(?:\.?\d*)$/D', $val ) || !is_numeric( $val ) ) { - wfDebugLog( 'XMP', __METHOD__ . " Expected rating but got $val" ); + $this->logger->info( __METHOD__ . " Expected rating but got $val" ); $val = null; return; @@ -105,13 +121,13 @@ // We do < 0 here instead of < -1 here, since // the values between 0 and -1 are also illegal // as -1 is meant as a special reject rating. - wfDebugLog( 'XMP', __METHOD__ . " Rating too low, setting to -1 (Rejected)" ); + $this->logger->info( __METHOD__ . " Rating too low, setting to -1 (Rejected)" ); $val = '-1'; return; } if ( $nVal > 5 ) { - wfDebugLog( 'XMP', __METHOD__ . " Rating too high, setting to 5" ); + $this->logger->info( __METHOD__ . " Rating too high, setting to 5" ); $val = '5'; return; @@ -126,13 +142,13 @@ * @param mixed &$val Current value to validate * @param bool $standalone If this is a simple property or array */ - public static function validateInteger( $info, &$val, $standalone ) { + public function validateInteger( $info, &$val, $standalone ) { if ( !$standalone ) { // this only validates standalone properties, not arrays, etc return; } if ( !preg_match( '/^[-+]?\d+$/D', $val ) ) { - wfDebugLog( 'XMP', __METHOD__ . " Expected integer but got $val" ); + $this->logger->info( __METHOD__ . " Expected integer but got $val" ); $val = null; } } @@ -145,7 +161,7 @@ * @param mixed &$val Current value to validate * @param bool $standalone If this is a simple property or array */ - public static function validateClosed( $info, &$val, $standalone ) { + public function validateClosed( $info, &$val, $standalone ) { if ( !$standalone ) { // this only validates standalone properties, not arrays, etc return; @@ -163,7 +179,7 @@ } if ( !isset( $info['choices'][$val] ) && !$inRange ) { - wfDebugLog( 'XMP', __METHOD__ . " Expected closed choice, but got $val" ); + $this->logger->info( __METHOD__ . " Expected closed choice, but got $val" ); $val = null; } } @@ -175,7 +191,7 @@ * @param mixed &$val Current value to validate * @param bool $standalone If this is a simple property or array */ - public static function validateFlash( $info, &$val, $standalone ) { + public function validateFlash( $info, &$val, $standalone ) { if ( $standalone ) { // this only validates flash structs, not individual properties return; @@ -186,7 +202,7 @@ && isset( $val['RedEyeMode'] ) && isset( $val['Return'] ) ) ) { - wfDebugLog( 'XMP', __METHOD__ . " Flash structure did not have all the required components" ); + $this->logger->info( __METHOD__ . " Flash structure did not have all the required components" ); $val = null; } else { $val = ( "\0" | ( $val['Fired'] === 'True' ) @@ -209,14 +225,14 @@ * @param mixed &$val Current value to validate * @param bool $standalone If this is a simple property or array */ - public static function validateLangCode( $info, &$val, $standalone ) { + public function validateLangCode( $info, &$val, $standalone ) { if ( !$standalone ) { // this only validates standalone properties, not arrays, etc return; } if ( !preg_match( '/^[-A-Za-z0-9]{2,}$/D', $val ) ) { //this is a rather naive check. - wfDebugLog( 'XMP', __METHOD__ . " Expected Lang code but got $val" ); + $this->logger->info( __METHOD__ . " Expected Lang code but got $val" ); $val = null; } } @@ -238,7 +254,7 @@ * 2011:04. * @param bool $standalone If this is a simple property or array */ - public static function validateDate( $info, &$val, $standalone ) { + public function validateDate( $info, &$val, $standalone ) { if ( !$standalone ) { // this only validates standalone properties, not arrays, etc return; @@ -252,7 +268,7 @@ ) { // @codingStandardsIgnoreEnd - wfDebugLog( 'XMP', __METHOD__ . " Expected date but got $val" ); + $this->logger->info( __METHOD__ . " Expected date but got $val" ); $val = null; } else { /* @@ -270,7 +286,7 @@ * some programs convert between metadata formats. */ if ( $res[1] === '0000' ) { - wfDebugLog( 'XMP', __METHOD__ . " Invalid date (year 0): $val" ); + $this->logger->info( __METHOD__ . " Invalid date (year 0): $val" ); $val = null; return; @@ -339,7 +355,7 @@ * or DDD,MM.mmk form * @param bool $standalone If its a simple prop (should always be true) */ - public static function validateGPS( $info, &$val, $standalone ) { + public function validateGPS( $info, &$val, $standalone ) { if ( !$standalone ) { return; } @@ -371,7 +387,7 @@ return; } else { - wfDebugLog( 'XMP', __METHOD__ + $this->logger->info( __METHOD__ . " Expected GPSCoordinate, but got $val." ); $val = null; diff --git a/tests/phpunit/includes/media/XMPValidateTest.php b/tests/phpunit/includes/media/XMPValidateTest.php index ebec8f6..53671d4 100644 --- a/tests/phpunit/includes/media/XMPValidateTest.php +++ b/tests/phpunit/includes/media/XMPValidateTest.php @@ -1,5 +1,7 @@ <?php +use Psr\Log\NullLogger; + /** * @group Media */ @@ -11,7 +13,8 @@ */ public function testValidateDate( $value, $expected ) { // The method should modify $value. - XMPValidate::validateDate( array(), $value, true ); + $validate = new XMPValidate( new NullLogger() ); + $validate->validateDate( array(), $value, true ); $this->assertEquals( $expected, $value ); } -- To view, visit https://gerrit.wikimedia.org/r/213011 To unsubscribe, visit https://gerrit.wikimedia.org/r/settings Gerrit-MessageType: merged Gerrit-Change-Id: I486192a718576a4d1e585ffb390e297b14dde087 Gerrit-PatchSet: 11 Gerrit-Project: mediawiki/core Gerrit-Branch: master Gerrit-Owner: Legoktm <[email protected]> Gerrit-Reviewer: Aaron Schulz <[email protected]> Gerrit-Reviewer: Brian Wolff <[email protected]> Gerrit-Reviewer: BryanDavis <[email protected]> Gerrit-Reviewer: jenkins-bot <> _______________________________________________ MediaWiki-commits mailing list [email protected] https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
