Last modified: 2009-05-03 22:38:20 UTC
Created attachment 6068 [details] Patch to Pager.php, against r49962 I'm currently plugging away at formatting [[Special:Allmessages]] using the TablePager class, from Pager.php. In order to avoid some horrible hacks (returning a value of """foo" rowspan="2""" to fill into something like """<td class="$class">""") I made a few changes to TablePager itself, which I think can probably stand on their own, so I'm submitting them separately here. With this patch in place, the code in /extensions/CodeReview/CodeRevisionListView.php marked with evils (line273-295) can also be refactored; it shouldn't be necessary to overload the formatRow() function at all. Changes are quite simple: setting $mCurrentRow right at the start of the function, so its context is available for the attribute-getting functions (this is the issue that forced CodeReview to clone the function), replacing the over-specific getRowClass() function with a more general getRowAttrs(), and implementing a parallel getCellAttrs(). getRowClass() is still retained for B/C, naturally. I think it's fully backwards-compatible, and it certainly works on my test install, although I haven't tested it with CodeReview itself. Bitsize chunks :D
(In reply to comment #0) > Created an attachment (id=6068) [details] > Patch to Pager.php, against r49962 > > I'm currently plugging away at formatting [[Special:Allmessages]] using the > TablePager class, from Pager.php. In order to avoid some horrible hacks > (returning a value of """foo" rowspan="2""" to fill into something like """<td > class="$class">""") I'm surprised that that even works: it's a sanitization bug.
You mean the hack? TBH I didn't actually try it, I knew what I needed and what was available in TablePager, and gave up in disgust. Whoever wrote that comment in CodeReview was right, the function wasn't particularly accessible. The positioning of the $this->mCurrentRow=$row; line is just stupid. Or do you mean the patch? :-S
(In reply to comment #2) > You mean the hack? TBH I didn't actually try it, I knew what I needed and what > was available in TablePager, and gave up in disgust. Whoever wrote that comment > in CodeReview was right, the function wasn't particularly accessible. The > positioning of the $this->mCurrentRow=$row; line is just stupid. > > Or do you mean the patch? :-S > No, I meant the HTML injection hack. I hadn't even looked at the patch yet. Now that I have: $td = Xml::openElement( 'td', $this->getCellAttrs($field,$value) ); $s .= "$td $formatted </td>"; You should simply use: $s .= Xml::element( 'td', $this->getCellAttrs( $field, $value ), $formatted ); (or Xml::tags() if $formatted contains HTML and should not be escaped; I couldn't tell from the patch). return array( 'class' => htmlspecialchars( $this->getRowClass( $row ) ) ); You don't need to (and shouldn't) use htmlspecialchars() here: the Xml:: functions will handle proper escaping. This way, you'll probably end up double-escaping stuff. The same applies to getCellAttrs().
I tried the first method and got PHP's version of a compile error (unexpected character on the last line); I'm not sure either why it didn't work ($formatted could in principle contain raw HTML, but would that really *kill* the program?); but doing it that way definitely did fix it.
Created attachment 6069 [details] Updated with Roan's htmlspecialchars() fixes
(In reply to comment #4) > I tried the first method and got PHP's version of a compile error (unexpected > character on the last line); I'm not sure either why it didn't work My fragment looks correct, I'm not sure your error is related. Please try again, as your current patch still doesn't use Xml::element() or Xml::tags() on that line. > ($formatted > could in principle contain raw HTML, but would that really *kill* the > program?); but doing it that way definitely did fix it. > No, but it would show the HTML as text in the browser rather than actually treating it as HTML, so you'd have a bug in case $formatted is *supposed* to contain HTML.
Created attachment 6070 [details] updated, use Xml::tags(); still against r49962 I guess you're right; tried and it worked fine. It definitely needs to be tags(); using elements escapes things like links. htmlspecialchars is the thing to use to disable tags, right? (Not relevant for this, but will be in my rewrite of SpecialAllmessages).
(In reply to comment #7) > Created an attachment (id=6070) [details] > updated, use Xml::tags(); still against r49962 > > I guess you're right; tried and it worked fine. It definitely needs to be > tags(); using elements escapes things like links. Patch looks OK, committed in r50046 > htmlspecialchars is the thing > to use to disable tags, right? (Not relevant for this, but will be in my > rewrite of SpecialAllmessages). > Yes, but you probably want to use Xml:: functions there as well, as they sanitize stuff for you.
