r95496 MediaWiki - Code Review archive

• reverting r88111

Why adding a message in MessagesEn.php instead of removing the one from messages.inc?

-		'sp-contributions-showsizediff',

+'sp-contributions-showsizediff-warn'   => 'This instance has recent changes log limited to $1 . If you use this option, pages not in this log will not be displayed',

God know, svn reverted it fine, feel free to delete both messages. I'm away from computer till later

r95506

PHP Notice: Undefined property: ContribsPager::$showSizeDiff in /www/w/includes/specials/SpecialContributions.php on line 681

Also: getting an undefined index on line 129 since $this->opts['showSizeDiff'] is never set.

I don't think this is resolved. I'm running r95814 on my test wiki and pagination of my contributions is completely broken due to this revision. When I sync the previous revision of SpecialContributions.php (r93246) to my server, pagination works again and I can see my older contributions.

Works fine for me on trunk...

And for Roan/Niklas

So there were undisclosed changes in some of the previous revisions that were accidentally reverted here. Basically what needs to happen is...

		$fields = array(
			'page_namespace', 'page_title', 'page_is_new', 'page_latest', 'page_is_redirect',
			'page_len','rev_id', 'rev_page', 'rev_text_id', 'rev_timestamp', 'rev_comment',
			'rev_minor_edit', 'rev_user', 'rev_user_text', 'rev_parent_id', 'rev_deleted'
		);

		$queryInfo = array(
			'tables' => $tables,
			'fields' => array(
				'page_namespace', 'page_title', 'page_is_new', 'page_latest', 'page_is_redirect',
				'page_len','rev_id', 'rev_page', 'rev_text_id', 'rev_timestamp', 'rev_comment',
				'rev_minor_edit', 'rev_user', 'rev_user_text', 'rev_parent_id', 'rev_deleted',
				'rc_old_len', 'rc_new_len'
			),
			'conds' => $conds,
			'options' => array( 'USE INDEX' => array('revision' => $index) ),
			'join_conds' => $join_cond
		);

This section needs to be made sane. You're defining $fields twice, once with 'rc_old_len' and 'rc_new_len' and once without. One possible solution:

		$fields = array(
			'page_namespace', 'page_title', 'page_is_new', 'page_latest', 'page_is_redirect',
			'page_len','rev_id', 'rev_page', 'rev_text_id', 'rev_timestamp', 'rev_comment',
			'rev_minor_edit', 'rev_user', 'rev_user_text', 'rev_parent_id', 'rev_deleted'
		);

		$queryInfo = array(
			'tables' => $tables,
			'fields' => $fields,
			'conds' => $conds,
			'options' => array( 'USE INDEX' => array('revision' => $index) ),
			'join_conds' => $join_cond
		);

Later in the file...

		if( $this->target == 'newbies' ) {
			$tables = array( 'recentchanges', 'user_groups', 'page', 'revision' );
			$max = $this->mDb->selectField( 'user', 'max(user_id)', false, __METHOD__ );
			$condition[] = 'rev_user >' . (int)($max - $max / 100);
			$condition[] = 'ug_group IS NULL';
			$index = 'user_timestamp';
			# @todo FIXME: Other groups may have 'bot' rights
			$join_conds['user_groups'] = array( 'LEFT JOIN', "ug_user = rev_user AND ug_group = 'bot'" );
			$join_conds['recentchanges'] = array( 'INNER JOIN', "rev_id = rc_this_oldid" );
		} else {
			$tables = array( 'recentchanges', 'page', 'revision' );
			$condition['rev_user_text'] = $this->target;
			$join_conds['recentchanges'] = array( 'INNER JOIN', "rev_id = rc_this_oldid" );
			$index = 'usertext_timestamp';
		}

This is still doing an inner join on rev_id = rc_this_oldid. Don't do this. This could be switched to something like...

		if( $this->target == 'newbies' ) {
			$tables = array( 'recentchanges', 'user_groups', 'page', 'revision' );
			$max = $this->mDb->selectField( 'user', 'max(user_id)', false, __METHOD__ );
			$condition[] = 'rev_user >' . (int)($max - $max / 100);
			$condition[] = 'ug_group IS NULL';
			$index = 'user_timestamp';
			# @todo FIXME: Other groups may have 'bot' rights
			$join_conds['user_groups'] = array( 'LEFT JOIN', "ug_user = rev_user AND ug_group = 'bot'" );
			$join_conds['recentchanges'] = array( 'INNER JOIN', "rev_id = rc_this_oldid" );
		} else {
			$tables = array( 'page', 'revision' );
			$condition['rev_user_text'] = $this->target;
			$index = 'usertext_timestamp';
		}

And finally, this part needs a healthy evaluation:

		$diffOut = ' . . ' . $wgLang->getDirMark() .
			ChangesList::showCharacterDifference( $row->rc_old_len, $row->rc_new_len );

Thanks to Fran Rogers for helping me debug this mess.

The inner join on rev_id = rc_this_oldid is still there in your code

In the "if ... newbies" branch? It has to be, I think?

ALSO!

Stuff like this is much more useful as a patch

Bite me.

Also, are there unit tests for Special:Contributions? All of this newbie / diffchange / latest revisions only code seems very fragile.

No, there aren't any unit tests for Special:Contributions.

If I've made a b0rked merge, I'm happy to revert out and try again. If there are other issues with the code, that's a separate issue, and shouldn't really be part of the revert and merge...

As I said above, this is actively breaking Special:Contributions as of r95814. It's joining against recentchanges, making viewing older contributions impossible. Current example: <http://pruebita.com/wiki/Special:Contributions/MZMcBride>. I have contributions on this test site dating back for years, but it stops showing results from before June 2011. This needs to be fixed as soon as possible.

Aaah, yeah I guess changing INNER JOIN to LEFT JOIN might fix that.

How did this ever work before then? Looking at r88111, and as such, this revision, the reversion is just adding back the lines removed by r88111.

Not to be rude, but if you spent a minute reading through the code, you'd be able to see what went wrong. r88111 wasn't the first revision and it wasn't what broke Special:Contributions. r88008 broke Special:Contributions. r88111 fixed Special:Contributions (along with attempting to fix a separate issue). When you reverted r88111, you re-broke Special:Contributions. You can read all about this in the code comments of this revision, r88111, and r88008.

I already went through and described, in detail, what needs to be done in order to get Special:Contributions working again. I've also provided a working demonstration of the brokenness. Short of getting commit access and fixing the problem myself, I'm not sure what more I can really do here.

Doesn't really help much when revisions aren't tagged as followups and/or mentioned in commit summaries

Fixed properly in r96306 by reverting the entire r88008 saga.