r90256 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r90255‎ | r90256 | r90257 >
Date:03:37, 17 June 2011
Author:bawolff
Status:ok (Comments)
Tags:
Comment:
(follow-up r86567) per CR rename the class JpegOrTiffHandler to ExifBitmapHandler.
Modified paths:
  • /trunk/phase3/RELEASE-NOTES-1.19 (modified) (history)
  • /trunk/phase3/includes/AutoLoader.php (modified) (history)
  • /trunk/phase3/includes/media/ExifBitmap.php (added) (history)
  • /trunk/phase3/includes/media/Jpeg.php (modified) (history)
  • /trunk/phase3/includes/media/JpegOrTiff.php (deleted) (history)
  • /trunk/phase3/includes/media/Tiff.php (modified) (history)

Diff [purge]

Index: trunk/phase3/RELEASE-NOTES-1.19
——@@ -95,7 +95,8 @@
9696 as lastTabIndex().
9797 * (bug 29332) Warn if user requests mediawiki-announce subscription but does not
9898 enter an e-mail address.
99 -* (bug 25375) Add canonical namespaces to JavaScript "wgNamespaceIds"
 99+* (bug 25375) Add canonical namespaces to JavaScript "wgNamespaceIds"
 100+* The class JpegOrTiffHandler was renamed ExifBitmapHandler.
100101
101102 === API changes in 1.19 ===
102103 * BREAKING CHANGE: action=watch now requires POST and token.
Index: trunk/phase3/includes/media/JpegOrTiff.php
——@@ -1,127 +0,0 @@
2 -<?php
3 -/**
4 - * @file
5 - * @ingroup Media
6 - */
7 -
8 -/**
9 - * Stuff specific to JPEG and (built-in) TIFF handler.
10 - * All metadata related, since both JPEG and TIFF support Exif.
11 - *
12 - * @ingroup Media
13 - */
14 -class JpegOrTiffHandler extends BitmapHandler {
15 -
16 - const BROKEN_FILE = '-1'; // error extracting metadata
17 - const OLD_BROKEN_FILE = '0'; // outdated error extracting metadata.
18 -
19 - function convertMetadataVersion( $metadata, $version = 1 ) {
20 - // basically flattens arrays.
21 - $version = explode(';', $version, 2);
22 - $version = intval($version[0]);
23 - if ( $version < 1 || $version >= 2 ) {
24 - return $metadata;
25 - }
26 -
27 - $avoidHtml = true;
28 -
29 - if ( !is_array( $metadata ) ) {
30 - $metadata = unserialize( $metadata );
31 - }
32 - if ( !isset( $metadata['MEDIAWIKI_EXIF_VERSION'] ) || $metadata['MEDIAWIKI_EXIF_VERSION'] != 2 ) {
33 - return $metadata;
34 - }
35 -
36 - // Treat Software as a special case because in can contain
37 - // an array of (SoftwareName, Version).
38 - if (isset( $metadata['Software'] )
39 - && is_array( $metadata['Software'] )
40 - && is_array( $metadata['Software'][0])
41 - && isset( $metadata['Software'][0][0] )
42 - && isset( $metadata['Software'][0][1])
43 - ) {
44 - $metadata['Software'] = $metadata['Software'][0][0] . ' (Version '
45 - . $metadata['Software'][0][1] . ')';
46 - }
47 -
48 - // ContactInfo also has to be dealt with specially
49 - if ( isset( $metadata['Contact'] ) ) {
50 - $metadata['Contact'] =
51 - FormatMetadata::collapseContactInfo(
52 - $metadata['Contact'] );
53 - }
54 -
55 - foreach ( $metadata as &$val ) {
56 - if ( is_array( $val ) ) {
57 - $val = FormatMetadata::flattenArray( $val, 'ul', $avoidHtml );
58 - }
59 - }
60 - $metadata['MEDIAWIKI_EXIF_VERSION'] = 1;
61 - return $metadata;
62 - }
63 -
64 - function isMetadataValid( $image, $metadata ) {
65 - global $wgShowEXIF;
66 - if ( !$wgShowEXIF ) {
67 - # Metadata disabled and so an empty field is expected
68 - return self::METADATA_GOOD;
69 - }
70 - if ( $metadata === self::OLD_BROKEN_FILE ) {
71 - # Old special value indicating that there is no EXIF data in the file.
72 - # or that there was an error well extracting the metadata.
73 - wfDebug( __METHOD__ . ": back-compat version\n");
74 - return self::METADATA_COMPATIBLE;
75 - }
76 - if ( $metadata === self::BROKEN_FILE ) {
77 - return self::METADATA_GOOD;
78 - }
79 - wfSuppressWarnings();
80 - $exif = unserialize( $metadata );
81 - wfRestoreWarnings();
82 - if ( !isset( $exif['MEDIAWIKI_EXIF_VERSION'] ) ||
83 - $exif['MEDIAWIKI_EXIF_VERSION'] != Exif::version() )
84 - {
85 - if ( isset( $exif['MEDIAWIKI_EXIF_VERSION'] ) &&
86 - $exif['MEDIAWIKI_EXIF_VERSION'] == 1 )
87 - {
88 - //back-compatible but old
89 - wfDebug( __METHOD__.": back-compat version\n" );
90 - return self::METADATA_COMPATIBLE;
91 - }
92 - # Wrong (non-compatible) version
93 - wfDebug( __METHOD__.": wrong version\n" );
94 - return self::METADATA_BAD;
95 - }
96 - return self::METADATA_GOOD;
97 - }
98 -
99 - /**
100 - * @param $image File
101 - * @return array|bool
102 - */
103 - function formatMetadata( $image ) {
104 - $metadata = $image->getMetadata();
105 - if ( !$metadata ||
106 - $this->isMetadataValid( $image, $metadata ) === self::METADATA_BAD )
107 - {
108 - // So we don't try and display metadata from PagedTiffHandler
109 - // for example when using InstantCommons.
110 - return false;
111 - }
112 -
113 - $exif = unserialize( $metadata );
114 - if ( !$exif ) {
115 - return false;
116 - }
117 - unset( $exif['MEDIAWIKI_EXIF_VERSION'] );
118 - if ( count( $exif ) == 0 ) {
119 - return false;
120 - }
121 - return $this->formatMetadataHelper( $exif );
122 - }
123 -
124 - function getMetadataType( $image ) {
125 - return 'exif';
126 - }
127 -}
128 -
Index: trunk/phase3/includes/media/Jpeg.php
——@@ -8,11 +8,12 @@
99 * JPEG specific handler.
1010 * Inherits most stuff from BitmapHandler, just here to do the metadata handler differently.
1111 *
12 - * Metadata stuff common to Jpeg and built-in Tiff (not PagedTiffHandler) is in JpegOrTiffHandler.
 12+ * Metadata stuff common to Jpeg and built-in Tiff (not PagedTiffHandler) is
 13+ * in ExifBitmapHandler.
1314 *
1415 * @ingroup Media
1516 */
16 -class JpegHandler extends JpegOrTiffHandler {
 17+class JpegHandler extends ExifBitmapHandler {
1718
1819 function getMetadata ( $image, $filename ) {
1920 try {
——@@ -28,7 +29,7 @@
2930 // BitmapMetadataHandler throws an exception in certain exceptional cases like if file does not exist.
3031 wfDebug( __METHOD__ . ': ' . $e->getMessage() . "\n" );
3132
32 - /* This used to use 0 (JpegOrTiffHandler::OLD_BROKEN_FILE) for the cases
 33+ /* This used to use 0 (ExifBitmapHandler::OLD_BROKEN_FILE) for the cases
3334 * * No metadata in the file
3435 * * Something is broken in the file.
3536 * However, if the metadata support gets expanded then you can't tell if the 0 is from
——@@ -37,7 +38,7 @@
3839 * Thus switch to using -1 to denote only a broken file, and use an array with only
3940 * MEDIAWIKI_EXIF_VERSION to denote no props.
4041 */
41 - return JpegOrTiffHandler::BROKEN_FILE;
 42+ return ExifBitmapHandler::BROKEN_FILE;
4243 }
4344 }
4445
Index: trunk/phase3/includes/media/ExifBitmap.php
——@@ -0,0 +1,127 @@
 2+<?php
 3+/**
 4+ * @file
 5+ * @ingroup Media
 6+ */
 7+
 8+/**
 9+ * Stuff specific to JPEG and (built-in) TIFF handler.
 10+ * All metadata related, since both JPEG and TIFF support Exif.
 11+ *
 12+ * @ingroup Media
 13+ */
 14+class ExifBitmapHandler extends BitmapHandler {
 15+
 16+ const BROKEN_FILE = '-1'; // error extracting metadata
 17+ const OLD_BROKEN_FILE = '0'; // outdated error extracting metadata.
 18+
 19+ function convertMetadataVersion( $metadata, $version = 1 ) {
 20+ // basically flattens arrays.
 21+ $version = explode(';', $version, 2);
 22+ $version = intval($version[0]);
 23+ if ( $version < 1 || $version >= 2 ) {
 24+ return $metadata;
 25+ }
 26+
 27+ $avoidHtml = true;
 28+
 29+ if ( !is_array( $metadata ) ) {
 30+ $metadata = unserialize( $metadata );
 31+ }
 32+ if ( !isset( $metadata['MEDIAWIKI_EXIF_VERSION'] ) || $metadata['MEDIAWIKI_EXIF_VERSION'] != 2 ) {
 33+ return $metadata;
 34+ }
 35+
 36+ // Treat Software as a special case because in can contain
 37+ // an array of (SoftwareName, Version).
 38+ if (isset( $metadata['Software'] )
 39+ && is_array( $metadata['Software'] )
 40+ && is_array( $metadata['Software'][0])
 41+ && isset( $metadata['Software'][0][0] )
 42+ && isset( $metadata['Software'][0][1])
 43+ ) {
 44+ $metadata['Software'] = $metadata['Software'][0][0] . ' (Version '
 45+ . $metadata['Software'][0][1] . ')';
 46+ }
 47+
 48+ // ContactInfo also has to be dealt with specially
 49+ if ( isset( $metadata['Contact'] ) ) {
 50+ $metadata['Contact'] =
 51+ FormatMetadata::collapseContactInfo(
 52+ $metadata['Contact'] );
 53+ }
 54+
 55+ foreach ( $metadata as &$val ) {
 56+ if ( is_array( $val ) ) {
 57+ $val = FormatMetadata::flattenArray( $val, 'ul', $avoidHtml );
 58+ }
 59+ }
 60+ $metadata['MEDIAWIKI_EXIF_VERSION'] = 1;
 61+ return $metadata;
 62+ }
 63+
 64+ function isMetadataValid( $image, $metadata ) {
 65+ global $wgShowEXIF;
 66+ if ( !$wgShowEXIF ) {
 67+ # Metadata disabled and so an empty field is expected
 68+ return self::METADATA_GOOD;
 69+ }
 70+ if ( $metadata === self::OLD_BROKEN_FILE ) {
 71+ # Old special value indicating that there is no EXIF data in the file.
 72+ # or that there was an error well extracting the metadata.
 73+ wfDebug( __METHOD__ . ": back-compat version\n");
 74+ return self::METADATA_COMPATIBLE;
 75+ }
 76+ if ( $metadata === self::BROKEN_FILE ) {
 77+ return self::METADATA_GOOD;
 78+ }
 79+ wfSuppressWarnings();
 80+ $exif = unserialize( $metadata );
 81+ wfRestoreWarnings();
 82+ if ( !isset( $exif['MEDIAWIKI_EXIF_VERSION'] ) ||
 83+ $exif['MEDIAWIKI_EXIF_VERSION'] != Exif::version() )
 84+ {
 85+ if ( isset( $exif['MEDIAWIKI_EXIF_VERSION'] ) &&
 86+ $exif['MEDIAWIKI_EXIF_VERSION'] == 1 )
 87+ {
 88+ //back-compatible but old
 89+ wfDebug( __METHOD__.": back-compat version\n" );
 90+ return self::METADATA_COMPATIBLE;
 91+ }
 92+ # Wrong (non-compatible) version
 93+ wfDebug( __METHOD__.": wrong version\n" );
 94+ return self::METADATA_BAD;
 95+ }
 96+ return self::METADATA_GOOD;
 97+ }
 98+
 99+ /**
 100+ * @param $image File
 101+ * @return array|bool
 102+ */
 103+ function formatMetadata( $image ) {
 104+ $metadata = $image->getMetadata();
 105+ if ( !$metadata ||
 106+ $this->isMetadataValid( $image, $metadata ) === self::METADATA_BAD )
 107+ {
 108+ // So we don't try and display metadata from PagedTiffHandler
 109+ // for example when using InstantCommons.
 110+ return false;
 111+ }
 112+
 113+ $exif = unserialize( $metadata );
 114+ if ( !$exif ) {
 115+ return false;
 116+ }
 117+ unset( $exif['MEDIAWIKI_EXIF_VERSION'] );
 118+ if ( count( $exif ) == 0 ) {
 119+ return false;
 120+ }
 121+ return $this->formatMetadataHelper( $exif );
 122+ }
 123+
 124+ function getMetadataType( $image ) {
 125+ return 'exif';
 126+ }
 127+}
 128+
Property changes on: trunk/phase3/includes/media/ExifBitmap.php
___________________________________________________________________
Added: svn:mergeinfo
1129 Merged /branches/REL1_15/phase3/includes/media/Jpeg.php:r51646
2130 Merged /branches/sqlite/includes/media/Jpeg.php:r58211-58321
3131 Merged /branches/new-installer/phase3/includes/media/Jpeg.php:r43664-66004
4132 Merged /branches/wmf-deployment/includes/media/Jpeg.php:r53381
Added: svn:eol-style
5133 + native
Index: trunk/phase3/includes/media/Tiff.php
——@@ -11,7 +11,7 @@
1212 *
1313 * @ingroup Media
1414 */
15 -class TiffHandler extends JpegOrTiffHandler {
 15+class TiffHandler extends ExifBitmapHandler {
1616
1717 /**
1818 * Conversion to PNG for inline display can be disabled here...
——@@ -63,7 +63,7 @@
6464 $data['MEDIAWIKI_EXIF_VERSION'] = Exif::version();
6565 return serialize( $data );
6666 } else {
67 - return JpegOrTiffHandler::BROKEN_FILE;
 67+ return ExifBitmapHandler::BROKEN_FILE;
6868 }
6969 } else {
7070 return '';
Index: trunk/phase3/includes/AutoLoader.php
——@@ -528,7 +528,7 @@
529529 'IPTC' => 'includes/media/IPTC.php',
530530 'JpegHandler' => 'includes/media/Jpeg.php',
531531 'JpegMetadataExtractor' => 'includes/media/JpegMetadataExtractor.php',
532 - 'JpegOrTiffHandler' => 'includes/media/JpegOrTiff.php',
 532+ 'ExifBitmapHandler' => 'includes/media/ExifBitmap.php',
533533 'MediaHandler' => 'includes/media/Generic.php',
534534 'MediaTransformError' => 'includes/media/MediaTransformOutput.php',
535535 'MediaTransformOutput' => 'includes/media/MediaTransformOutput.php',

Follow-up revisions

RevisionCommit summaryAuthorDate
r91885(follow-up r90256) Unit tests.bawolff16:42, 11 July 2011

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r86567Remove the JPEG/TIFF specific metadata code from BitmapHandler and put it in ...bawolff23:15, 20 April 2011

Comments

#Comment by Brion VIBBER (talk | contribs)   21:31, 21 June 2011

Needs phpunit test cases to confirm the refactor causes no trouble.

#Comment by Bawolff (talk | contribs)   21:54, 21 June 2011

I assume you mean the refactor of moving the exif handling to a superclass of the Tiff and Jpeg classes (r86567), not the rename of the class from JpegOrTiffHandler to ExifBitmapHandler?

#Comment by Brion VIBBER (talk | contribs)   23:06, 21 June 2011

The actual name change isn't terribly exciting on its own, but the whole bunch of code-rearrangement should be double-checked by having tests that actually run the code. :)

#Comment by Bawolff (talk | contribs)   16:43, 11 July 2011

Added tests in r91885 - resetting to new and removing keyword.

#Comment by Hashar (talk | contribs)   14:57, 18 July 2011

Bump: we need to have this revision and the unit tests reviewed :-)

Status & tagging log