Last modified: 2014-09-24 00:01:04 UTC

Wikimedia Bugzilla is closed!

Wikimedia migrated from Bugzilla to Phabricator. Bug reports are handled in Wikimedia Phabricator.
This static website is read-only and for historical purposes. It is not possible to log in and except for displaying bug reports and their history, links might be broken. See T19329, the corresponding Phabricator task for complete and up-to-date bug report information.
Bug 17329 - Message handling in parser
Message handling in parser
Status: NEW
Product: MediaWiki
Classification: Unclassified
Parser (Other open bugs)
All All
: Low normal with 3 votes (vote)
: ---
Assigned To: Nobody - You can work on this!
: newparser, patch, patch-reviewed
: 21855 (view as bug list)
Depends on:
Blocks: 9762 14959 16129 16744
  Show dependency treegraph
Reported: 2009-02-02 23:52 UTC by P.Copp
Modified: 2014-09-24 00:01 UTC (History)
8 users (show)

See Also:
Web browser: ---
Mobile Platform: ---
Assignee Huggle Beta Tester: ---

Proposed patch: Use $wgMessageCache->mParser in $wgOut->parse (5.07 KB, patch)
2009-02-02 23:53 UTC, P.Copp

Description P.Copp 2009-02-02 23:52:02 UTC
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.
Comment 1 P.Copp 2009-02-02 23:53:40 UTC
Created attachment 5772 [details]
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.
Comment 2 Tim Starling 2009-04-30 04:02:35 UTC
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.
Comment 3 Alexandre Emsenhuber [IAlex] 2009-12-15 18:54:29 UTC
*** Bug 21855 has been marked as a duplicate of this bug. ***
Comment 4 Vitaliy Filippov 2009-12-15 23:17:37 UTC
Hi! Why not simply pass clearState=false to wgParser->parse()? Strip state markers are "globally unique" so what's wrong with this approach?
Comment 5 Platonides 2010-10-13 22:10:34 UTC
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.
Comment 6 Sumana Harihareswara 2011-11-06 14:51:27 UTC
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 to learn about the parser rewrite and suggest this kind of fix.
Comment 7 Platonides 2011-11-06 22:00:43 UTC
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).
Comment 8 Tim Landscheidt 2013-01-05 04:32:14 UTC
What's the state of this?  Has the functionality of the patch been included in core, or is this a WONTFIX?
Comment 9 Andre Klapper 2014-04-08 12:56:43 UTC
Paul / Tim Starling:

> What's the state of this?  Has the functionality of the patch been included
> in core, or is this a WONTFIX?
Comment 10 Andre Klapper 2014-07-03 21:58:15 UTC
Paul / Tim Starling:

> What's the state of this?  Has the functionality of the patch been included
> in core, or is this a WONTFIX?
Comment 11 wikimedia-gaijin42 2014-07-24 20:13:33 UTC
This bug appears to still be happening quite regularly on en.wikipedia
Comment 12 James Forrester 2014-07-24 20:20:59 UTC
(In reply to wikimedia-gaijin42 from comment #11)
> This bug appears to still be happening quite regularly on en.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.

Note You need to log in before you can comment on or make changes to this bug.