Last modified: 2011-01-25 00:31:52 UTC

Wikimedia Bugzilla is closed!

Wikimedia migrated from Bugzilla to Phabricator. Bug reports are handled in Wikimedia Phabricator.
This static website is read-only and for historical purposes. It is not possible to log in and except for displaying bug reports and their history, links might be broken. See T18950, the corresponding Phabricator task for complete and up-to-date bug report information.
Bug 16950 - Show move log when viewing/creating a deleted page
Show move log when viewing/creating a deleted page
Status: RESOLVED FIXED
Product: MediaWiki
Classification: Unclassified
General/Unknown (Other open bugs)
unspecified
All All
: High enhancement with 5 votes (vote)
: ---
Assigned To: Church of emacs
: patch, patch-need-review
: 17334 18298 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-01-09 19:58 UTC by Church of emacs
Modified: 2011-01-25 00:31 UTC (History)
7 users (show)

See Also:
Web browser: ---
Mobile Platform: ---
Assignee Huggle Beta Tester: ---


Attachments
replaces showDeletionLog by a more general showLogs both in EditPage.php and Article.php (10.13 KB, patch)
2009-03-23 14:39 UTC, Church of emacs
Details
uses an array to fetch the move log as well as the deletion log. Fixes limitType (hopefully) (6.58 KB, patch)
2009-04-20 19:16 UTC, Church of emacs
Details
Fixed (hopefully all :)) issues with the previous patch (7.21 KB, patch)
2009-04-24 15:57 UTC, Church of emacs
Details
Fixes bug after r51041 broke it. (1.21 KB, patch)
2009-06-16 09:09 UTC, Church of emacs
Details

Description Church of emacs 2009-01-09 19:58:05 UTC
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.
Comment 1 Mike.lifeguard 2009-02-03 05:37:28 UTC
*** Bug 17334 has been marked as a duplicate of this bug. ***
Comment 2 Church of emacs 2009-02-03 09:26:23 UTC
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).
Comment 3 Roger W Haworth 2009-03-22 03:23:32 UTC
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. 
Comment 4 Roger W Haworth 2009-03-23 09:52:49 UTC
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.
Comment 5 Church of emacs 2009-03-23 09:59:27 UTC
I'm not really sure how this relates to the feature request described above. Perhaps a separate bug report is adequate for that problem.
Comment 6 Roger W Haworth 2009-03-23 10:02:33 UTC
It relates because it is another result of the new (and excellent) move-without-leaving-redirect option.
Comment 7 Benoit Blanchard 2009-03-23 11:59:14 UTC
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.
Comment 8 Church of emacs 2009-03-23 14:39:00 UTC
Created attachment 5953 [details]
replaces showDeletionLog by a more general showLogs both in EditPage.php and Article.php
Comment 9 Church of emacs 2009-03-23 14:39:46 UTC
added keywords
Comment 10 Mike.lifeguard 2009-04-01 20:43:07 UTC
*** Bug 18298 has been marked as a duplicate of this bug. ***
Comment 11 Aryeh Gregor (not reading bugmail, please e-mail directly) 2009-04-01 20:54:52 UTC
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.
Comment 12 Church of emacs 2009-04-03 11:17:57 UTC
Thanks for the pointers. I'm working on a new patch, but it's not ready yet, therefore I hereby remove the keywords...
Comment 13 Church of emacs 2009-04-20 19:16:55 UTC
Created attachment 6044 [details]
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.
Comment 14 Aryeh Gregor (not reading bugmail, please e-mail directly) 2009-04-21 00:07:06 UTC
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.
Comment 15 Aryeh Gregor (not reading bugmail, please e-mail directly) 2009-04-21 01:26:14 UTC
-	 * @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.
Comment 16 Church of emacs 2009-04-24 15:57:18 UTC
Created attachment 6055 [details]
Fixed (hopefully all :)) issues with the previous patch
Comment 17 Aryeh Gregor (not reading bugmail, please e-mail directly) 2009-04-24 17:24:55 UTC
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.
Comment 18 Church of emacs 2009-04-25 11:40:31 UTC
Thanks for committing the patch and your help & pointers, Simetrical.
Comment 19 Splarka 2009-06-16 00:49:37 UTC
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)
Comment 20 Church of emacs 2009-06-16 07:40:54 UTC
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)
Comment 21 Roan Kattouw 2009-06-16 08:39:24 UTC
(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'"
Comment 22 Church of emacs 2009-06-16 09:09:23 UTC
Created attachment 6230 [details]
Fixes bug after r51041 broke it.
Comment 23 Church of emacs 2009-06-16 09:11:39 UTC
> Just use "log_action != 'revision'"

Ooops. Easy fix. I focused too much on the array structure that was previously in place.
Comment 24 Aryeh Gregor (not reading bugmail, please e-mail directly) 2009-06-16 10:28:05 UTC
Committed as r51956 with whitespace fixes.  See [[mw:Manual:Coding conventions]], we use spaces around all operators and inside all parentheses.

Note You need to log in before you can comment on or make changes to this bug.


Navigation
Links