r90858 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r90857‎ | r90858 | r90859 >
Date:22:58, 26 June 2011
Author:robin
Status:resolved (Comments)
Tags:parser 
Comment:
Make parser->getFunctionLang be dependent on title->getPageLanguage() instead of $wgContLang, i.e. the page content language instead of the wiki content language. This sets the right language on page view + edit for all pages, instead of only edit preview on MediaWiki namespace pages (as in EditPage.php).
Modified paths:
  • /trunk/phase3/includes/EditPage.php (modified) (history)
  • /trunk/phase3/includes/parser/Parser.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/parser/Parser.php
——@@ -673,7 +673,7 @@
674674 if ( $target !== null ) {
675675 return $target;
676676 } else {
677 - return $this->mOptions->getInterfaceMessage() ? $wgLang : $wgContLang;
 677+ return $this->mOptions->getInterfaceMessage() ? $wgLang : $this->mTitle->getPageLanguage();
678678 }
679679 }
680680
Index: trunk/phase3/includes/EditPage.php
——@@ -2071,11 +2071,6 @@
20722072
20732073 wfRunHooks( 'EditPageGetPreviewText', array( $this, &$toparse ) );
20742074
2075 - // In which language to parse the page
2076 - // (Should this still be only for MediaWiki pages, or for all pages?)
2077 - if ( $this->mTitle->getNamespace() == NS_MEDIAWIKI ) {
2078 - $parserOptions->setTargetLanguage( $this->mTitle->getPageLanguage() );
2079 - }
20802075 $parserOptions->setTidy( true );
20812076 $parserOptions->enableLimitReport();
20822077 $parserOutput = $wgParser->parse( $this->mArticle->preSaveTransform( $toparse ),

Follow-up revisions

RevisionCommit summaryAuthorDate
r96405Per r90858 CR, throw MW exception on missing title contextrobin04:05, 7 September 2011

Comments

#Comment by Platonides (talk | contribs)   22:59, 27 June 2011

I'm not convinced about this, since that requires a much bigger knowledge at the parser side. Setting the getPageLanguage() to the ParserOptions seems a better alternative IMHO.

#Comment by SPQRobin (talk | contribs)   23:45, 27 June 2011

It's a temporary fix/improvement, because as I said in an earlier commit, there should probably be a better way but I don't know enough about MediaWiki core (and certainly not the parser) to find the best way. See also bugzilla:6100#c49.

Move the code from getPageLanguage to getFunctionLang? Move getPageLanguage to OutputPage? Something else?

It should at least be easy to define the page content language in extensions.

Also, what do you mean with "that requires a much bigger knowledge at the parser side"?

#Comment by Platonides (talk | contribs)   21:56, 28 June 2011

It makes the parser integrate more tightly with the titles.

#Comment by Brion VIBBER (talk | contribs)   17:14, 4 August 2011

I'm actually kind of inclined to do it robin's way; the content direction is a property of the page, and having to explicitly set it _separately_ from other page metadata like the page title feels a bit odd. Every bit of code that calls the parser would have to know to check the page's directionality and pass it in if we have to add it to ParserOptions, which feels worse to me.

Alternatively, ParserOptions could be initialized from both a user and a title, but a) that still requires changing all the existing code and b) I'm not sure that fits with the theory of ParserOptions. Since page metadata like title and revision ID aren't passed through ParserOptions either, I kinda feel like it should come along with the title.

In the next-gen parser interface, we may want to have separate notions of 'general environment info' and 'this-page info', where this-page includes the title, revision id, and any other such metadata that's related to what you're parsing rather than who is parsing it.

#Comment by 😂 (talk | contribs)   20:45, 16 August 2011

This will cause breakages when the Parser doesn't have a title context. r94680 exposed it.

#Comment by Brion VIBBER (talk | contribs)   20:57, 16 August 2011

Isn't the parser supposed to always have a title context? Pretty sure I've seen other things break in this circumstance and it seems like it violates basic assumptions.

#Comment by 😂 (talk | contribs)   21:09, 16 August 2011

You'd think so, but it might not. In r94680, we exploded on the Magic Words tests.

#Comment by Bryan (talk | contribs)   21:59, 19 August 2011

It's not necessary a bad thing to explode on missing context.

#Comment by 😂 (talk | contribs)   22:12, 19 August 2011

Exploding sanely, sure. But an E_FATAL is never nice :)

#Comment by SPQRobin (talk | contribs)   16:21, 31 August 2011

Does this still need to be marked fixme?

I was thinking to perhaps check if we have a $this->mTitle object, and if not, either give an error or fallback to $wgContLang however that would give different output for e.g. magic words.

#Comment by 😂 (talk | contribs)   00:39, 7 September 2011

You could throw an exception. Like Brion said, it probably makes the most sense to die in scenarios where we don't have a title. An exception avoids the E_FATAL and provides a nice usable stack trace.

#Comment by SPQRobin (talk | contribs)   04:06, 7 September 2011

Done in r96405

Status & tagging log