r96545 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r96544‎ | r96545 | r96546 >
Date:08:57, 8 September 2011
Author:nikerabbit
Status:ok (Comments)
Tags:
Comment:
Use LogFormatter to format log entries.
* Anonymous users now display correctly
This paves the way for new LogFormatters than enable proper i18n.
Each log type must be converted individually.
Modified paths:
  • /trunk/phase3/includes/LogEventsList.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/LogEventsList.php
——@@ -326,18 +326,19 @@
327327 * @return String: Formatted HTML list item
328328 */
329329 public function logLine( $row ) {
330 - $classes = array( 'mw-logline-' . $row->log_type );
331 - $title = Title::makeTitle( $row->log_namespace, $row->log_title );
332 - // Log time
333 - $time = $this->logTimestamp( $row );
334 - // User links
335 - $userLink = $this->logUserLinks( $row );
 330+ $entry = DatabaseLogEntry::newFromRow( $row );
 331+ $formatter = LogFormatter::newFromEntry( $entry );
 332+ $formatter->setShowUserToolLinks( !( $this->flags & self::NO_EXTRA_USER_LINKS ) );
 333+
 334+ $action = $formatter->getActionText();
 335+ $comment = $formatter->getComment();
 336+
 337+ $classes = array( 'mw-logline-' . $entry->getType() );
 338+ $title = $entry->getTarget();
 339+ $time = $this->logTimestamp( $entry );
 340+
336341 // Extract extra parameters
337342 $paramArray = LogPage::extractParams( $row->log_params );
338 - // Event description
339 - $action = $this->logAction( $row, $title, $paramArray );
340 - // Log comment
341 - $comment = $this->logComment( $row );
342343 // Add review/revert links and such...
343344 $revert = $this->logActionLinks( $row, $title, $paramArray, $comment );
344345
——@@ -350,68 +351,16 @@
351352 $classes = array_merge( $classes, $newClasses );
352353
353354 return Xml::tags( 'li', array( "class" => implode( ' ', $classes ) ),
354 - $del . "$time $userLink $action $comment $revert $tagDisplay" ) . "\n";
 355+ $del . "$time $action $comment $revert $tagDisplay" ) . "\n";
355356 }
356357
357 - private function logTimestamp( $row ) {
 358+ private function logTimestamp( LogEntry $entry ) {
358359 global $wgLang;
359 - $time = $wgLang->timeanddate( wfTimestamp( TS_MW, $row->log_timestamp ), true );
 360+ $time = $wgLang->timeanddate( wfTimestamp( TS_MW, $entry->getTimestamp() ), true );
360361 return htmlspecialchars( $time );
361362 }
362363
363364 /**
364 - * @param $row
365 - * @return String
366 - */
367 - private function logUserLinks( $row ) {
368 - if( self::isDeleted( $row, LogPage::DELETED_USER ) ) {
369 - $userLinks = '<span class="history-deleted">' .
370 - wfMsgHtml( 'rev-deleted-user' ) . '</span>';
371 - } else {
372 - $userLinks = Linker::userLink( $row->log_user, $row->user_name );
373 - // Talk|Contribs links...
374 - if( !( $this->flags & self::NO_EXTRA_USER_LINKS ) ) {
375 - $userLinks .= Linker::userToolLinks(
376 - $row->log_user, $row->user_name, true, 0, $row->user_editcount );
377 - }
378 - }
379 - return $userLinks;
380 - }
381 -
382 - /**
383 - * @param $row
384 - * @param $title
385 - * @param $paramArray
386 - * @return string
387 - */
388 - private function logAction( $row, $title, $paramArray ) {
389 - if( self::isDeleted( $row, LogPage::DELETED_ACTION ) ) {
390 - $action = '<span class="history-deleted">' .
391 - wfMsgHtml( 'rev-deleted-event' ) . '</span>';
392 - } else {
393 - $action = LogPage::actionText(
394 - $row->log_type, $row->log_action, $title, $this->skin, $paramArray, true );
395 - }
396 - return $action;
397 - }
398 -
399 - /**
400 - * @param $row
401 - * @return string
402 - */
403 - private function logComment( $row ) {
404 - if( self::isDeleted( $row, LogPage::DELETED_COMMENT ) ) {
405 - $comment = '<span class="history-deleted">' .
406 - wfMsgHtml( 'rev-deleted-comment' ) . '</span>';
407 - } else {
408 - global $wgLang;
409 - $comment = $wgLang->getDirMark() .
410 - Linker::commentBlock( $row->log_comment );
411 - }
412 - return $comment;
413 - }
414 -
415 - /**
416365 * @TODO: split up!
417366 *
418367 * @param $row

Follow-up revisions

RevisionCommit summaryAuthorDate
r96649This should fix regression reported in r96545...nikerabbit08:04, 9 September 2011

Comments

#Comment by Aaron Schulz (talk | contribs)   22:16, 8 September 2011

I'm getting: 

Notice: Undefined offset: 1 in D:\www\MW_trunk\phase3\extensions\FlaggedRevs\dataclasses\FlaggedRevsLog.php on line 156

...and log entries like: 

(show/hide) 18:15, 8 September 2011 Lightning Angel (Talk | contribs | block) protected "Documentation/zh"‎ ‎[edit=autoconfirmed] (infini) ‎[move=autoconfirmed] (infini) ‎ (hist | change)

My testwiki has fr as the content lang and I was using an en lang account.

#Comment by Nikerabbit (talk | contribs)   07:56, 9 September 2011

The log entry thing doesn't seem to be a regression, that is how it works currently, the [...] () thingies are formatted according to the content language and just concatenated. This should change when I convert the protection log.

#Comment by Aaron Schulz (talk | contribs)   07:58, 9 September 2011

Oh, to be clear, I'm talking about "(infini)", which should be "(indefinite)".

#Comment by Nikerabbit (talk | contribs)   07:59, 9 September 2011

Yes, that's exactly what I meant. It is not a regression. Compare with http://fi.wikipedia.org/wiki/Toiminnot:Loki/protect?uselang=en for example

#Comment by Nikerabbit (talk | contribs)   08:06, 9 September 2011

The notice you reported should be fixed in r96545, which should make your change in r96632 redundant. Marking as new.

#Comment by Aaron Schulz (talk | contribs)   22:40, 8 September 2011

Note sure exactly what commit to tag for this BTW.

#Comment by Nikerabbit (talk | contribs)   05:25, 9 September 2011

Can you reproduce it consistently? If so could you do a bisect to pinpoint the exact revision? I'd assume it is something to do with how parameters are parsed.

#Comment by Aaron Schulz (talk | contribs)   05:53, 9 September 2011

The first error occurs when viewing the stability log.

#Comment by Nikerabbit (talk | contribs)   05:57, 9 September 2011

It is a bit difficult for me to test, because I don't have a stability log. I think I have an idea for the second issue.

#Comment by Aaron Schulz (talk | contribs)   06:11, 9 September 2011

Just add FlaggedRevs to LocalSettings and run update.php. Go to the protection form and click the link to configure the page stability settings.

The problem definitely shows up with this revision (after trying various SVN versions), I'm still not sure where the offended code is.

#Comment by Aaron Schulz (talk | contribs)   05:59, 5 November 2011

getPerformer() gets called on each LogEventsListRow, which is a similar performance regression as what happened with BlockList recently. 

	public function getPerformer() {
		$userId = (int) $this->row->log_user;
		if ( $userId !== 0 ) {
			return User::newFromRow( $this->row );
		} else {
			$userText = $this->row->log_user_text;
			return User::newFromName( $userText, false );
		}
	}
#Comment by Nikerabbit (talk | contribs)   14:06, 5 November 2011

Can you elaborate what exactly is slow, and how it could be fixed?

#Comment by Tim Starling (talk | contribs)   03:15, 25 November 2011

When I view a log page, there are certainly a lot of queries, but the fault seems to lie with the Template:GENDER calls introduced in r96546 etc. rather than this revision. Here, User::newFromRow() seems to work successfully.

#Comment by Aaron Schulz (talk | contribs)   03:32, 25 November 2011

I must have been seeing newFromId() or something in my mind when I read this. That code is OK.

Status & tagging log