MediaWiki r41710 - Code Review

Jump to: navigation, search
Repository:MediaWiki
Revision:r41709‎ | r41710 (on ViewVC)‎ | r41711 >
Date:20:27, 5 October 2008
Author:skizzerz
Status:old (Comments)
Tags:
Comment:
* integrate Poem extension into core (patch by Nathaniel Herman)
Modified paths:

Diff [purge]

Index: trunk/phase3/CREDITS
===================================================================
--- trunk/phase3/CREDITS	(revision 41709)
+++ trunk/phase3/CREDITS	(revision 41710)
@@ -53,6 +53,7 @@
 * Michael De La Rue
 * Mike Horvath
 * Mormegil
+* Nathaniel Herman
 * RememberTheDot
 * ST47
 
Index: trunk/phase3/includes/parser/Parser.php
===================================================================
--- trunk/phase3/includes/parser/Parser.php	(revision 41709)
+++ trunk/phase3/includes/parser/Parser.php	(revision 41710)
@@ -127,7 +127,7 @@
 		$this->mTransparentTagHooks = array();
 		$this->mFunctionHooks = array();
 		$this->mFunctionSynonyms = array( 0 => array(), 1 => array() );
-		$this->mDefaultStripList = $this->mStripList = array( 'nowiki', 'gallery' );
+		$this->mDefaultStripList = $this->mStripList = array( 'nowiki', 'gallery', 'poem' );
 		$this->mUrlProtocols = wfUrlProtocols();
 		$this->mExtLinkBracketedRegex = '/\[(\b(' . wfUrlProtocols() . ')'.
 			'[^][<>"\\x00-\\x20\\x7F]+) *([^\]\\x0a\\x0d]*?)\]/S';
@@ -3304,6 +3304,9 @@
 				case 'gallery':
 					$output = $this->renderImageGallery( $content, $attributes );
 					break;
+				case 'poem':
+					$output = $this->renderPoem( $content, $attributes );
+					break;
 				default:
 					if( isset( $this->mTagHooks[$name] ) ) {
 						# Workaround for PHP bug 35229 and similar
@@ -4173,6 +4176,40 @@
 		return $ig->toHTML();
 	}
 
+	/** Renders any text in between <poem></poem> tags
+	 * based on http://www.mediawiki.org/wiki/Extension:Poem
+	 */
+
+	function renderPoem( $in, $param=array() ) {
+
+		/* using newlines in the text will cause the parser to add <p> tags,
+ 	 	* which may not be desired in some cases
+	 	*/
+		$nl = isset( $param['compact'] ) ? '' : "\n";
+  
+		$tag = $this->insertStripItem( "<br />", $this->mStripState );
+		$text = preg_replace(
+			array( "/^\n/", "/\n$/D", "/\n/", "/^( +)/me" ),
+			array( "", "", "$tag\n", "str_replace(' ','&nbsp;','\\1')" ),
+			$in );
+		$text = $this->recursiveTagParse( $text );
+
+		// Pass HTML attributes through to the output.
+		$attribs = Sanitizer::validateTagAttributes( $param, 'div' );
+
+		// Wrap output in a <div> with "poem" class.
+		if( isset( $attribs['class'] ) ) {
+			$attribs['class'] = 'poem ' . $attribs['class'];
+		} else {
+			$attribs['class'] = 'poem';
+		}
+
+		return wfOpenElement( 'div', $attribs ) .
+			$nl .
+			trim( $text ) .
+			"$nl</div>";
+	}
+
 	function getImageParams( $handler ) {
 		if ( $handler ) {
 			$handlerClass = get_class( $handler );
Index: trunk/phase3/RELEASE-NOTES
===================================================================
--- trunk/phase3/RELEASE-NOTES	(revision 41709)
+++ trunk/phase3/RELEASE-NOTES	(revision 41710)
@@ -60,6 +60,7 @@
 * RenderHash
 * NoMoveUserPages
 * Special:Nuke to mass delete all pages created by a user
+* Poem (patch by Nathaniel Herman)
 
 === New features in 1.14 ===
 

Follow-up revisions

Rev.Commit summaryAuthorDate
r41978* fixes for the merged Poem extension, per comments by Tim Starling on CodeRe...skizzerz21:53, 11 October 2008
r43520Revert r41710, r41978, r42012, r42048 (integration of Poem extension to core ...demon01:19, 15 November 2008

Comments

#Comment by Tim Starling (Talk | contribs)   15:09, 11 October 2008

Revert or fix:

  • Don't use /e modifier, it's against our security policy. There are some alternatives in StringUtils.php if you're desperate.
  • Improper escaping "$tag\n", may contain metacharacters
  • Note that every element of the match array will cause a regex compilation and execution, and will decrease performance. The first two can apparently be replaced with trim($in, "\n") which would be much faster. The third would be faster with str_replace(), only the fourth actually needs to be a regex.
  • I need convincing that insertStripItem() is actually necessary. Please test.
  • Use Xml::openElement() not wfOpenElement()
#Comment by Skizzerz (Talk | contribs)   19:46, 11 October 2008
  • Don't use /e modifier, it's against our security policy. There are some alternatives in StringUtils.php if you're desperate.

Is preg_replace_callback ok?

  • Improper escaping "$tag\n", may contain metacharacters

The only metacharacter is \x7f (the delete character), because that's what is set in the Parser as the UNIQ prefix. Since $tag is not user input (it is a strip tag from the parser), and because that needs to be there for the parser to unstrip it properly, I don't see the issue.

  • Note that every element of the match array will cause a regex compilation and execution, and will decrease performance.

The first two can apparently be replaced with trim($in, "\n") which would be much faster. The third would be faster with str_replace(), only the fourth actually needs to be a regex.

trim() doesn't provide the exact functionality (since it should only remove the very first and very last \n), so I used some subst() magic instead for that. str_replace is used for the third and the final uses preg_replace_callback (see point 1)

  • I need convincing that insertStripItem() is actually necessary. Please test.

Unfortunately, it is. Without it, using <nowiki> in the <poem> tag does not work across multiple lines. e.g.

<poem><nowiki>
This
is
a
test
</nowiki></poem>

would output "This<br />is<br />a<br />test" instead of having actual linebreaks

  • Use Xml::openElement() not wfOpenElement()

Done

#Comment by Skizzerz (Talk | contribs)   21:58, 11 October 2008

See [[Special:Code/MediaWiki/41978|r41978]] for the fix I applied.

#Comment by ^demon (Talk | contribs)   17:50, 13 October 2008

I'm not 100% sure why Poem is going into core. I suppose DeletedContributions and RenameUser should go in (arguably, they're features that should've always been in core)...but custom wiki tag extensions seem a little less crucial.

#Comment by Danny B. (Talk | contribs)   23:16, 18 October 2008

I agree with ^demon that Poem shouldn't go into core. Also please keep in mind, if it will get into core, that it's not being used for poems only, but for many other purposes - it's kinda lightweight "pre" tag. So I'd suggest more universal and function-descriptive name, eg. "Wraplines" or something like that.

#Comment by Simetrical (Talk | contribs)   20:10, 19 October 2008

If there's some reason for this to go into core, it *really* should not be called "poem". That's just way too narrow for the functionality it actually represents.

#Comment by ^demon (Talk | contribs)   21:18, 19 October 2008

Can't get a clean revert on this either.

#Comment by Skizzerz (Talk | contribs)   21:25, 19 October 2008

renaming the tag is trivial, but I kept it as-is mainly for the reason that if we changed it to something other than poem, it would mean that it'd have to be changed on all of the wmf wikis.

I don't see the harm in keeping it in core, however. Its benefits far outweigh its downsides. As for what to rename it to... we'll need to get some consensus for that.

#Comment by Brion VIBBER (Talk | contribs)   01:14, 24 October 2008

Regex issues were resolved in later edits.

Status & tagging log

  • 15:32, 12 September 2011 Meno25 (Talk | contribs) changed the status of r41710 [removed: resolved added: old]
Personal tools
Namespaces
Variants
Views
Actions
Site
Support
Download
Development
Communication
Toolbox