r85635 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r85634‎ | r85635 | r85636 >
Date:20:19, 7 April 2011
Author:btongminh
Status:resolved (Comments)
Tags:
Comment:
First part of bug 22881: Allow uploading directly into the archive to support importing files. Based on a patch by Vitaliy Filippov with some major rewrites by me.
* LocalFile::publish() supports an extra parameter to support publishing into the archive
* Added OldLocalFile::uploadOld(), which is the OldImage equivalent to LocalFile::upload(), but does not override it because it has an entirely different function signature.
Modified paths:
  • /trunk/phase3/includes/filerepo/LocalFile.php (modified) (history)
  • /trunk/phase3/includes/filerepo/OldLocalFile.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/filerepo/OldLocalFile.php
@@ -212,4 +212,71 @@
213213 $this->load();
214214 return Revision::userCanBitfield( $this->deleted, $field );
215215 }
 216+
 217+ /**
 218+ * Upload a file directly into archive. Generally for Special:Import.
 219+ *
 220+ * @param $srcPath string File system path of the source file
 221+ * @param $archiveName string Full archive name of the file, in the form
 222+ * $timestamp!$filename, where $filename must match $this->getName()
 223+ */
 224+ function uploadOld( $srcPath, $archiveName, $comment, $user ) {
 225+ $this->lock();
 226+ $status = $this->publish( $srcPath, $flags, $archiveName );
 227+
 228+ if ( $status->isGood() ) {
 229+ if ( !$this->recordOldUpload( $srcPath, $archiveName, $comment, $user ) ) {
 230+ $status->fatal( 'filenotfound', $srcPath );
 231+ }
 232+ }
 233+
 234+ $this->unlock();
 235+
 236+ return $status;
 237+ }
 238+
 239+ /**
 240+ * Record a file upload in the oldimage table, without adding log entries.
 241+ *
 242+ * @param $srcPath string File system path to the source file
 243+ * @param $archiveName string The archive name of the file
 244+ * @param $comment string Upload comment
 245+ * @param $user User User who did this upload
 246+ * @return bool
 247+ */
 248+ function recordOldUpload( $srcPath, $archiveName, $comment, $user ) {
 249+ $dbw = $this->repo->getMasterDB();
 250+ $dbw->begin();
 251+
 252+ $dstPath = $this->repo->getZonePath( 'public' ) . '/' . $this->getRel();
 253+ $props = self::getPropsFromPath( $dstPath );
 254+ if ( !$props['fileExists'] ) {
 255+ return false;
 256+ }
 257+
 258+ $dbw->insert( 'oldimage',
 259+ array(
 260+ 'oi_name' => $this->getName(),
 261+ 'oi_archive_name' => $archiveName,
 262+ 'oi_size' => $props['size'],
 263+ 'oi_width' => intval( $props['width'] ),
 264+ 'oi_height' => intval( $props['height'] ),
 265+ 'oi_bits' => $props['bits'],
 266+ 'oi_timestamp' => $props['timestamp'],
 267+ 'oi_description' => $comment,
 268+ 'oi_user' => $user->getId(),
 269+ 'oi_user_text' => $user->getName(),
 270+ 'oi_metadata' => $props['metadata'],
 271+ 'oi_media_type' => $props['media_type'],
 272+ 'oi_major_mime' => $props['major_mime'],
 273+ 'oi_minor_mime' => $props['minor_mime'],
 274+ 'oi_sha1' => $props['sha1'],
 275+ ), __METHOD__
 276+ );
 277+
 278+ $dbw->commit();
 279+
 280+ return true;
 281+ }
 282+
216283 }
Index: trunk/phase3/includes/filerepo/LocalFile.php
@@ -1045,16 +1045,22 @@
10461046 *
10471047 * @param $srcPath String: local filesystem path to the source image
10481048 * @param $flags Integer: a bitwise combination of:
1049 - * File::DELETE_SOURCE Delete the source file, i.e. move
1050 - * rather than copy
 1049+ * File::DELETE_SOURCE Delete the source file, i.e. move rather than copy
 1050+ * @param $dstArchiveName string File name if the file is to be published
 1051+ * into the archive
10511052 * @return FileRepoStatus object. On success, the value member contains the
10521053 * archive name, or an empty string if it was a new file.
10531054 */
1054 - function publish( $srcPath, $flags = 0 ) {
 1055+ function publish( $srcPath, $flags = 0, $dstArchiveName = null ) {
10551056 $this->lock();
10561057
1057 - $dstRel = $this->getRel();
1058 - $archiveName = gmdate( 'YmdHis' ) . '!' . $this->getName();
 1058+ if ( $dstArchiveName ) {
 1059+ $dstRel = 'archive/' . $this->getHashPath() . $dstArchiveName;
 1060+ } else {
 1061+ $dstRel = $this->getRel();
 1062+ }
 1063+
 1064+ $archiveName = wfTimestamp( TS_MW ) . '!'. $this->getName();
10591065 $archiveRel = 'archive/' . $this->getHashPath() . $archiveName;
10601066 $flags = $flags & File::DELETE_SOURCE ? LocalRepo::DELETE_SOURCE : 0;
10611067 $status = $this->repo->publish( $srcPath, $dstRel, $archiveRel, $flags );

Follow-up revisions

RevisionCommit summaryAuthorDate
r85644Second part of bug bug 22881: Allow exporting files along with the XML as mim...btongminh21:44, 7 April 2011
r85911Add support for importing/exporting files. This can be done by embedding the ...btongminh19:25, 12 April 2011
r90296Fix r85635. Move LocalFile::publish() magic to LocalFile::publishTo() which h...btongminh17:12, 17 June 2011

Comments

#Comment by Tim Starling (talk | contribs)   07:17, 2 May 2011

Causes "Strict Standards: Declaration of ForeignDBFile::publish() should be compatible with that of LocalFile::publish() in .../ForeignDBFile.php on line 57".

There's no reason this feature has to be limited to LocalFile. I'm not sure why you changed LocalFile::publish() at all, since basically the only thing that it does is calculates the archive name. Why not just use FileRepo::publish() directly? Or add a helper function to File/LocalFile with a different name.

#Comment by Bryan (talk | contribs)   10:53, 14 May 2011

I don't see how it belongs in the File base class, because File does not know about the distinction between new and old files. The hierarchy which associates old files with new files is made in LocalFile and its descendants.

I originally had the idea to put publish() directly into OldLocalFile, but that doesn't play nice with declaration compatibility either. Any ideas?

#Comment by Tim Starling (talk | contribs)   01:38, 30 May 2011

You could use a separate method name with LocalFile-specific functionality, and have LocalFile::publish() call it.

#Comment by Bryan (talk | contribs)   17:13, 17 June 2011

Fixed in r90296

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

Needs merge for 1.18 branch.

Status & tagging log