Page MenuHomePhabricator

Message handling in parser
Open, LowPublic

Description

There are some bugs open about strip markers being exposed, see bug T11762, bug T16959, bug T18129, bug T18744 (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

Details

Reference
bz17329

Event Timeline

bzimport raised the priority of this task from to Low.Nov 21 2014, 10:24 PM
bzimport set Reference to bz17329.
bzimport added a subscriber: Unknown Object (MLST).

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:

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.

  • Bug 21855 has been marked as a duplicate of this bug. ***

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

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.

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.

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).

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

Paul / Tim Starling:

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

Paul / Tim Starling:

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

(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.