r62506 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r62505‎ | r62506 | r62507 >
Date:09:51, 15 February 2010
Author:happydog
Status:reverted (Comments)
Tags:
Comment:
CodeReview: MWTidy was only introduced in v1.15, so I've fixed the new CodeCommentLinker::truncateHtml() function (introduced in r62488) to use parser::tidy() instead of MWTidy::tidy(). The former is now just a wrapper function for the latter, and for core code the latter should be used. However extensions should continue to use the former (which is functionally identical) in order to maintain backwards compatibility with pre 1.15 versions of MediaWiki.
Modified paths:
  • /trunk/extensions/CodeReview/backend/CodeCommentLinker.php (modified) (history)

Diff [purge]

Index: trunk/extensions/CodeReview/backend/CodeCommentLinker.php
——@@ -34,7 +34,7 @@
3535 if( strlen($text) <= $maxLen ) {
3636 return $text; // string short enough even *with* HTML
3737 }
38 - $text = MWTidy::tidy( $text ); // fix tags
 38+ $text = parser::tidy( $text ); // fix tags
3939 $displayLen = 0;
4040 $doTruncate = true; // truncated string plus '...' shorter than original?
4141 $tagType = 0; // 0-open, 1-close

Follow-up revisions

RevisionCommit summaryAuthorDate
r62700Revert r62506 (use Parser::tidy() for b/c instead of newer MWTidy::tidy()). K...demon11:22, 19 February 2010

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r62488* Fixed bug 22326: "Truncated urls in summaries lead to different places"...aaron03:07, 15 February 2010

Comments

#Comment by Aaron Schulz (talk | contribs)   18:31, 15 February 2010

Note that extensions are branched with /trunk, so there is a 1.15 CodeReview version and this will be 1.16. This avoids a lot of the b/c hacks that extensions had.

#Comment by HappyDog (talk | contribs)   11:29, 16 February 2010

In this case, I wouldn't really call it a hack. No point breaking b/c if it doesn't gain us anything.

#Comment by Aaron Schulz (talk | contribs)   20:53, 17 February 2010

Deprecated functions have a tendancy to get removed...it helps when things don't use them.

#Comment by HappyDog (talk | contribs)   17:06, 18 February 2010

This kind of thing is always a balancing act between serving the needs of users (who may not be on the latest version) and that of the developers (who see this extra code as 'clutter' and an extra maintenance burden).

I would assume that anyone removing the function would do a grep to make sure that any code using it gets updated to use the new function instead, so it should be safe to leave it as is until that happens. If the concern is that they won't check extensions (even though they are in use on MW servers) then there are two solutions, that I can see:

  1. Add a comment to the deprecated function, reminding people that it may be used by extensions so to check carefully before removing it.
  2. Bullet-proof the extension, so it won't break if/when the function is removed:
   if ( class_exists( "MWTidy" ) )
      $text = MWTidy::tidy( $text );
   else
      $text = parser::tidy( $text );

(The third option, to break backwards-compatibility is, imho, far too premature and will needlessly break existing installations, including my own. Upgrading will happen in time, but it shouldn't be forced onto users unnecessarily.)

#Comment by Tim Starling (talk | contribs)   08:04, 19 February 2010

I don't really care which you use as long as you capitalise Parser correctly.

Status & tagging log