Message handling in parser
OpenPublic

Description

Author: paul.copperman

Description:
There are some bugs open about strip markers being exposed, see bug 9762, bug 14959, bug 16129, bug 16744 (perhaps more?)
The issue seems to be always the same: Some tag extension or parser function innocently calling wfMsgExt(..'parse'), wfMsgExt(..'parseinline') or in case of transcluded special pages also $wgOut->addWikiText().
From within a parse, these calls lead to recursive execution of $wgParser->parse() and thus clearing strip state.

Some possible solutions I can think of:

  • Fix all extensions and special pages that call these functions by replacing them with the appropriate non-parsing version of wfMsgWhatever AND add a big warning to the appropriate places to keep extension authors from using the former.
  • Use another parser for message parsing. Luckily, this is already done within MessageCache::transform, so we could just reuse this second parser for $wgOut->parse().
  • Add an own message parsing function to the parser

The third is probably the cleanest solution, as it would also fix the dependency of $wgTitle, but a bit hard to implement, as all the tag extensions and parser functions would have to be changed, too.

So, for now, the second solution seems the easiest and most reasonable to me. I'll add a patch for this in a moment, to clarify.


Version: 1.15.x
Severity: normal

bzimport added a subscriber: wikibugs-l.
bzimport set Reference to bz17329.
bzimport created this task.Via LegacyFeb 2 2009, 11:52 PM
bzimport added a comment.Via ConduitFeb 2 2009, 11:53 PM

paul.copperman wrote:

Proposed patch: Use $wgMessageCache->mParser in $wgOut->parse

This patch basically does three things:
*Add ->mParsing to the Parser to keep track, if this parser is currently in use.
*Make MessageCache's parser publicly usable
*Add a check to OutputPage::parse to switch to MessageCache's parser in case that $wgParser is in use.

I've tested it locally and it seems that it fixes the four bugs mentioned above.

Attached: bug17329.patch

tstarling added a comment.Via ConduitApr 30 2009, 4:02 AM

The problem is that $wgParser->parse() is not re-entrant. It could be made to be re-entrant, by splitting the Parser class into a configuration class and a context class. So $wgParser would be the configuration object, and it would create a fresh context object, with a new set of strip markers, at each call to $wgParser->parse(). This would mean that callback hooks would need to get the context object rather than the configuration object, which could cause large-scale b/c breakage if not done carefully.

I think juggling a whole lot of parser object, storing them in unlikely places like $wgMessageCache, and requiring the user to guess which one to use, would just be a recipe for more errors.

IAlex added a comment.Via ConduitDec 15 2009, 6:54 PM
  • Bug 21855 has been marked as a duplicate of this bug. ***
VitaliyFilippov added a comment.Via ConduitDec 15 2009, 11:17 PM

Hi! Why not simply pass clearState=false to wgParser->parse()? Strip state markers are "globally unique" so what's wrong with this approach?

Platonides added a comment.Via ConduitOct 13 2010, 10:10 PM

What about replacing uses of $wgParser with a new method Parser:newParser() ?

Instead of using the global parser and hoping it is available, you just request a new parser (initialised to the defaults) each time.
Then that function could reuse the same parser object if it is safe to do.

bzimport added a comment.Via ConduitNov 6 2011, 2:51 PM

sumanah wrote:

Thank you for your patch, P.Copp. Since Tim reviewed it, I'm changing the "need-review" keyword to "reviewed." I'm also adding the "newparser" keyword since this seems like the kind of thing Gabriel Wicke & Brion Vibber will want to address in their current parser rewrite.

You might also want to subscribe to https://lists.wikimedia.org/mailman/listinfo/wikitext-l to learn about the parser rewrite and suggest this kind of fix.

Platonides added a comment.Via ConduitNov 6 2011, 10:00 PM

I think this was fixed along the way.
MessageCache uses a clone of the parser, and MessageCache::parse is itself not reentrant since r86304, which also mage wfMsgExt() no longer use $wgOut->parse() (bug 28532).

scfc added a comment.Via ConduitJan 5 2013, 4:32 AM

What's the state of this? Has the functionality of the patch been included in core, or is this a WONTFIX?

Aklapper added a comment.Via ConduitApr 8 2014, 12:56 PM

Paul / Tim Starling:

What's the state of this? Has the functionality of the patch been included
in core, or is this a WONTFIX?

Aklapper added a comment.Via ConduitJul 3 2014, 9:58 PM

Paul / Tim Starling:

What's the state of this? Has the functionality of the patch been included
in core, or is this a WONTFIX?

bzimport added a comment.Via ConduitJul 24 2014, 8:13 PM

wikimedia-gaijin42 wrote:

This bug appears to still be happening quite regularly on en.wikipedia

https://en.wikipedia.org/wiki/Wikipedia:Village_pump_(technical)#some_sort_of_mediawiki_error_is_happening

Jdforrester-WMF added a comment.Via ConduitJul 24 2014, 8:20 PM

(In reply to wikimedia-gaijin42 from comment #11)

This bug appears to still be happening quite regularly on en.wikipedia

https://en.wikipedia.org/wiki/Wikipedia:
Village_pump_(technical)#some_sort_of_mediawiki_error_is_happening

That was a new issue caused by a problem with the Cite extension that we reverted and shouldn't happen again.

Add Comment

Column Prototype
This is a very early prototype of a persistent column. It is not expected to work yet, and leaving it open will activate other new features which will break things. Press "\" (backslash) on your keyboard to close it now.