r97342 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r97341‎ | r97342 | r97343 >
Date:22:30, 16 September 2011
Author:sean_colombo
Status:resolved (Comments)
Tags:
Comment:
Merged in changes from LogEventsList which prevent missing usernames in log-lines and adds hook LogEventsListShowLogExtract. Part of merge from here: http://www.mediawiki.org/wiki/Wikia_code
Modified paths:
  • /trunk/phase3/includes/LogEventsList.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/LogEventsList.php
@@ -326,6 +326,7 @@
327327 * @return String: Formatted HTML list item
328328 */
329329 public function logLine( $row ) {
 330+ $row->user_name = $this->fixUserName($row->user_name, $row->log_user);
330331 $entry = DatabaseLogEntry::newFromRow( $row );
331332 $formatter = LogFormatter::newFromEntry( $entry );
332333 $formatter->setShowUserToolLinks( !( $this->flags & self::NO_EXTRA_USER_LINKS ) );
@@ -706,6 +707,11 @@
707708 $s = str_replace( '$1', $s, $wrap );
708709 }
709710
 711+ /* hook can return false, if we don't want the message to be emitted (Wikia BugId:7093) */
 712+ if (!wfRunHooks('LogEventsListShowLogExtract', array(&$s, $types, $page, $user, $param))) {
 713+ return $pager->getNumRows();
 714+ }
 715+
710716 // $out can be either an OutputPage object or a String-by-reference
711717 if( $out instanceof OutputPage ){
712718 $out->addHTML( $s );
@@ -740,6 +746,23 @@
741747 }
742748 return false;
743749 }
 750+
 751+ /**
 752+ * if user_name is empty - use User class to get his name
 753+ * @param $user_name string
 754+ * @param $user_id integer
 755+ * @return string
 756+ */
 757+ public function fixUserName($user_name, $user_id) {
 758+ if ( empty($user_name) ) {
 759+ $oUser = User::newFromID($user_id);
 760+ if ( $oUser instanceof User ) {
 761+ $user_name = $oUser->getName();
 762+ }
 763+ }
 764+
 765+ return $user_name;
 766+ }
744767 }
745768
746769 /**
@@ -1004,6 +1027,10 @@
10051028 if( $this->getNumRows() > 0 ) {
10061029 $lb = new LinkBatch;
10071030 foreach ( $this->mResult as $row ) {
 1031+ $row->user_name = $this->mLogEventsList->fixUserName($row->user_name, $row->log_user);
 1032+ if ( empty($row->user_name) ) {
 1033+ continue;
 1034+ }
10081035 $lb->add( $row->log_namespace, $row->log_title );
10091036 $lb->addObj( Title::makeTitleSafe( NS_USER, $row->user_name ) );
10101037 $lb->addObj( Title::makeTitleSafe( NS_USER_TALK, $row->user_name ) );

Follow-up revisions

RevisionCommit summaryAuthorDate
r97348Followup to r97342 to add docs for new hook.sean_colombo23:14, 16 September 2011
r98156Followup to r97342 to remove the now-unneeded checks for empty username & to ...sean_colombo18:49, 26 September 2011

Comments

#Comment by Reedy (talk | contribs)   22:32, 16 September 2011

If you're commiting in new hooks, please add them to docs/hooks.txt, and also create an onwiki page

ALSO, we no liek empty()

Aswell as there not being enough spaces in your code ;)

#Comment by SColombo~mediawikiwiki (talk | contribs)   23:16, 16 September 2011

Thanks for the tips! Added to docs/hooks.txt, added page (linked above).

After reading that page, I think empty() is still what we want there since were testing for null, completely unset, etc.

If I'm misinterpreting that page & there's a more MediaWiki way you guys prefer to do it, please tag that as a followup commit so I can learn :) thanks!

#Comment by Nikerabbit (talk | contribs)   07:36, 17 September 2011

As far as I can see this code is no longer needed after my recent refactoring. Can you elaborate on the "missing usernames" part?

#Comment by Nikerabbit (talk | contribs)   05:50, 20 September 2011

Are you there?

#Comment by SColombo~mediawikiwiki (talk | contribs)   20:24, 20 September 2011

Sorry for not responding earlier... I couldn't find the details myself enough to answer if the refactoring would remove the case where we were seeing missing usernames, so I pinged Macbre who made the changes originally.

Unfortunately, he's on vacation this week and will return on Monday.

The hook part, I was able to look into and it DOES still seem like something we should keep even if we remove the username checks.

#Comment by Nikerabbit (talk | contribs)   06:27, 21 September 2011

Hook seems fine yes.

#Comment by SColombo~mediawikiwiki (talk | contribs)   18:32, 26 September 2011

Looked at it more, it is actually this changeset which introduced the other changes: http://trac.wikia-code.com/changeset/24640

The changes seem to be because of sharded user-tables. Any thoughts on how this meshes with your changes (or if this is so Wikia-specific that we should use hooks or something?).

#Comment by Nikerabbit (talk | contribs)   18:38, 26 September 2011

I've already fixed the join from INNER to LEFT and changed the display code to handle unregistered users. I don't see anything that this change adds (except the hook).

#Comment by SColombo~mediawikiwiki (talk | contribs)   18:42, 26 September 2011

Awesome... thanks for the review! I'll roll back all the changes other than the hook.

#Comment by SColombo~mediawikiwiki (talk | contribs)   21:07, 28 September 2011

I think it's fixed... set to "new"

Status & tagging log