Show move log when viewing/creating a deleted page
Closed, ResolvedPublic

Description

Since some time, the deletion log is displayed if you visit a deleted page or create it. This is a very useful feature, however it does not help in two cases:
(1) The page is moved and the administrator deletes the redirect without including a link to the new page title in the deletion comment. The user sees only a "unnecessary redirect after page move" in the deletion log (which doesn't really help); only experienced users will visit Special:Log/<pagename> as there is no link to the general log of the page.
(2) The page is moved with suppressredirect enabled. Then, there isn't even a deletion log entry, thus no information provided for the user (who in most cases searches the page). Again, the only option is to guess that the page was moved and check the log.
Therefor I think that just like the deletion log, the move log should be displayed as well.


Version: unspecified
Severity: enhancement

bzimport set Reference to bz16950.
Tobias created this task.Via LegacyJan 9 2009, 7:58 PM
bzimport added a comment.Via ConduitFeb 3 2009, 5:37 AM

mike.lifeguard+bugs wrote:

*** Bug 17334 has been marked as a duplicate of this bug. ***

Tobias added a comment.Via ConduitFeb 3 2009, 9:26 AM

Changed to Priority High. suppressredirect has been enabled on all WMF wikis for sysops ([[:bugzilla:14998]]), and therefor this bug becomes more urgent. This is a really big transparency issue, as no information is shown and there is no link to the move log (see (2) in the original bug description). Only if you go manually to Special:Log/move and search for the page name, you get the log (but only few people know how to do this, I guess).

bzimport added a comment.Via ConduitMar 22 2009, 3:23 AM

W wrote:

Please see [http://en.wikipedia.org/w/index.php?title=User_talk:RHaworth&oldid=278845386#Sharai_parda this request] and [http://en.wikipedia.org/w/index.php?title=Special:Log&hide_patrol_log=0&page=sharai+parda this log record].

I think the problem arose because I did a move without redirect before the article had been marked patrolled.

bzimport added a comment.Via ConduitMar 23 2009, 9:52 AM

W wrote:

I can confirm my comment above. Look at this log report: http://en.wikipedia.org/w/index.php?title=Special:NewPages&offset=200903230423&limit=1

Try marking Tony Issac as patrolled or removing him from the new pages list since there is no longer any article.

Tobias added a comment.Via ConduitMar 23 2009, 9:59 AM

I'm not really sure how this relates to the feature request described above. Perhaps a separate bug report is adequate for that problem.

bzimport added a comment.Via ConduitMar 23 2009, 10:02 AM

W wrote:

It relates because it is another result of the new (and excellent) move-without-leaving-redirect option.

bzimport added a comment.Via ConduitMar 23 2009, 11:59 AM

blanchardb wrote:

It looks like any time a page is moved with redirect suppressed, its entry in Special:NewPages remains lingering as an unpatrolled page until it is bumped out as a month-old entry. The only way I see to fix this is to recreate the page, mark it as patrolled, then tag it for deletion as a test page.

So far, every time I've seen it happen, it was because the page has been moved to the creator's userspace. In one instance, the original location has been protected against recreation, and I had to ask an administrator to recreate the page just so that its entry in Special:NewPages could be suppressed.

I will make a separate bug report as suggested in Comment #5.

Tobias added a comment.Via ConduitMar 23 2009, 2:39 PM

replaces showDeletionLog by a more general showLogs both in EditPage.php and Article.php

attachment showmovelog.patch ignored as obsolete

Tobias added a comment.Via ConduitMar 23 2009, 2:39 PM

added keywords

bzimport added a comment.Via ConduitApr 1 2009, 8:43 PM

mike.lifeguard+bugs wrote:

*** Bug 18298 has been marked as a duplicate of this bug. ***

bzimport added a comment.Via ConduitApr 1 2009, 8:54 PM

ayg wrote:

The patch seems to go to great lengths to only show moves and deletes. Wouldn't it be simpler (and possibly more future-proof) to just show all log entries for the page? The last one is always going to be the relevant move or deletion, anyway.

If there is some way to display multiple logs, it should surely be abstracted into LogPager, not done on the caller side and duplicated in two different places. Actually, it looks like you could already do something like

		$pager = new LogPager( $loglist, array('move', 'delete'), false, $this->mTitle->getPrefixedText() );

and it might magically work, from a glance at the source code. You'd want to adjust LogPager::limitType() to handle this more explicitly, specifically in the $wgLogRestrictions check on about line 547, but it looks like it would be a simple change.

Tobias added a comment.Via ConduitApr 3 2009, 11:17 AM

Thanks for the pointers. I'm working on a new patch, but it's not ready yet, therefore I hereby remove the keywords...

Tobias added a comment.Via ConduitApr 20 2009, 7:16 PM

uses an array to fetch the move log as well as the deletion log. Fixes limitType (hopefully)

I tried to implement the proposal of Simetrical. I must admit, I don't really understand parts of the code (lines 565 and 567/571 of the patched LogEventsList). Therefore this patch should not be applied without testing it thoroughly.

attachment showmovelog-newversion.patch ignored as obsolete

bzimport added a comment.Via ConduitApr 21 2009, 12:07 AM

ayg wrote:

You should try to follow our coding style:

http://www.mediawiki.org/wiki/Manual:Coding_conventions

I haven't looked at the content of the patch, but hopefully I'll find time, since it's not too big.

bzimport added a comment.Via ConduitApr 21 2009, 1:26 AM

ayg wrote:

  • * @param $type String: A log type ('upload', 'delete', etc)

+ * @param $type Array: Log types ('upload', 'delete', etc)

"String or array" is more correct.

+ // If $type is not an array, make it an array
+ if ( !is_array( $type ) )
+ {
+ $type=array( "$type" );
+ }

This should just be:

		$type = (array)$type;

Also note some style issues here (opening brace is on the same line as the if, "=" should have spaces around it).

Also, $type should probably be renamed $types, it's a little confusing now.

  • if( isset($wgLogRestrictions[$type]) && !$wgUser->isAllowed($wgLogRestrictions[$type]) ) {

+ foreach ( $type as $i => $t ) {
+ if( isset( $wgLogRestrictions[$t] ) && !$wgUser->isAllowed($wgLogRestrictions[$t]) ) {
+ unset( $type["$i"] );
+ }

You seem to have too much indentation here. $type["$i"] seems a bit odd to me, may as well just be $type[$i]. Also, maybe best to avoid the extra $i and do $type = array_diff( $type, array( $t ) ) instead of the unset. If $type were renamed $types, you could do foreach ( $types as $type ) instead of the slightly confusing $t.

+ if ( count($type)==0 ) {

Style:

		if ( count( $type ) == 0 ) {

We use lots of spaces . . .

  • 'type' => 'delete',

+ 'type' => '',

  • array( 'type' => 'delete', 'page' => $this->mTitle->getPrefixedText() )

+ array( 'type' => '', 'page' => $this->mTitle->getPrefixedText() )

Is there a reason you're doing 'type' => '' instead of just dropping type altogether? Does that not work?

-'recreate-deleted-warn' => "'''Warning: You are recreating a page that was previously deleted.'''
+'moveddeleted-notice' => 'This page has been moved and/or deleted.
+The move and deletion log for the page are provided below for reference.',
+'recreate-moveddeleted-warn' => "'''Warning: You are recreating a page that was previously moved and/or deleted.'''

Moving without redirect is very unlikely compared to deletion, so I think it would be best to keep these saying just "deleted" instead of "moved and/or deleted". If you're going to keep both, I'd say "deleted and/or moved without redirect", which a) puts the common case first and b) makes it clear when this actually shows up (you won't see it for pages that were moved normally and not deleted).

(Why did you change the order of these two messages?)

I don't see any other problems. With these fixed, I'd be willing to commit it, if no problems show up when I test it out.

Tobias added a comment.Via ConduitApr 24 2009, 3:57 PM

Fixed (hopefully all :)) issues with the previous patch

attachment showmovelog-final.patch ignored as obsolete

bzimport added a comment.Via ConduitApr 24 2009, 5:24 PM

ayg wrote:

Committed as r49821 with the following tweaks:

  • * @param $type String: A log type ('upload', 'delete', etc)

+ * @param $type String or array: Log types ('upload', 'delete', etc)

$type -> $types (in r49822)

+ // If $type is not an array, make it an array

$type -> $types

+ $types = array_diff( $types, array( $t ) );

$t -> $type

+ if ( count( $types ) == 0 ) {
+ $types = '';
+ }

Removed, array() evaluates to boolean false, so this isn't needed.

Also note for the future that you shouldn't add RELEASE-NOTES changes to your patches -- they're easy for the committer to add, and they'll virtually always cause a conflict when the committer tries to apply the patch.

Tobias added a comment.Via ConduitApr 25 2009, 11:40 AM

Thanks for committing the patch and your help & pointers, Simetrical.

bzimport added a comment.Via ConduitJun 16 2009, 12:49 AM

herd wrote:

Looking at an example URL of a page moved with redirect suppression:

http://en.wikipedia.org/w/index.php?title=Flagship_Bank&redirect=no&action=edit&redlink=1
http://en.wikipedia.org/w/index.php?title=Flagship_Bank&redirect=no

The delete log is shown on both edit (create) and view, but the move log is only shown on view (which is very hard to get to, all redlinks including nstab-main point to edit).

Another example:
http://en.wikipedia.org/w/index.php?title=%22Sexy_Music%22_(Meat_Puppets_song)&action=edit&redlink=1
http://en.wikipedia.org/w/index.php?title=%22Sexy_Music%22_(Meat_Puppets_song)

Tobias added a comment.Via ConduitJun 16 2009, 7:40 AM

r51041 broke this while fixing bug 18747. I'm working on a fix, but I'm stuck:

I want to negate the condition array('log_action'=>'revision') so that logs are shown only if they are not revisiondelete-logs, how do I do that? An ugly alternative would be to explicitly list every log_action (delete, restore, move, move_redir). (The disadvantage of the latter fix is, that in the future every new log_action of log_type delete/move has to be included manually)

Catrope added a comment.Via ConduitJun 16 2009, 8:39 AM

(In reply to comment #20)

r51041 broke this while fixing bug 18747. I'm working on a fix, but I'm stuck:

I want to negate the condition array('log_action'=>'revision') so that logs are
shown only if they are not revisiondelete-logs, how do I do that? An ugly
alternative would be to explicitly list every log_action (delete, restore,
move, move_redir). (The disadvantage of the latter fix is, that in the future
every new log_action of log_type delete/move has to be included manually)

Just use "log_action != 'revision'"

Tobias added a comment.Via ConduitJun 16 2009, 9:09 AM

Fixes bug after r51041 broke it.

Attached: bugfix.patch

Tobias added a comment.Via ConduitJun 16 2009, 9:11 AM

Just use "log_action != 'revision'"

Ooops. Easy fix. I focused too much on the array structure that was previously in place.

bzimport added a comment.Via ConduitJun 16 2009, 10:28 AM

ayg wrote:

Committed as r51956 with whitespace fixes. See [[mw:Manual:Coding conventions]], we use spaces around all operators and inside all parentheses.

Add Comment

Column Prototype
This is a very early prototype of a persistent column. It is not expected to work yet, and leaving it open will activate other new features which will break things. Press "\" (backslash) on your keyboard to close it now.