r32817 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r32816‎ | r32817 | r32818 >
Date:16:46, 5 April 2008
Author:tstarling
Status:old
Tags:
Comment:
* Fixed bug 13532, incorrect use of wfFindFile() causing total breakage of file revert
* Use protected not private
* Remove aggressive history-oriented caching from OldLocalFile
* Keep a process cache of the OldLocalFile object to avoid duplicate queries
Modified paths:
  • /trunk/phase3/includes/FileRevertForm.php (modified) (history)
  • /trunk/phase3/includes/filerepo/OldLocalFile.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/filerepo/OldLocalFile.php
@@ -45,8 +45,7 @@
4646 }
4747
4848 function getCacheKey() {
49 - $hashedName = md5($this->getName());
50 - return wfMemcKey( 'oldfile', $hashedName );
 49+ return false;
5150 }
5251
5352 function getArchiveName() {
@@ -64,105 +63,6 @@
6564 return $this->exists() && !$this->isDeleted(File::DELETED_FILE);
6665 }
6766
68 - /**
69 - * Try to load file metadata from memcached. Returns true on success.
70 - */
71 - function loadFromCache() {
72 - global $wgMemc;
73 - wfProfileIn( __METHOD__ );
74 - $this->dataLoaded = false;
75 - $key = $this->getCacheKey();
76 - if ( !$key ) {
77 - return false;
78 - }
79 - $oldImages = $wgMemc->get( $key );
80 -
81 - if ( isset( $oldImages['version'] ) && $oldImages['version'] == self::CACHE_VERSION ) {
82 - unset( $oldImages['version'] );
83 - $more = isset( $oldImages['more'] );
84 - unset( $oldImages['more'] );
85 - $found = false;
86 - if ( is_null( $this->requestedTime ) ) {
87 - foreach ( $oldImages as $timestamp => $info ) {
88 - if ( $info['archive_name'] == $this->archive_name ) {
89 - $found = true;
90 - break;
91 - }
92 - }
93 - } else {
94 - krsort( $oldImages );
95 - foreach ( $oldImages as $timestamp => $info ) {
96 - if ( $timestamp == $this->requestedTime ) {
97 - $found = true;
98 - break;
99 - }
100 - }
101 - }
102 - if ( $found ) {
103 - wfDebug( "Pulling file metadata from cache key {$key}[{$timestamp}]\n" );
104 - $this->dataLoaded = true;
105 - $this->fileExists = true;
106 - foreach ( $info as $name => $value ) {
107 - $this->$name = $value;
108 - }
109 - } elseif ( $more ) {
110 - wfDebug( "Cache key was truncated, oldimage row might be found in the database\n" );
111 - } else {
112 - wfDebug( "Image did not exist at the specified time.\n" );
113 - $this->fileExists = false;
114 - $this->dataLoaded = true;
115 - }
116 - }
117 -
118 - if ( $this->dataLoaded ) {
119 - wfIncrStats( 'image_cache_hit' );
120 - } else {
121 - wfIncrStats( 'image_cache_miss' );
122 - }
123 -
124 - wfProfileOut( __METHOD__ );
125 - return $this->dataLoaded;
126 - }
127 -
128 - function saveToCache() {
129 - // If a timestamp was specified, cache the entire history of the image (up to MAX_CACHE_ROWS).
130 - if ( is_null( $this->requestedTime ) ) {
131 - return;
132 - }
133 - // This is expensive, so we only do it if $wgMemc is real
134 - global $wgMemc;
135 - if ( $wgMemc instanceof FakeMemcachedClient ) {
136 - return;
137 - }
138 - $key = $this->getCacheKey();
139 - if ( !$key ) {
140 - return;
141 - }
142 - wfProfileIn( __METHOD__ );
143 -
144 - $dbr = $this->repo->getSlaveDB();
145 - $res = $dbr->select( 'oldimage', $this->getCacheFields( 'oi_' ),
146 - array( 'oi_name' => $this->getName() ), __METHOD__,
147 - array(
148 - 'LIMIT' => self::MAX_CACHE_ROWS + 1,
149 - 'ORDER BY' => 'oi_timestamp DESC',
150 - ));
151 - $cache = array( 'version' => self::CACHE_VERSION );
152 - $numRows = $dbr->numRows( $res );
153 - if ( $numRows > self::MAX_CACHE_ROWS ) {
154 - $cache['more'] = true;
155 - $numRows--;
156 - }
157 - for ( $i = 0; $i < $numRows; $i++ ) {
158 - $row = $dbr->fetchObject( $res );
159 - $decoded = $this->decodeRow( $row, 'oi_' );
160 - $cache[$row->oi_timestamp] = $decoded;
161 - }
162 - $dbr->freeResult( $res );
163 - $wgMemc->set( $key, $cache, 7*86400 /* 1 week */ );
164 - wfProfileOut( __METHOD__ );
165 - }
166 -
16767 function loadFromDB() {
16868 wfProfileIn( __METHOD__ );
16969 $this->dataLoaded = true;
Index: trunk/phase3/includes/FileRevertForm.php
@@ -8,10 +8,11 @@
99 */
1010 class FileRevertForm {
1111
12 - private $title = null;
13 - private $file = null;
14 - private $oldimage = '';
15 - private $timestamp = false;
 12+ protected $title = null;
 13+ protected $file = null;
 14+ protected $archiveName = '';
 15+ protected $timestamp = false;
 16+ protected $oldFile;
1617
1718 /**
1819 * Constructor
@@ -48,10 +49,10 @@
4950 return;
5051 }
5152
52 - $this->oldimage = $wgRequest->getText( 'oldimage' );
 53+ $this->archiveName = $wgRequest->getText( 'oldimage' );
5354 $token = $wgRequest->getText( 'wpEditToken' );
5455 if( !$this->isValidOldSpec() ) {
55 - $wgOut->showUnexpectedValueError( 'oldimage', htmlspecialchars( $this->oldimage ) );
 56+ $wgOut->showUnexpectedValueError( 'oldimage', htmlspecialchars( $this->archiveName ) );
5657 return;
5758 }
5859
@@ -62,8 +63,8 @@
6364 }
6465
6566 // Perform the reversion if appropriate
66 - if( $wgRequest->wasPosted() && $wgUser->matchEditToken( $token, $this->oldimage ) ) {
67 - $source = $this->file->getArchiveVirtualUrl( $this->oldimage );
 67+ if( $wgRequest->wasPosted() && $wgUser->matchEditToken( $token, $this->archiveName ) ) {
 68+ $source = $this->file->getArchiveVirtualUrl( $this->archiveName );
6869 $comment = $wgRequest->getText( 'wpComment' );
6970 // TODO: Preserve file properties from database instead of reloading from file
7071 $status = $this->file->upload( $source, $comment, $comment );
@@ -71,7 +72,7 @@
7273 $wgOut->addHtml( wfMsgExt( 'filerevert-success', 'parse', $this->title->getText(),
7374 $wgLang->date( $this->getTimestamp(), true ),
7475 $wgLang->time( $this->getTimestamp(), true ),
75 - wfExpandUrl( $this->file->getArchiveUrl( $this->oldimage ) ) ) );
 76+ wfExpandUrl( $this->file->getArchiveUrl( $this->archiveName ) ) ) );
7677 $wgOut->returnToMain( false, $this->title );
7778 } else {
7879 $wgOut->addWikiText( $status->getWikiText() );
@@ -86,16 +87,16 @@
8788 /**
8889 * Show the confirmation form
8990 */
90 - private function showForm() {
 91+ protected function showForm() {
9192 global $wgOut, $wgUser, $wgRequest, $wgLang, $wgContLang;
9293 $timestamp = $this->getTimestamp();
9394
9495 $form = Xml::openElement( 'form', array( 'method' => 'post', 'action' => $this->getAction() ) );
95 - $form .= Xml::hidden( 'wpEditToken', $wgUser->editToken( $this->oldimage ) );
 96+ $form .= Xml::hidden( 'wpEditToken', $wgUser->editToken( $this->archiveName ) );
9697 $form .= '<fieldset><legend>' . wfMsgHtml( 'filerevert-legend' ) . '</legend>';
9798 $form .= wfMsgExt( 'filerevert-intro', 'parse', $this->title->getText(),
9899 $wgLang->date( $timestamp, true ), $wgLang->time( $timestamp, true ),
99 - wfExpandUrl( $this->file->getArchiveUrl( $this->oldimage ) ) );
 100+ wfExpandUrl( $this->file->getArchiveUrl( $this->archiveName ) ) );
100101 $form .= '<p>' . Xml::inputLabel( wfMsg( 'filerevert-comment' ), 'wpComment', 'wpComment',
101102 60, wfMsgForContent( 'filerevert-defaultcomment',
102103 $wgContLang->date( $timestamp, false, false ), $wgContLang->time( $timestamp, false, false ) ) ) . '</p>';
@@ -109,7 +110,7 @@
110111 /**
111112 * Set headers, titles and other bits
112113 */
113 - private function setHeaders() {
 114+ protected function setHeaders() {
114115 global $wgOut, $wgUser;
115116 $wgOut->setPageTitle( wfMsg( 'filerevert', $this->title->getText() ) );
116117 $wgOut->setRobotPolicy( 'noindex,nofollow' );
@@ -121,10 +122,10 @@
122123 *
123124 * @return bool
124125 */
125 - private function isValidOldSpec() {
126 - return strlen( $this->oldimage ) >= 16
127 - && strpos( $this->oldimage, '/' ) === false
128 - && strpos( $this->oldimage, '\\' ) === false;
 126+ protected function isValidOldSpec() {
 127+ return strlen( $this->archiveName ) >= 16
 128+ && strpos( $this->archiveName, '/' ) === false
 129+ && strpos( $this->archiveName, '\\' ) === false;
129130 }
130131
131132 /**
@@ -133,9 +134,8 @@
134135 *
135136 * @return bool
136137 */
137 - private function haveOldVersion() {
138 - $file = wfFindFile( $this->title, $this->oldimage );
139 - return $file && $file->exists() && $file->isLocal();
 138+ protected function haveOldVersion() {
 139+ return $this->getOldFile()->exists();
140140 }
141141
142142 /**
@@ -143,10 +143,10 @@
144144 *
145145 * @return string
146146 */
147 - private function getAction() {
 147+ protected function getAction() {
148148 $q = array();
149149 $q[] = 'action=revert';
150 - $q[] = 'oldimage=' . urlencode( $this->oldimage );
 150+ $q[] = 'oldimage=' . urlencode( $this->archiveName );
151151 return $this->title->getLocalUrl( implode( '&', $q ) );
152152 }
153153
@@ -155,12 +155,17 @@
156156 *
157157 * @return string
158158 */
159 - private function getTimestamp() {
 159+ protected function getTimestamp() {
160160 if( $this->timestamp === false ) {
161 - $file = RepoGroup::singleton()->getLocalRepo()->newFromArchiveName( $this->title, $this->oldimage );
162 - $this->timestamp = $file->getTimestamp();
 161+ $this->timestamp = $this->getOldFile()->getTimestamp();
163162 }
164163 return $this->timestamp;
165164 }
166 -
167 -}
\ No newline at end of file
 165+
 166+ protected function getOldFile() {
 167+ if ( !isset( $this->oldFile ) ) {
 168+ $this->oldFile = RepoGroup::singleton()->getLocalRepo()->newFromArchiveName( $this->title, $this->archiveName );
 169+ }
 170+ return $this->oldFile;
 171+ }
 172+}

Follow-up revisions

RevisionCommit summaryAuthorDate
r33001* (bug 13532) Use proper timestamp call when reverting images...brion00:26, 9 April 2008

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r32526Wrap timestamp for DB query in $dbr->timestamp() call. Bug 13532.greg21:55, 27 March 2008
r32527Note bug 13532greg21:57, 27 March 2008

Status & tagging log