r62697 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r62696‎ | r62697 | r62698 >
Date:10:37, 19 February 2010
Author:churchofemacs
Status:ok (Comments)
Tags:
Comment:
LogExtract on blocked user's pages: fixing r62241 (see comments by Tim)
Modified paths:
  • /trunk/phase3/includes/Article.php (modified) (history)
  • /trunk/phase3/includes/EditPage.php (modified) (history)
  • /trunk/phase3/languages/messages/MessagesEn.php (modified) (history)
  • /trunk/phase3/maintenance/language/messages.inc (modified) (history)

Diff [purge]

Index: trunk/phase3/maintenance/language/messages.inc
——@@ -548,6 +548,7 @@
549549 'noarticletextanon',
550550 'userpage-userdoesnotexist',
551551 'userpage-userdoesnotexist-view',
 552+ 'blocked-notice-logextract',
552553 'clearyourcache',
553554 'usercssyoucanpreview',
554555 'userjsyoucanpreview',
Index: trunk/phase3/includes/Article.php
——@@ -1212,23 +1212,23 @@
12131213 if ( $this->mTitle->getNamespace() == NS_USER || $this->mTitle->getNamespace() == NS_USER_TALK ) {
12141214 $parts = explode( '/', $this->mTitle->getText() );
12151215 $rootPart = $parts[0];
1216 - $id = User::idFromName( $rootPart );
 1216+ $user = User::newFromName( $rootPart, false /* allow IP users*/ );
12171217 $ip = User::isIP( $rootPart );
1218 - if ( $id == 0 && !$ip ) { # User does not exist
 1218+ if ( !$user->isLoggedIn() && !$ip ) { # User does not exist
12191219 $wgOut->wrapWikiMsg( "<div class=\"mw-userpage-userdoesnotexist error\">\n\$1</div>",
12201220 array( 'userpage-userdoesnotexist-view', $rootPart ) );
1221 - } else if ( User::newFromId( $id )->isBlocked() ) { # Show log extract if the user is currently blocked
 1221+ } else if ( $user->isBlocked() ) { # Show log extract if the user is currently blocked
12221222 LogEventsList::showLogExtract(
12231223 $wgOut,
12241224 'block',
1225 - $this->mTitle->getSubjectPage()->getPrefixedText(),
 1225+ $user->getUserPage()->getPrefixedText(),
12261226 '',
12271227 array(
12281228 'lim' => 1,
12291229 'showIfEmpty' => false,
12301230 'msgKey' => array(
1231 - 'sp-contributions-blocked-notice',
1232 - $this->mTitle->getSubjectPage()->getPrefixedText() # Support GENDER in notice
 1231+ 'blocked-notice-logextract',
 1232+ $user->getName() # Support GENDER in notice
12331233 )
12341234 )
12351235 );
Index: trunk/phase3/includes/EditPage.php
——@@ -779,23 +779,23 @@
780780 if ( $namespace == NS_USER || $namespace == NS_USER_TALK ) {
781781 $parts = explode( '/', $this->mTitle->getText(), 2 );
782782 $username = $parts[0];
783 - $id = User::idFromName( $username );
 783+ $user = User::newFromName( $username, false /* allow IP users*/ );
784784 $ip = User::isIP( $username );
785 - if ( $id == 0 && !$ip ) { # User does not exist
 785+ if ( !$user->isLoggedIn() && !$ip ) { # User does not exist
786786 $wgOut->wrapWikiMsg( "<div class=\"mw-userpage-userdoesnotexist error\">\n$1</div>",
787787 array( 'userpage-userdoesnotexist', $username ) );
788 - } else if (User::newFromId($id)->isBlocked()) { # Show log extract if the user is currently blocked
 788+ } else if ( $user->isBlocked() ) { # Show log extract if the user is currently blocked
789789 LogEventsList::showLogExtract(
790790 $wgOut,
791791 'block',
792 - $this->mTitle->getSubjectPage()->getPrefixedText(),
 792+ $user->getUserPage()->getPrefixedText(),
793793 '',
794794 array(
795795 'lim' => 1,
796796 'showIfEmpty' => false,
797797 'msgKey' => array(
798 - 'sp-contributions-blocked-notice',
799 - $this->mTitle->getSubjectPage()->getPrefixedText() # Support GENDER in notice
 798+ 'blocked-notice-logextract',
 799+ $user->getName() # Support GENDER in notice
800800 )
801801 )
802802 );
Index: trunk/phase3/languages/messages/MessagesEn.php
——@@ -1264,6 +1264,8 @@
12651265 'userpage-userdoesnotexist' => 'User account "$1" is not registered.
12661266 Please check if you want to create/edit this page.',
12671267 'userpage-userdoesnotexist-view' => 'User account "$1" is not registered.',
 1268+'blocked-notice-logextract' => 'This user is currently blocked.
 1269+The latest block log entry is provided below for reference:',
12681270 'clearyourcache' => "'''Note: After saving, you may have to bypass your browser's cache to see the changes.'''
12691271 '''Mozilla / Firefox / Safari:''' hold ''Shift'' while clicking ''Reload'', or press either ''Ctrl-F5'' or ''Ctrl-R'' (''Command-R'' on a Macintosh);
12701272 '''Konqueror: '''click ''Reload'' or press ''F5'';

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r62241showLogExtract of block log on user (talk) pages of blocked userschurchofemacs13:08, 10 February 2010

Comments

#Comment by P.Copp (talk | contribs)   18:19, 19 February 2010

An entry in RELEASE-NOTES would be awesome :)

Also, I wonder if code duplication could be reduced a bit by moving this into an own function. (LogEventsList::showBlockLogExtract( $user, ... ) or something)

#Comment by Church of emacs (talk | contribs)   18:47, 19 February 2010

I added a note to RELEASE-NOTES in r62714. About your second comment: I already reduced code duplication by creating LogEventsList::showLogExtract – before that, every request created a pager manually. With showLogExtract being used more and more, the function also got more arguments and that's basically how it is now. It's imho still relatively easy to call showLogExtract.

Afaik block log extracts are shown in Article.php, EditPage.php and SpecialContributions.php. Three calls doesn't justify a new function imho, also you have to consider that for example delete/move log extracts are also both in Article.php and EditPage.php. We don't want a new function for every use case of showLogExtract.

#Comment by P.Copp (talk | contribs)   18:53, 19 February 2010

We don't want a new function for every use case of showLogExtract. - Why not? :D

Anyway, it's your decision. I wouldn't want 14 lines of identical code on two or more places in my own code, but that's probably just my personal taste :)

#Comment by Nikerabbit (talk | contribs)   19:42, 19 February 2010

Could refactoring help here? Maybe a new class with sane default values for options?

#Comment by Church of emacs (talk | contribs)   20:13, 19 February 2010

I'm not really sure what sane default values would be like, in contrast to current default values.

Let's look at showLogExtract's parameters:

  1. $out: Sometimes you don't want to print the extract to $wgOut directly. String-by-reference for $out could be useful, although it's currently not used much.
  2. $types: obviously non-optional
  3. $page: already has default value ''
  4. $user: already has default value ''
  5. $param: optional array of additional parameters:
    • lim: Obviously useful. E.g. for a block notice, it's currently 1, for deletion/move notice it's (iirc) 10
    • conds: Additional conditions. Needed for "log_action != 'revision'" when showing deletion log
    • showIfEmpty: There quite some occasions where this is set to false and where this is set to true. Needed.
    • msgKey: obviously needed and useful. You need to tell users what log you are showing to them
    • offset: Needed for block log on Special:Contributions
    • wrap: useful for adding CSS classes

I don't really see much potential for using sane default values other than what's currently already being done.

Regards, --Church of emacs

#Comment by Nikerabbit (talk | contribs)   07:57, 20 February 2010

What I was trying to say, was that if it is a class, there is no need for function which takes ten parameters and array of options.

#Comment by Church of emacs (talk | contribs)   11:45, 22 February 2010

I suppose that'd be possible. Though I'm not sure if it's worth the effort.

#Comment by Tim Starling (talk | contribs)   10:05, 22 February 2010

Thanks CoE, looks good.

Status & tagging log