Last modified: 2009-05-03 22:38:20 UTC

Wikimedia Bugzilla is closed!

Wikimedia has migrated from Bugzilla to Phabricator. Bug reports should be created and updated in Wikimedia Phabricator instead. Please create an account in Phabricator and add your Bugzilla email address to it.
Wikimedia Bugzilla is read-only. If you try to edit or create any bug report in Bugzilla you will be shown an intentional error message.
In order to access the Phabricator task corresponding to a Bugzilla report, just remove "static-" from its URL.
You could still run searches in Bugzilla or access your list of votes but bug reports will obviously not be up-to-date in Bugzilla.
Bug 18620 - Improve TablePager (patch)
Improve TablePager (patch)
Status: RESOLVED FIXED
Product: MediaWiki
Classification: Unclassified
General/Unknown (Other open bugs)
1.15.x
All All
: Normal enhancement (vote)
: ---
Assigned To: Nobody - You can work on this!
: patch, patch-reviewed
Depends on:
Blocks: 16497
  Show dependency treegraph
 
Reported: 2009-04-28 22:47 UTC by Happy-melon
Modified: 2009-05-03 22:38 UTC (History)
1 user (show)

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


Attachments
Patch to Pager.php, against r49962 (2.08 KB, patch)
2009-04-28 22:47 UTC, Happy-melon
Details
Updated with Roan's htmlspecialchars() fixes (2.04 KB, patch)
2009-04-29 14:46 UTC, Happy-melon
Details
updated, use Xml::tags(); still against r49962 (2.02 KB, patch)
2009-04-29 15:37 UTC, Happy-melon
Details

Description Happy-melon 2009-04-28 22:47:32 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
Comment 1 Roan Kattouw 2009-04-29 10:16:49 UTC
(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.
Comment 2 Happy-melon 2009-04-29 12:47:35 UTC
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
Comment 3 Roan Kattouw 2009-04-29 12:58:33 UTC
(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().



Comment 4 Happy-melon 2009-04-29 14:45:16 UTC
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. 
Comment 5 Happy-melon 2009-04-29 14:46:07 UTC
Created attachment 6069 [details]
Updated with Roan's htmlspecialchars() fixes
Comment 6 Roan Kattouw 2009-04-29 15:00:49 UTC
(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.
Comment 7 Happy-melon 2009-04-29 15:37:54 UTC
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).
Comment 8 Roan Kattouw 2009-04-29 16:00:12 UTC
(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.

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


Navigation
Links