r88997 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r88996‎ | r88997 | r88998 >
Date:20:55, 27 May 2011
Author:mah
Status:reverted (Comments)
Tags:
Comment:
fix for Bug #93 “tilde signatures inside nowiki tags sometimes get expanded (<includeonly><nowiki>~~~~</nowiki></includeonly>)”
Patch from Brad Jorsch

Fix for includeonly case. Hopefully fixing this ancient bug doesn't
‘cause people to cry
Modified paths:
  • /trunk/phase3/includes/parser/Preprocessor_DOM.php (modified) (history)
  • /trunk/phase3/includes/parser/Preprocessor_Hash.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/parser/Preprocessor_Hash.php
——@@ -144,6 +144,9 @@
145145 if ( strpos( $text, '<onlyinclude>' ) !== false && strpos( $text, '</onlyinclude>' ) !== false ) {
146146 $enableOnlyinclude = true;
147147 }
 148+ } else if ( $this->parser->ot['wiki'] ) {
 149+ $ignoredTags = array( 'noinclude', '/noinclude', 'onlyinclude', '/onlyinclude', 'includeonly', '/includeonly' );
 150+ $ignoredElements = array();
148151 } else {
149152 $ignoredTags = array( 'noinclude', '/noinclude', 'onlyinclude', '/onlyinclude' );
150153 $ignoredElements = array( 'includeonly' );
Index: trunk/phase3/includes/parser/Preprocessor_DOM.php
——@@ -185,6 +185,9 @@
186186 if ( strpos( $text, '<onlyinclude>' ) !== false && strpos( $text, '</onlyinclude>' ) !== false ) {
187187 $enableOnlyinclude = true;
188188 }
 189+ } else if ( $this->parser->ot['wiki'] ) {
 190+ $ignoredTags = array( 'noinclude', '/noinclude', 'onlyinclude', '/onlyinclude', 'includeonly', '/includeonly' );
 191+ $ignoredElements = array();
189192 } else {
190193 $ignoredTags = array( 'noinclude', '/noinclude', 'onlyinclude', '/onlyinclude' );
191194 $ignoredElements = array( 'includeonly' );

Follow-up revisions

RevisionCommit summaryAuthorDate
r89000Fix failing preprocessor tests by checking if ot is set.mah21:43, 27 May 2011
r89013follow up r88997 with a rough attempt at a parser test, though, really, we ca...mah02:09, 28 May 2011
r89308Revert r88997: fix for bug 93 caused some additional problems....brion00:32, 2 June 2011
r89648Another try at fixing bug 93 "tilde signatures inside nowiki tags sometimes g...pcopp15:12, 7 June 2011
r89662Provisional revert of r89648: "Another try at fixing bug 93 "tilde signatures...brion17:34, 7 June 2011

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r5295BUG#93 Fix handling of <nowiki> in templatesjeluf23:14, 18 September 2004
r5296BUG#93 Fix handling of <nowiki> in templates. From HEADjeluf23:17, 18 September 2004
r13917(bug 93) <nowiki> tags and tildes in templatesrobchurch02:20, 29 April 2006

Comments

#Comment by 😂 (talk | contribs)   20:59, 27 May 2011

Definitely needs parser tests.

#Comment by Brion VIBBER (talk | contribs)   21:13, 27 May 2011

http://ci.tesla.usability.wikimedia.org/cruisecontrol/buildresults/mw?tab=testResults says breakage! 

Error:

PreprocessorTest::testPreprocessorOutput with data set #0 ('Foo', '<root>Foo</root>')
Undefined property: PreprocessorTest::$ot

/home/ci/cruisecontrol-bin-2.8.3/projects/mw/source/includes/parser/Preprocessor_DOM.php:188
/home/ci/cruisecontrol-bin-2.8.3/projects/mw/source/tests/phpunit/includes/parser/PreprocessorTest.php:112
/home/ci/cruisecontrol-bin-2.8.3/projects/mw/source/tests/phpunit/MediaWikiTestCase.php:63
/home/ci/cruisecontrol-bin-2.8.3/projects/mw/source/tests/phpunit/MediaWikiPHPUnitCommand.php:20
/home/ci/cruisecontrol-bin-2.8.3/projects/mw/source/tests/phpunit/phpunit.php:60
#Comment by P.Copp (talk | contribs)   21:51, 29 May 2011

This breaks substed templates inside includeonly: 

<includeonly>{{subst:Foo}}</includeonly>

If you try to put this in a page, Template:Foo is substituted into it, which is not what is desired. (includeonly means: only render this when included from somewhere)

Will also break the preprocessor xml cache unless you put $this->parser->ot['wiki'] in the cache key.

#Comment by MarkAHershberger (talk | contribs)   22:03, 29 May 2011

Wow! thanks so much for spotting this, especially the xml cache bit. Where is the cache key for the xml cache defined?

#Comment by P.Copp (talk | contribs)   22:11, 29 May 2011

It's in the same file, function preprocessToObj(). However, I don't think we can keep this rev anyway, as it will break lots of templates on wmf wikis. Putting a substed template in includeonly tags is a pretty common use case.

#Comment by MarkAHershberger (talk | contribs)   22:50, 29 May 2011

I was afraid of that. So, should the bug be WONTFIXED? or should we find another way to fix it?

#Comment by 😂 (talk | contribs)   22:52, 29 May 2011

Newparser?

#Comment by Brion VIBBER (talk | contribs)   00:33, 2 June 2011

I've reverted for now in r89308. The test case (new version from r89191) is still in parser tests but is disabled for now so it doesn't trigger. (The test case also didn't work with the fix in though...)

Status & tagging log