r90101 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r90100‎ | r90101 | r90102 >
Date:23:50, 14 June 2011
Author:brion
Status:ok (Comments)
Tags:
Comment:
Revert r86764, r89134, r86827 -- added a second opaque boolean parameter to Linker::commentBlock() which appeared to mostly just turn it into Linker::formatComment().

commentBlock() exists for the sole purpose of embedding a comment into parentheses if it exists so you can append it to a line of text -- if you're not putting stuff in parentheses, don't use commentBlock() because you're not generating a parenthesized comment block.
Opaque boolean parameters are also very poor form, especially when tacking on multiple ones. There was already a nasty optional '$local' boolean param, forcing all uses of this other parameter to add *two* parameters, making illegible stuff like 'false, false'.
Modified paths:
  • /trunk/phase3/includes/ImagePage.php (modified) (history)
  • /trunk/phase3/includes/Linker.php (modified) (history)
  • /trunk/phase3/includes/specials/SpecialListfiles.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/ImagePage.php
@@ -1111,7 +1111,7 @@
11121112 if ( $file->isDeleted( File::DELETED_COMMENT ) ) {
11131113 $row .= '<span class="history-deleted">' . wfMsgHtml( 'rev-deleted-comment' ) . '</span>';
11141114 } else {
1115 - $row .= $this->skin->commentBlock( $description, $this->title, false, false );
 1115+ $row .= $this->skin->commentBlock( $description, $this->title );
11161116 }
11171117 $row .= '</td>';
11181118
Index: trunk/phase3/includes/Linker.php
@@ -1343,10 +1343,10 @@
13441344 * @param $comment String
13451345 * @param $title Mixed: Title object (to generate link to section in autocomment) or null
13461346 * @param $local Boolean: whether section links should refer to local page
1347 - * @param $embraced Boolean: whether the formatted comment should be embraced with ()
 1347+ *
13481348 * @return string
13491349 */
1350 - static function commentBlock( $comment, $title = null, $local = false, $embraced = true ) {
 1350+ static function commentBlock( $comment, $title = null, $local = false ) {
13511351 // '*' used to be the comment inserted by the software way back
13521352 // in antiquity in case none was provided, here for backwards
13531353 // compatability, acc. to brion -ævar
@@ -1354,10 +1354,7 @@
13551355 return '';
13561356 } else {
13571357 $formatted = self::formatComment( $comment, $title, $local );
1358 - if ( $embraced ) {
1359 - $formatted = wfMessage( 'parentheses' )->rawParams( $formatted )->escaped();
1360 - }
1361 - return Html::rawElement( 'span', array( 'class' => 'comment' ), $formatted );
 1358+ return " <span class=\"comment\">($formatted)</span>";
13621359 }
13631360 }
13641361
@@ -1377,7 +1374,7 @@
13781375 if ( $rev->isDeleted( Revision::DELETED_COMMENT ) && $isPublic ) {
13791376 $block = " <span class=\"comment\">" . wfMsgHtml( 'rev-deleted-comment' ) . "</span>";
13801377 } else if ( $rev->userCan( Revision::DELETED_COMMENT ) ) {
1381 - $block = ' ' . self::commentBlock( $rev->getComment( Revision::FOR_THIS_USER ),
 1378+ $block = self::commentBlock( $rev->getComment( Revision::FOR_THIS_USER ),
13821379 $rev->getTitle(), $local );
13831380 } else {
13841381 $block = " <span class=\"comment\">" . wfMsgHtml( 'rev-deleted-comment' ) . "</span>";
Index: trunk/phase3/includes/specials/SpecialListfiles.php
@@ -221,7 +221,7 @@
222222 case 'img_size':
223223 return $this->getSkin()->formatSize( $value );
224224 case 'img_description':
225 - return $this->getSkin()->commentBlock( $value, null, false, false );
 225+ return $this->getSkin()->commentBlock( $value );
226226 case 'count':
227227 return intval( $value ) + 1;
228228 }

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r86764Embrace comment with () only when really needed. In Special:ListFiles and Ima...raymond13:23, 23 April 2011
r86827Followup r86764: don't parse the comment. Spotted by SPQRobin@Translatewiki a...raymond17:58, 24 April 2011
r89134Add back space between size and comment in history pages (fix for r86764)aaron20:33, 29 May 2011

Comments

#Comment by Brion VIBBER (talk | contribs)   23:52, 14 June 2011

Reverts stuff that made it into REL1_18 from 1.18 code review; revert needs merging to branch before release.

Status & tagging log