r83461 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r83460‎ | r83461 | r83462 >
Date:17:07, 7 March 2011
Author:btongminh
Status:ok (Comments)
Tags:
Comment:
(bug 27722) list=filearchive now supports revdel
* Adds a condition fa_deleted=0 if the user does not have the suppressrevision rights. This field is unindexed. This should however not be a big problem as files with fa_deleted are rare. Unfortunately this hides files that do not have DELETED_RESTRICTED, but I don't know how bad fa_deleted & DELETED_RESTRICTED = 0 is for performance
* Added deletedfile, deletedcomment, deleteduser and deletedrestricted to the output for what I think are appropriate fa_deleted fields, but it's hard to tell what's corrent without a single line of documentation or even comment in the code. Perhaps somebody can dig up a commit message where the purpose of the constants is explained?
Modified paths:
  • /trunk/phase3/includes/api/ApiQueryFilearchive.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/api/ApiQueryFilearchive.php
@@ -66,7 +66,7 @@
6767
6868 $this->addTables( 'filearchive' );
6969
70 - $this->addFields( 'fa_name' );
 70+ $this->addFields( array( 'fa_name', 'fa_deleted' ) );
7171 $this->addFieldsIf( 'fa_storage_key', $fld_sha1 );
7272 $this->addFieldsIf( 'fa_timestamp', $fld_timestamp );
7373
@@ -92,9 +92,22 @@
9393 $dir = ( $params['dir'] == 'descending' ? 'older' : 'newer' );
9494 $from = ( is_null( $params['from'] ) ? null : $this->titlePartToKey( $params['from'] ) );
9595 $this->addWhereRange( 'fa_name', $dir, $from, null );
96 - if ( isset( $params['prefix'] ) )
 96+ if ( isset( $params['prefix'] ) ) {
9797 $this->addWhere( 'fa_name' . $db->buildLike( $this->titlePartToKey( $params['prefix'] ), $db->anyString() ) );
 98+ }
 99+
 100+ if ( !$wgUser->isAllowed( 'suppressrevision' ) ) {
 101+ // Filter out revisions that the user is not allowed to see. There
 102+ // is no way to indicate that we have skipped stuff because the
 103+ // continuation parameter is fa_name
 104+
 105+ // Note that this field is unindexed. This should however not be
 106+ // a big problem as files with fa_deleted are rare
 107+ $this->addWhereFld( 'fa_deleted', 0 );
 108+ }
98109
 110+
 111+
99112 $limit = $params['limit'];
100113 $this->addOption( 'LIMIT', $limit + 1 );
101114 $this->addOption( 'ORDER BY', 'fa_name' .
@@ -147,7 +160,22 @@
148161 if ( $fld_mime ) {
149162 $file['mime'] = "$row->fa_major_mime/$row->fa_minor_mime";
150163 }
 164+
 165+ if ( $row->fa_deleted & File::DELETED_FILE ) {
 166+ $file['deletedfile'] = '';
 167+ }
 168+ if ( $row->fa_deleted & File::DELETED_COMMENT ) {
 169+ $file['deletedcomment'] = '';
 170+ }
 171+ if ( $row->fa_deleted & File::DELETED_USER ) {
 172+ $file['deleteduser'] = '';
 173+ }
 174+ if ( $row->fa_deleted & File::DELETED_RESTRICTED ) {
 175+ // This file is deleted for normal admins
 176+ $file['deletedrestricted'] = '';
 177+ }
151178
 179+
152180 $fit = $result->addValue( array( 'query', $this->getModuleName() ), null, $file );
153181 if ( !$fit ) {
154182 $this->setContinueEnumParameter( 'from', $this->keyToTitle( $row->fa_name ) );

Sign-offs

UserFlagDate
Jarry1250tested17:59, 7 March 2011

Follow-up revisions

RevisionCommit summaryAuthorDate
r83463Follow-up r83461, replace deleted with hidden and reverse the word, e.g. dele...btongminh17:17, 7 March 2011
r834871.17wmf1: MFT r82696, r83270, r83284, r83374, r83390, r83392, r83402, r83403,...catrope21:44, 7 March 2011
r85354MFT r82518, r82530, r82538, r82547, r82550, r82565, r82572, r82608, r82696, r...demon18:25, 4 April 2011

Comments

#Comment by Jarry1250 (talk | contribs)   18:04, 7 March 2011

(Tested successfully against a basic "total" revision deletion of an entry which now no longer shows up in the file archive, even logged in as the same user who revdel'ed the entry. I assume that was the intended behaviour.)

#Comment by Bryan (talk | contribs)   18:12, 7 March 2011

Jep, it only shows up if the user has the suppressrevision right.

#Comment by Duplicatebug (talk | contribs)   19:37, 7 March 2011

But that differ from other api modules. Other modules always select the row and replace the content of the field with *hidden="", if it is revdel. If the user can see the content of that field, than it is not added in all modules to the output.

DELETED_RESTRICTED is never checked.

#Comment by Bawolff (talk | contribs)   02:12, 8 March 2011

Well the deletedrevs module (which this one is most close to) doesn't show deleted rev things under any circumstances.


I've always thought that revdel only applied to the content of the file (or edit summary or user), and that only normal view deleted stuff rights was needed to view the title of the image (in which case we'd be able to show the image titles), although I guess having content and title go hand in hand does make sense from an oversighting point of view.

#Comment by Bryan (talk | contribs)   09:51, 8 March 2011

Good point. I might look into making it more functional later, but for now I'd err on the side of caution.

Status & tagging log