r111472 MediaWiki - Code Review archive

// fetch our activity database information
$feedback = $this->fetchFeedback( $feedbackId );

// get the string title for the page
$page = Title::newFromID( $feedback->af_page_id );

$feedback may be a ResultWrapper or boolean false, that should be checked

+ af_activity_count integer unsigned NOT NULL DEFAULT 0

+ if( !$wgUser->isAllowed( 'aftv5-hide-feedback' )) {
+ return;
+ }

You should return an error message rather than just returning nothing. Use $this->dieUsage( "You don't have permission to hide feedback", 'permissiondenied' ); or something.

+ $page = Title::newFromID( $feedback->af_page_id );
+ $title = $page->getPartialURL();

This will throw a fatal error if someone supplies a bad page ID, because Title::newFromID() can return null. This is in addition to what Benny said about $feedback possibly being false. And this also needs ->getText() rather than ->getPartialURL(), just like the other instance we talked about earlier. Note that ->getText() will return a string with spaces rather than underscores, so for use in DB queries against log_title you will need to use ->getDBKey() instead. You can also use ->getDBKey() throughout if you want, the Title constructors accept both text form and dbkey form (but not URL form).

+ // <div>Feedback Post #{$feedbackid} by {$user_link}</div>
+ $html .= Html::openElement( 'div', array() );
+ $html .= wfMessage( 'articlefeedbackv5-activity-feedback-info',
+ array($feedback->af_id))->text()
+ . $this->getUserLink($feedback->af_user_id, $feedback->af_user_ip);
+ $html .= Html::closeElement( 'div' );

This is what we call lego messages, and should be avoided. String concatenation is not a valid replacement for $1 parameters. Instead, you should add a $2 parameter to the message that takes the user link. Because it's HTML, you'll need to use ->rawParams() rather than ->params() to set it.

+ array( $feedback->af_activity_count ))->text() );

This needs number formatting, i.e. array( $wgLang->formatNum( $feedback->af_activity_count ) ) . Also,

+ 'articlefeedbackv5-activity-count' => '$1 actions on this post',

needs pluralization, i.e. $1 actions on this post . This doesn't seem terribly important in English, but other language (either Polish or Russian, I forget) have plural cases that resemble numbering in English (-st, -nd, -rd and -th) so it definitely matters for those.

+ $html .= $this->getUserLink($item->log_user, $item->log_user_text)
+ . Html::element( 'div', array(),
+ wfMessage( 'articlefeedbackv5-activity-' . $item->log_action,
+ array())->text() )
+ . $wgLang->timeanddate( $item->log_timestamp );

This is also lego.

+ if (!empty($item->log_comment)) {

Don't use empty(), it suppresses all errors, so if someone misspelled log_comment here no one would ever find out. Perhaps you meant !$item->log_comment or $item->log_comment == or something else.

+ // figure out if we have more based on our new continue value
+ $more = $this->fetchHasMore($title, $feedbackId, $continue);

The preferred way to do paging in MediaWiki is to fetch one row more than you need (i.e. 'LIMIT' => $limit + 1) and then have a counter that ensures that excess row is never output, but its ID is used as the continue value. This is different from what you're doing, you're outputting the ID of the last row as the continue value and then checking separately if there are more results. With my suggested approach, you would have to use foo >= $continue rather than foo > $continue , but you wouldn't have to check separately if there are more results (presence or absence of the excess row indicates this) and you wouldn't have to output this fact separately either (presence or absence of the continue parameter indicates this).

Reading the actual query, it looks like you are requesting $limit+1 rows, but the code that processes these rows is not aware of it? I don't quite get it.

+ $result->addValue( $this->getModuleName(), 'continue', $continue );

The standard way of doing this is $this->setContinueEnumParameter( 'continue', $continue ); . That will put it in the query-continue element.

+ $where[] = 'log_id < ' . intval($continue);
[...]
+ 'ORDER BY' => 'log_timestamp DESC'

These don't mix well together (read: the database really will not like this query). You'll need to do your ordering and your continuation using the same field or set of fields. I think you can probably get away with ordering by log_id instead of log_timestamp, because out-of-order entries should be very rare and will probably only be off by one or two seconds if at all (and presumably your timestamp display in the UI shows timestamps to the minute, not to the second).

+ parent::__construct( $query, $moduleName, 'af' );

Module prefixes should be unique, and I'm pretty sure 'af' is already in use by another AFTv5 module.

These should now all be fixed - took at least 6 different commits for the different issues

Looks like this has all been taken care of.