r103029 MediaWiki - Code Review archive

Repository:	MediaWiki
Revision:	< r103028‎ \| r103029 \| r103030 >
Date:	20:58, 14 November 2011
Author:	reedy
Status:	ok
Tags:
Comment:	REL1_18 MFT r99236, r102301, r102303, r102498, r102710, r102751, r102951, r102954
Modified paths:	/branches/REL1_18/phase3 (modified) (history) /branches/REL1_18/phase3/CREDITS (modified) (history) /branches/REL1_18/phase3/includes (modified) (history) /branches/REL1_18/phase3/includes/AutoLoader.php (modified) (history) /branches/REL1_18/phase3/includes/GlobalFunctions.php (modified) (history) /branches/REL1_18/phase3/includes/LinksUpdate.php (modified) (history) /branches/REL1_18/phase3/includes/api (modified) (history) /branches/REL1_18/phase3/includes/api/ApiQueryBase.php (modified) (history) /branches/REL1_18/phase3/includes/api/ApiQueryExternalLinks.php (modified) (history) /branches/REL1_18/phase3/includes/installer/DatabaseUpdater.php (modified) (history) /branches/REL1_18/phase3/includes/media/ExifBitmap.php (modified) (history) /branches/REL1_18/phase3/maintenance/fixExtLinksProtocolRelative.php (added) (history) /branches/REL1_18/phase3/resources/jquery/jquery.tablesorter.js (modified) (history) /branches/REL1_18/phase3/tests/phpunit/includes/GlobalFunctions/GlobalTest.php (modified) (history) /branches/REL1_18/phase3/tests/phpunit/includes/media/ExifRotationTest.php (modified) (history) /branches/REL1_18/phase3/tests/qunit/suites/resources/jquery/jquery.tablesorter.test.js (modified) (history)

Diff [purge]

Index: branches/REL1_18/phase3/maintenance/fixExtLinksProtocolRelative.php
—	—	@@ -0,0 +1,81 @@
	2	+<?php
	3	+/**
	4	+ * Fixes any entries for protocol-relative URLs in the externallinks table,
	5	+ * replacing each protocol-relative entry with two entries, one for http
	6	+ * and one for https.
	7	+ *
	8	+ * This program is free software; you can redistribute it and/or modify
	9	+ * it under the terms of the GNU General Public License as published by
	10	+ * the Free Software Foundation; either version 2 of the License, or
	11	+ * (at your option) any later version.
	12	+ *
	13	+ * This program is distributed in the hope that it will be useful,
	14	+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
	15	+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
	16	+ * GNU General Public License for more details.
	17	+ *
	18	+ * You should have received a copy of the GNU General Public License along
	19	+ * with this program; if not, write to the Free Software Foundation, Inc.,
	20	+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
	21	+ * http://www.gnu.org/copyleft/gpl.html
	22	+ *
	23	+ * @ingroup Maintenance
	24	+ */
	25	+
	26	+require_once( dirname( __FILE__ ) . '/Maintenance.php' );
	27	+
	28	+class FixExtLinksProtocolRelative extends LoggedUpdateMaintenance {
	29	+ public function __construct() {
	30	+ parent::__construct();
	31	+ $this->mDescription = "Fixes any entries in the externallinks table containing protocol-relative URLs";
	32	+ }
	33	+
	34	+ protected function getUpdateKey() {
	35	+ return 'fix protocol-relative URLs in externallinks';
	36	+ }
	37	+
	38	+ protected function updateSkippedMessage() {
	39	+ return 'protocol-relative URLs in externallinks table already fixed.';
	40	+ }
	41	+
	42	+ protected function doDBUpdates() {
	43	+ $db = wfGetDB( DB_MASTER );
	44	+ if ( !$db->tableExists( 'externallinks' ) ) {
	45	+ $this->error( "externallinks table does not exist" );
	46	+ return false;
	47	+ }
	48	+ $this->output( "Fixing protocol-relative entries in the externallinks table...\n" );
	49	+ $res = $db->select( 'externallinks', array( 'el_from', 'el_to', 'el_index' ),
	50	+ array( 'el_index' . $db->buildLike( '//', $db->anyString() ) ),
	51	+ __METHOD__
	52	+ );
	53	+ $count = 0;
	54	+ foreach ( $res as $row ) {
	55	+ $count++;
	56	+ if ( $count % 100 == 0 ) {
	57	+ $this->output( $count );
	58	+ wfWaitForSlaves();
	59	+ }
	60	+ $db->insert( 'externallinks',
	61	+ array(
	62	+ array(
	63	+ 'el_from' => $row->el_from,
	64	+ 'el_to' => $row->el_to,
	65	+ 'el_index' => "http:{$row->el_index}",
	66	+ ),
	67	+ array(
	68	+ 'el_from' => $row->el_from,
	69	+ 'el_to' => $row->el_to,
	70	+ 'el_index' => "https:{$row->el_index}",
	71	+ )
	72	+ ), __METHOD__, array( 'IGNORE' )
	73	+ );
	74	+ $db->delete( 'externallinks', array( 'el_index' => $row->el_index, 'el_from' => $row->el_from, 'el_to' => $row->el_to ), __METHOD__ );
	75	+ }
	76	+ $this->output( "Done, $count rows updated.\n" );
	77	+ return true;
	78	+ }
	79	+}
	80	+
	81	+$maintClass = "FixExtLinksProtocolRelative";
	82	+require_once( RUN_MAINTENANCE_IF_MAIN );
Property changes on: branches/REL1_18/phase3/maintenance/fixExtLinksProtocolRelative.php
___________________________________________________________________
Added: svn:eol-style
1	83	+ native
Index: branches/REL1_18/phase3/CREDITS
—	—	@@ -96,6 +96,7 @@
97	97	* Denny Vrandecic
98	98	* Erwin Dokter
99	99	* FunPika
	100	+* fomafix
100	101	* Grunny
101	102	* Harry Burt
102	103	* Ireas
Index: branches/REL1_18/phase3/tests/qunit/suites/resources/jquery/jquery.tablesorter.test.js
—	—	@@ -368,6 +368,27 @@
369	369	}
370	370	);
371	371
	372	+/** FIXME: the diff output is not very readeable. */
	373	+test( 'bug 32047 - caption must be before thead', function() {
	374	+ var $table;
	375	+ $table = $(
	376	+ '<table class="sortable">' +
	377	+ '<caption>CAPTION</caption>' +
	378	+ '<tr><th>THEAD</th></tr>' +
	379	+ '<tr><td>A</td></tr>' +
	380	+ '<tr><td>B</td></tr>' +
	381	+ '<tr class="sortbottom"><td>TFOOT</td></tr>' +
	382	+ '</table>'
	383	+ );
	384	+ $table.tablesorter();
	385	+
	386	+ equals(
	387	+ $table.children( ).get( 0 ).nodeName,
	388	+ 'CAPTION',
	389	+ 'First element after <thead> must be <caption> (bug 32047)'
	390	+ );
	391	+});
	392	+
372	393	test( 'data-sort-value attribute, when available, should override sorting position', function() {
373	394	var $table, data;
374	395
Index: branches/REL1_18/phase3/tests/phpunit/includes/media/ExifRotationTest.php
—	—	@@ -20,10 +20,15 @@
21	21	global $wgShowEXIF;
22	22	$this->show = $wgShowEXIF;
23	23	$wgShowEXIF = true;
	24	+
	25	+ global $wgEnableAutoRotation;
	26	+ $this->oldAuto = $wgEnableAutoRotation;
	27	+ $wgEnableAutoRotation = true;
24	28	}
25	29	public function tearDown() {
26		~~- global $wgShowEXIF;~~
	30	+ global $wgShowEXIF, $wgEnableAutoRotation;
27	31	$wgShowEXIF = $this->show;
	32	+ $wgEnableAutoRotation = $this->oldAuto;
28	33	}
29	34
30	35	/**
—	—	@@ -31,6 +36,9 @@
32	37	* @dataProvider providerFiles
33	38	*/
34	39	function testMetadata( $name, $type, $info ) {
	40	+ if ( !BitmapHandler::canRotate() ) {
	41	+ $this->markTestSkipped( "This test needs a rasterizer that can auto-rotate." );
	42	+ }
35	43	$file = UnregisteredLocalFile::newFromPath( $this->filePath . $name, $type );
36	44	$this->assertEquals( $info['width'], $file->getWidth(), "$name: width check" );
37	45	$this->assertEquals( $info['height'], $file->getHeight(), "$name: height check" );
—	—	@@ -41,6 +49,9 @@
42	50	* @dataProvider providerFiles
43	51	*/
44	52	function testRotationRendering( $name, $type, $info, $thumbs ) {
	53	+ if ( !BitmapHandler::canRotate() ) {
	54	+ $this->markTestSkipped( "This test needs a rasterizer that can auto-rotate." );
	55	+ }
45	56	foreach( $thumbs as $size => $out ) {
46	57	if( preg_match('/^(\d+)px$/', $size, $matches ) ) {
47	58	$params = array(
—	—	@@ -109,6 +120,95 @@
110	121	)
111	122	);
112	123	}
	124	+
	125	+ /**
	126	+ * Same as before, but with auto-rotation disabled.
	127	+ * @dataProvider providerFilesNoAutoRotate
	128	+ */
	129	+ function testMetadataNoAutoRotate( $name, $type, $info ) {
	130	+ global $wgEnableAutoRotation;
	131	+ $wgEnableAutoRotation = false;
	132	+
	133	+ $file = UnregisteredLocalFile::newFromPath( $this->filePath . $name, $type );
	134	+ $this->assertEquals( $info['width'], $file->getWidth(), "$name: width check" );
	135	+ $this->assertEquals( $info['height'], $file->getHeight(), "$name: height check" );
	136	+
	137	+ $wgEnableAutoRotation = true;
	138	+ }
	139	+
	140	+ /**
	141	+ *
	142	+ * @dataProvider providerFilesNoAutoRotate
	143	+ */
	144	+ function testRotationRenderingNoAutoRotate( $name, $type, $info, $thumbs ) {
	145	+ global $wgEnableAutoRotation;
	146	+ $wgEnableAutoRotation = false;
	147	+
	148	+ foreach( $thumbs as $size => $out ) {
	149	+ if( preg_match('/^(\d+)px$/', $size, $matches ) ) {
	150	+ $params = array(
	151	+ 'width' => $matches[1],
	152	+ );
	153	+ } elseif ( preg_match( '/^(\d+)x(\d+)px$/', $size, $matches ) ) {
	154	+ $params = array(
	155	+ 'width' => $matches[1],
	156	+ 'height' => $matches[2]
	157	+ );
	158	+ } else {
	159	+ throw new MWException('bogus test data format ' . $size);
	160	+ }
	161	+
	162	+ $file = $this->localFile( $name, $type );
	163	+ $thumb = $file->transform( $params, File::RENDER_NOW );
	164	+
	165	+ $this->assertEquals( $out[0], $thumb->getWidth(), "$name: thumb reported width check for $size" );
	166	+ $this->assertEquals( $out[1], $thumb->getHeight(), "$name: thumb reported height check for $size" );
	167	+
	168	+ $gis = getimagesize( $thumb->getPath() );
	169	+ if ($out[0] > $info['width']) {
	170	+ // Physical image won't be scaled bigger than the original.
	171	+ $this->assertEquals( $info['width'], $gis[0], "$name: thumb actual width check for $size");
	172	+ $this->assertEquals( $info['height'], $gis[1], "$name: thumb actual height check for $size");
	173	+ } else {
	174	+ $this->assertEquals( $out[0], $gis[0], "$name: thumb actual width check for $size");
	175	+ $this->assertEquals( $out[1], $gis[1], "$name: thumb actual height check for $size");
	176	+ }
	177	+ }
	178	+ $wgEnableAutoRotation = true;
	179	+ }
	180	+
	181	+ function providerFilesNoAutoRotate() {
	182	+ return array(
	183	+ array(
	184	+ 'landscape-plain.jpg',
	185	+ 'image/jpeg',
	186	+ array(
	187	+ 'width' => 1024,
	188	+ 'height' => 768,
	189	+ ),
	190	+ array(
	191	+ '800x600px' => array( 800, 600 ),
	192	+ '9999x800px' => array( 1067, 800 ),
	193	+ '800px' => array( 800, 600 ),
	194	+ '600px' => array( 600, 450 ),
	195	+ )
	196	+ ),
	197	+ array(
	198	+ 'portrait-rotated.jpg',
	199	+ 'image/jpeg',
	200	+ array(
	201	+ 'width' => 1024, // since not rotated
	202	+ 'height' => 768, // since not rotated
	203	+ ),
	204	+ array(
	205	+ '800x600px' => array( 800, 600 ),
	206	+ '9999x800px' => array( 1067, 800 ),
	207	+ '800px' => array( 800, 600 ),
	208	+ '600px' => array( 600, 450 ),
	209	+ )
	210	+ )
	211	+ );
	212	+ }
113	213
114	214
115	215	const TEST_WIDTH = 100;
Index: branches/REL1_18/phase3/tests/phpunit/includes/GlobalFunctions/GlobalTest.php
—	—	@@ -831,42 +831,42 @@
832	832	}
833	833
834	834	/**
835		~~- * @dataProvider provideMakeUrlIndex()~~
	835	+ * @dataProvider provideMakeUrlIndexes()
836	836	*/
837		~~- function testMakeUrlIndex( $url, $expected ) {~~
838		~~- $index = wfMakeUrlIndex( $url );~~
839		~~- $this->assertEquals( $expected, $index, "wfMakeUrlIndex(\"$url\")" );~~
	837	+ function testMakeUrlIndexes( $url, $expected ) {
	838	+ $index = wfMakeUrlIndexes( $url );
	839	+ $this->assertEquals( $expected, $index, "wfMakeUrlIndexes(\"$url\")" );
840	840	}
841	841
842		~~- function provideMakeUrlIndex() {~~
	842	+ function provideMakeUrlIndexes() {
843	843	return array(
844	844	array(
845	845	// just a regular :)
846	846	'https://bugzilla.wikimedia.org/show_bug.cgi?id=28627',
847		~~- 'https://org.wikimedia.bugzilla./show_bug.cgi?id=28627'~~
	847	+ array( 'https://org.wikimedia.bugzilla./show_bug.cgi?id=28627' )
848	848	),
849	849	array(
850	850	// mailtos are handled special
851	851	// is this really right though? that final . probably belongs earlier?
852	852	'mailto:wiki@wikimedia.org',
853		~~- 'mailto:org.wikimedia@wiki.',~~
	853	+ array( 'mailto:org.wikimedia@wiki.' )
854	854	),
855	855
856	856	// file URL cases per bug 28627...
857	857	array(
858	858	// three slashes: local filesystem path Unix-style
859	859	'file:///whatever/you/like.txt',
860		~~- 'file://./whatever/you/like.txt'~~
	860	+ array( 'file://./whatever/you/like.txt' )
861	861	),
862	862	array(
863	863	// three slashes: local filesystem path Windows-style
864	864	'file:///c:/whatever/you/like.txt',
865		~~- 'file://./c:/whatever/you/like.txt'~~
	865	+ array( 'file://./c:/whatever/you/like.txt' )
866	866	),
867	867	array(
868	868	// two slashes: UNC filesystem path Windows-style
869	869	'file://intranet/whatever/you/like.txt',
870		~~- 'file://intranet./whatever/you/like.txt'~~
	870	+ array( 'file://intranet./whatever/you/like.txt' )
871	871	),
872	872	// Multiple-slash cases that can sorta work on Mozilla
873	873	// if you hack it just right are kinda pathological,
—	—	@@ -875,6 +875,15 @@
876	876	//
877	877	// Those will survive the algorithm but with results that
878	878	// are less consistent.
	879	+
	880	+ // protocol-relative URL cases per bug 29854...
	881	+ array(
	882	+ '//bugzilla.wikimedia.org/show_bug.cgi?id=28627',
	883	+ array(
	884	+ 'http://org.wikimedia.bugzilla./show_bug.cgi?id=28627',
	885	+ 'https://org.wikimedia.bugzilla./show_bug.cgi?id=28627'
	886	+ )
	887	+ ),
879	888	);
880	889	}
881	890
Index: branches/REL1_18/phase3/includes/GlobalFunctions.php
—	—	@@ -602,12 +602,12 @@
603	603	}
604	604
605	605	/**
606		~~- * Make a URL index, appropriate for the el_index field of externallinks.~~
	606	+ * Make URL indexes, appropriate for the el_index field of externallinks.
607	607	*
608	608	* @param $url String
609		~~- * @return String~~
	609	+ * @return array
610	610	*/
611		~~-function wfMakeUrlIndex( $url ) {~~
	611	+function wfMakeUrlIndexes( $url ) {
612	612	$bits = wfParseUrl( $url );
613	613
614	614	// Reverse the labels in the hostname, convert to lower case
—	—	@@ -647,7 +647,12 @@
648	648	if ( isset( $bits['fragment'] ) ) {
649	649	$index .= '#' . $bits['fragment'];
650	650	}
651		~~- return $index;~~
	651	+
	652	+ if ( $prot == '' ) {
	653	+ return array( "http:$index", "https:$index" );
	654	+ } else {
	655	+ return array( $index );
	656	+ }
652	657	}
653	658
654	659	/**
Property changes on: branches/REL1_18/phase3/includes/GlobalFunctions.php
___________________________________________________________________
Modified: svn:mergeinfo
655	660	Merged /trunk/phase3/includes/GlobalFunctions.php:r102951,102954
Index: branches/REL1_18/phase3/includes/installer/DatabaseUpdater.php
—	—	@@ -40,7 +40,8 @@
41	41	protected $shared = false;
42	42
43	43	protected $postDatabaseUpdateMaintenance = array(
44		~~- 'DeleteDefaultMessages'~~
	44	+ 'DeleteDefaultMessages',
	45	+ 'FixExtLinksProtocolRelative',
45	46	);
46	47
47	48	/**
Index: branches/REL1_18/phase3/includes/LinksUpdate.php
—	—	@@ -438,11 +438,13 @@
439	439	$arr = array();
440	440	$diffs = array_diff_key( $this->mExternals, $existing );
441	441	foreach( $diffs as $url => $dummy ) {
442		~~- $arr[] = array(~~
443		~~- 'el_from' => $this->mId,~~
444		~~- 'el_to' => $url,~~
445		~~- 'el_index' => wfMakeUrlIndex( $url ),~~
446		~~- );~~
	442	+ foreach( wfMakeUrlIndexes( $url ) as $index ) {
	443	+ $arr[] = array(
	444	+ 'el_from' => $this->mId,
	445	+ 'el_to' => $url,
	446	+ 'el_index' => $index,
	447	+ );
	448	+ }
447	449	}
448	450	return $arr;
449	451	}
Index: branches/REL1_18/phase3/includes/api/ApiQueryBase.php
—	—	@@ -228,7 +228,7 @@
229	229	protected function addTimestampWhereRange( $field, $dir, $start, $end, $sort = true ) {
230	230	$db = $this->getDb();
231	231	return $this->addWhereRange( $field, $dir,
232		~~- $db->timestamp( $start ), $db->timestamp( $end ), $sort );~~
	232	+ $db->timestampOrNull( $start ), $db->timestampOrNull( $end ), $sort );
233	233	}
234	234
235	235	/**
Index: branches/REL1_18/phase3/includes/api/ApiQueryExternalLinks.php
—	—	@@ -69,6 +69,11 @@
70	70	$this->addOption( 'ORDER BY', 'el_from' );
71	71	}
72	72
	73	+ // If we're querying all protocols, use DISTINCT to avoid repeating protocol-relative links twice
	74	+ if ( $protocol === null ) {
	75	+ $this->addOption( 'DISTINCT' );
	76	+ }
	77	+
73	78	$this->addOption( 'LIMIT', $params['limit'] + 1 );
74	79	$offset = isset( $params['offset'] ) ? $params['offset'] : 0;
75	80	if ( $offset ) {
Property changes on: branches/REL1_18/phase3/includes/api
___________________________________________________________________
Modified: svn:mergeinfo
76	81	Merged /trunk/phase3/includes/api:r99236,102301,102303,102498,102710,102751,102951,102954
Index: branches/REL1_18/phase3/includes/media/ExifBitmap.php
—	—	@@ -134,12 +134,17 @@
135	135	* @return array
136	136	*/
137	137	function getImageSize( $image, $path ) {
	138	+ global $wgEnableAutoRotation;
138	139	$gis = parent::getImageSize( $image, $path );
139	140
140	141	// Don't just call $image->getMetadata(); File::getPropsFromPath() calls us with a bogus object.
141	142	// This may mean we read EXIF data twice on initial upload.
142		~~- $meta = $this->getMetadata( $image, $path );~~
143		~~- $rotation = $this->getRotationForExif( $meta );~~
	143	+ if ( $wgEnableAutoRotation ) {
	144	+ $meta = $this->getMetadata( $image, $path );
	145	+ $rotation = $this->getRotationForExif( $meta );
	146	+ } else {
	147	+ $rotation = 0;
	148	+ }
144	149
145	150	if ($rotation == 90 \|\| $rotation == 270) {
146	151	$width = $gis[0];
Property changes on: branches/REL1_18/phase3/includes/media/ExifBitmap.php
___________________________________________________________________
Modified: svn:mergeinfo
147	152	Merged /trunk/phase3/includes/media/ExifBitmap.php:r102751,102951,102954
Index: branches/REL1_18/phase3/includes/AutoLoader.php
—	—	@@ -827,6 +827,7 @@
828	828	'DeleteDefaultMessages' => 'maintenance/deleteDefaultMessages.php',
829	829	'FakeMaintenance' => 'maintenance/Maintenance.php',
830	830	'Maintenance' => 'maintenance/Maintenance.php',
	831	+ 'FixExtLinksProtocolRelative' => 'maintenance/fixExtLinksProtocolRelative.php',
831	832	'PopulateCategory' => 'maintenance/populateCategory.php',
832	833	'PopulateLogSearch' => 'maintenance/populateLogSearch.php',
833	834	'PopulateLogUsertext' => 'maintenance/populateLogUsertext.php',
Property changes on: branches/REL1_18/phase3/includes/AutoLoader.php
___________________________________________________________________
Modified: svn:mergeinfo
834	835	Merged /trunk/phase3/includes/AutoLoader.php:r102951,102954
Property changes on: branches/REL1_18/phase3/includes
___________________________________________________________________
Modified: svn:mergeinfo
835	836	Merged /trunk/phase3/includes:r99236,102301,102303,102498,102710,102751,102951,102954
Index: branches/REL1_18/phase3/resources/jquery/jquery.tablesorter.js
—	—	@@ -240,7 +240,7 @@
241	241	}
242	242	$thead.append( this );
243	243	} );
244		~~- $table.prepend( $thead );~~
	244	+ $table.children('tbody').before( $thead );
245	245	}
246	246	if( !$table.get(0).tFoot ) {
247	247	var $tfoot = $( '<tfoot>' );
Property changes on: branches/REL1_18/phase3
___________________________________________________________________
Modified: svn:mergeinfo
248	248	Merged /trunk/phase3:r99236,102301,102303,102498,102710,102751,102951,102954

Past revisions this follows-up on

Revision	Commit summary	Author	Date
r99236	Follow-up r84765, use timestampOrNull	btongminh	19:11, 7 October 2011
r102301	Fix Bug #32047: in table with class="sortable", thead is before...	mah	17:33, 7 November 2011
r102303	tests for bug 32047 / r102301...	hashar	17:48, 7 November 2011
r102498	Comments out console.log calls...	hashar	09:01, 9 November 2011
r102710	on tablesort, verify <caption> is the first child in the table...	hashar	22:54, 10 November 2011
r102751	(follow-up r99910) Make $wgEnableAutoRotation work...	bawolff	04:09, 11 November 2011
r102951	(bug 29854) Store protocol-relative links twice in the externallinks table, o...	catrope	09:13, 14 November 2011
r102954	Fix for r102949: don't delete too much	catrope	09:16, 14 November 2011

Status & tagging log

22:10, 20 November 2011 😂 (talk | contribs) changed the status of r103029 [removed: new added: ok]