MediaWiki r113297 - Code Review

Jump to: navigation, search
Repository:MediaWiki
Revision:r113296‎ | r113297 (on ViewVC)‎ | r113298 >
Date:21:06, 7 March 2012
Author:wikinaut
Status:reverted (Comments)
Tags:gerritmigration 
Comment:
fix for bug34763 'RSS feed items (HTML) are not rendered as HTML but htmlescaped'; tolerated controlled regression bug30377 'feed item length limitation', because this now becomes very tricky when we allow some tags in order to close bug 34763.
Modified paths:

Diff [purge]

Loading diff…

Follow-up revisions

Rev.Commit summaryAuthorDate
r114390Revert r111347, r111348, r111350, r111351, r111515, r111816, r112243, r112251......catrope18:40, 21 March 2012

Past revisions this follows-up on

Rev.Commit summaryAuthorDate
r111350fix for bug30377 : add a new parameter to limit the number of characters when...wikinaut07:23, 13 February 2012
r112709function name typo correction. Version number updatewikinaut20:00, 29 February 2012

Comments

#Comment by Bawolff (talk | contribs)   02:53, 8 March 2012

$ret = Sanitizer::removeHTMLtags( $text, null, array(), $extra, array( "iframe" ) );

Why is iframe specifically mentioned? If there's a good reason, should have a comment.


+ $renderedFeed = $this->sandboxParse( $renderedFeed );

Why does this use its own instance of the parser?

In regards to initializing own instance of parser, initializing the parser using $wgUser 's options, and $wgTitle doesn't seem like the greatest idea. $parser->getTitle() would probably be better than $wgTitle, and for $wgUser, sometimes the parser needs to parse things using a user's options other than the current user (I think it does this to get a canonical version of the page for links tables, but don't quote me on that). You can get the current parser options using $parser->getOptions(). However as I said above, it doesn't seem like the sandboxParse is actually necessary in your code.

#Comment by Wikinaut (talk | contribs)   06:51, 8 March 2012

ad i) iframe: I saw in a E:RSS rendered feed http://rss.slashdot.org/Slashdot/slashdot one or several iframe tags which came through htmlescaped. I will make a deeper analysis of this specific case of iframes. I just wanted to be sure that iframes are expressly escaped, however we can further discuss this depending on my analysis.

ad ii) this was also discussed with Brion, and not working in the first test: not working without a (local sandbox) parsing in the local context as described here https://www.mediawiki.org/wiki/Manual:Special_pages#workaround_.231 from where it is verbatim copied. Your comment means that the text on https://www.mediawiki.org/wiki/Manual:Special_pages#workaround_.231 should be further corrected (Brion already corrected https://www.mediawiki.org/w/index.php?title=Manual%3ASpecial_pages&diff=505706&oldid=490966 "fix for calling private func; use the public one").

Summary: I will checked and make further analysis of the two points you had. However, the current code is working as intended, can perhaps be improved. I invite everyone - especially parser experts - to help me here, because it was a long and windy road already.

Does this sufficiently answer your questions ?

#Comment by Wikinaut (talk | contribs)   07:51, 8 March 2012

RE: + $renderedFeed = $this->sandboxParse( $renderedFeed );

I double-checked it. result: the sandbox-parsign is needed. If I use recursiveTagParse the wiki page is not correctly rendered and elements from the content window break out and appear outside, navigation bar broken etc. So what has been written on https://www.mediawiki.org/wiki/Manual:Special_pages#workaround_.231 applies to the present issue.

Status & tagging log

  • 18:40, 21 March 2012 Catrope (talk | contribs) changed the status of r113297 [removed: new added: reverted]
  • 01:03, 21 March 2012 Catrope (talk | contribs) changed the tags for r113297 [added: gerritmigration]