--- Comment #12 from Bryan Tong Minh <> 2010-11-26 
20:55:42 UTC ---
Comments only on the filerepo part:

+        global $wgUser;
File repo should never ever ever ever use globals like $wgUser. Pass the user
object as a function argument, similar to LocalFile::upload

+        $dstPath = $this->repo->getZonePath('public') . '/archive/' .
$this->getHashPath() . $dstName;
+        $dstName = gmdate( 'YmdHis', wfTimestamp( TS_UNIX, $timestamp ) ) .
'!' . $this->getName();

You are essentially duplicating this from LocalFile::publish. The archive name
generation should be moved to a separate function, which is called by both
publish() and recordOldUpload().
E.g. $dstName = $this->generateArchiveName( $timestamp );

I find it strange that you are doing path generation in recordOldUpload(). Is
there a specific reason you are not doing that in the caller and pass the
entire archive name? That would seem a bit more logical to me.

+        /* Original gmdate( 'YmdHis' ) is not corrent AT ALL! */
+        /* It gives an inconsistency: file name has one timestamp and database
row has another. */
Correct, we should fix that anyway.

+        $props['timestamp'] = wfTimestamp( TS_MW, $timestamp );
You should use $dbw->timestamp( $timestamp )

I don't know the import/export code at all, so I won't comment on that.

In general good work. You are on a good way to finally get upload
importing/exporting available.

Configure bugmail:
------- You are receiving this mail because: -------
You are the assignee for the bug.
You are on the CC list for the bug.

Wikibugs-l mailing list

Reply via email to