r41983 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r41982‎ | r41983 | r41984 >
Date:02:20, 12 October 2008
Author:skizzerz
Status:old (Comments)
Tags:
Comment:
Modified paths:
  • /trunk/phase3/includes/parser/Parser.php (modified) (history)
  • /trunk/phase3/maintenance/parserTests.txt (modified) (history)

Diff [purge]

Index: trunk/phase3/maintenance/parserTests.txt
@@ -7234,9 +7234,7 @@
72357235 this<br />
72367236 is<br />
72377237 a<br />
7238 -test<br />
7239 -
7240 -</p>
 7238+test</p>
72417239 </div>
72427240
72437241 !!end
@@ -7256,9 +7254,7 @@
72577255 this<br />
72587256 '''is'''<br />
72597257 a<br />
7260 -test<br />
7261 -
7262 -</p>
 7258+test</p>
72637259 </div>
72647260
72657261 !! end
Index: trunk/phase3/includes/parser/Parser.php
@@ -4190,23 +4190,20 @@
41914191 */
41924192 $nl = array_key_exists( 'compact', $param ) ? '' : "\n";
41934193
4194 - $tag = $this->insertStripItem( "<br />", $this->mStripState );
 4194+ $replacer = new DoubleReplacer( ' ', '&nbsp;' );
 4195+ $text = $this->recursiveTagParse( $in );
 4196+ $text = $this->mStripState->unstripNoWiki( $text );
41954197 // Only strip the very first and very last \n (which trim cannot do)
4196 - $text = $in;
4197 - if( substr( $in, 0, 1 ) == "\n" )
4198 - $text = substr( $in, 1 );
 4198+ if( substr( $text, 0, 1 ) == "\n" )
 4199+ $text = substr( $text, 1 );
41994200 if( substr( $text, -1 ) == "\n" )
42004201 $text = substr( $text, 0, -1 );
42014202
4202 - $text = str_replace( "\n", "$tag\n", $text );
 4203+ $text = str_replace( "\n", "<br />\n", $text );
42034204 $text = preg_replace_callback(
42044205 "/^( +)/m",
4205 - create_function(
4206 - '$matches',
4207 - 'return str_replace(" ", "&nbsp;", "$matches[0]");'
4208 - ),
 4206+ $replacer->cb(),
42094207 $text );
4210 - $text = $this->recursiveTagParse( $text );
42114208
42124209 // Pass HTML attributes through to the output.
42134210 $attribs = Sanitizer::validateTagAttributes( $param, 'div' );
@@ -4218,7 +4215,7 @@
42194216 $attribs['class'] = 'poem';
42204217 }
42214218
4222 - return XML::openElement( 'div', $attribs ) . $nl . trim( $text ) . $nl . XML::closeElement( 'div' );
 4219+ return Xml::openElement( 'div', $attribs ) . $nl . trim( $text ) . $nl . Xml::closeElement( 'div' );
42234220 }
42244221
42254222 function getImageParams( $handler ) {

Follow-up revisions

RevisionCommit summaryAuthorDate
r42005Revert mystery change r41983 -- no explanation of the change or its purpose....brion23:14, 12 October 2008
r42012* re-implement r41983 (forgot to add log message). Changes made:...skizzerz02:18, 13 October 2008

Comments

#Comment by Voice of All (talk | contribs)   17:47, 12 October 2008

Summaries please? What is this?

#Comment by Brion VIBBER (talk | contribs)   23:15, 12 October 2008

Reverted on general principle in r42005.

#Comment by Skizzerz (talk | contribs)   02:09, 13 October 2008

Sorry 'bout that. Anyway, the full log file is linked after this sentence, but I've attached just the necessary pieces here as well: http://toolserver.org/~amidaniel/chanlogs/%23mediawiki/20081012.txt

[01:21:07] <Skizzerz> TimStarling: I tried contacting you earlier regarding the Poem issue, but you were afk so I just went ahead and did a few things. [01:21:07] <TimStarling> in this particular case there's a thing called DoubleReplacer which does exactly what that /e is doing [01:21:21] <TimStarling> it's in StringUtils [01:26:02] <Skizzerz> TimStarling: so are these changes acceptible? I tried to implement as much as you said, although I left a few comments on the original commit's discussion regarding some of the things that could not be changed without breaking stuff: http://www.mediawiki.org/wiki/Special:Code/MediaWiki/41978 [01:28:02] <TimStarling> ouch, no not create_function either [01:28:09] <TimStarling> here's how you do it, I only gave you a hint before [01:28:23] <TimStarling> $replacer = new DoubleReplacer( ' ', ' ' ); [01:28:27] <Skizzerz> ah [01:28:35] <Skizzerz> I looked through StringUtils but I must have missed that [01:28:55] <TimStarling> $text = preg_replace_callback( '/^( +)/m', $replacer->cb() ); [01:29:15] <TimStarling> well, without the brackets, they're redundant [01:30:08] <TimStarling> why do you only want to remove the first line break, not all the leading line breaks? [01:30:29] <TimStarling> you think that's what was intended? [01:30:32] <Skizzerz> yes [01:30:41] <Skizzerz> when I used trim() it failed one of the parser tests for it [01:30:54] <Skizzerz> when an empty line was inserted before and after a line [01:31:24] <Skizzerz> granted... I don't see the usefulness for that, but I wanted to try to not break sites using the existing extension that switch over to this

[01:32:21] <Skizzerz> with only replacing the first and last \n,

\n\ntest\n\n

would render as


test

, but with trim() it would render as

test

[01:33:43] <TimStarling> it looks good otherwise [01:34:11] <Skizzerz> so do you think we should keep that as-is or just use trim()? [01:35:30] <TimStarling> as is [01:35:36] <Skizzerz> ok [01:35:37] *Skizzerz tests [01:35:39] <TimStarling> did you confirm that insertStripItem is required? [01:36:03] <TimStarling> ah yes, there's a comment [01:36:32] <TimStarling> heh, interesting comment [01:36:33] <Skizzerz> yeah... <nowiki> inside the tag didn't work as expected without it [01:37:21] <Skizzerz> wait... maybe instead of
I replace it with \n\n? [01:37:36] <Skizzerz> Then the parser should still parse the newline and a strip item wouldn't be needed

[01:37:50] <Skizzerz> of course... would that add a
or a new

? =\ [01:38:16] <TimStarling> why does it call recursiveTagParse after the newline cleanup? [01:38:40] <TimStarling> instead of before? [01:39:09] <TimStarling> I guess some parser stages might slightly corrupt newlines... [01:39:12] <TimStarling> hmm [01:39:18] <TimStarling> no, if they did that, DBL wouldn't work [01:39:55] <Skizzerz> lemme test [01:40:03] <TimStarling> if you did it after recursiveTagParse, you'd be positioning it in an equivalent location to doBlockLevels [01:40:39] <TimStarling> maybe the problem is the lack of an unstrip? [01:41:03] <TimStarling> you'll need recursiveTagParse and then unstrip() [01:41:11] <Skizzerz> it unstrips fine [01:41:29] <TimStarling> I'd better get the latest version so I can test it too [01:41:59] <Skizzerz> well, if we put the recursiveTagParse before the replacements, I don't think having strip items would even be necessary [01:42:16] <TimStarling> that's what I was thinking [01:44:20] <TimStarling> yeah, works for me [01:44:59] <TimStarling> and like I say, it matches the parse order of the rest of the parser more accurately [01:47:03] <Skizzerz> hmm [01:47:19] <Skizzerz> moving the recursiveTagParse up makes wiki markup not work for me [01:48:04] <TimStarling> like links? [01:48:11] <Skizzerz> everything [01:48:23] <Skizzerz> wait, bullets work [01:48:30] <Skizzerz> but bold, etc. do not [01:48:45] <TimStarling> check $in vs $text [01:48:59] <TimStarling> works for me but you've got to watch that [01:49:47] <TimStarling> http://p.defau.lt/?pRDXYT3IwGTkp_kqYYB__g [01:51:21] <Skizzerz> yeah, I have that too, but doesn't work for me... [01:51:39] <Skizzerz> wait, there we go [01:51:47] <Skizzerz> my browser had the page cached =/ [01:51:57] <TimStarling> I always use preview when I'm testing the parser [01:52:21] <Skizzerz> same here... but somehow that didn't work [01:52:31] <Skizzerz> but clearing my cache did [01:53:39] <TimStarling> obviously $tag can be removed now, replaced with a string literal, if you haven't done that already [01:53:50] <Skizzerz> I did [01:53:56] <TimStarling> and note that it's Xml::openElement not XML::openElement [01:54:23] <TimStarling> as if it were a word [01:54:24] <Skizzerz> oh [01:54:34] <Skizzerz> hmm, <nowiki> doesn't work again [01:55:05] <TimStarling> needs unstrip [01:55:32] <TimStarling> yeah, seems to work with unstrip [01:55:36] <TimStarling> I'd better try with gallery [01:56:27] <TimStarling> mmm, some extra
s, but probably valid [01:56:52] <TimStarling> $text = $this->mStripState->unstripBoth( $text ); [01:57:00] <TimStarling> after recursiveTagParse [01:57:37] <TimStarling> otherwise the nowiki section is just a marker, so it can't replace line breaks inside it [01:58:07] <Skizzerz> ah [01:58:26] <TimStarling> it'll go back into a marker when renderPoem() returns, so there's no conflict of formats [01:59:30] <Skizzerz> so is that it then? [01:59:39] <TimStarling> maybe you should only unstrip <nowiki> [01:59:43] <TimStarling> not the other strip markers [01:59:46] <TimStarling> what do you think? [01:59:55] <TimStarling> probably <poem> isn't really meant to affect <gallery> [02:00:01] <Skizzerz> probably not [02:00:26] <TimStarling> $text = $this->mStripState->unstripNoWiki( $text ); [02:02:28] <Skizzerz> ok, that all looks good then [02:02:58] <Skizzerz> it even removed the unnecessary linebreak from the end [02:04:45] <TimStarling> ok, commit it then [02:07:37] <Skizzerz> ... wtf [02:07:40] *Skizzerz shoots winblows [02:18:07] <Skizzerz> TimStarling: can you commit, Notepad++ is being gay and adding in the wrong line endings, and I can't figure out why [02:19:53] <Skizzerz> wait, think I got it now =/ [02:20:21] <CIA-6> �03skizzerz� * r�41983� �10�/trunk/phase3/ (includes/parser/Parser.php maintenance/parserTests.txt)�:�[02:20:37] <Skizzerz> er... must've forgot the log msg >_>

#Comment by Brion VIBBER (talk | contribs)   02:20, 13 October 2008

Restored with commit message in r42012, so now we know. :D

Status & tagging log