r95382 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r95381‎ | r95382 | r95383 >
Date:06:56, 24 August 2011
Author:bawolff
Status:deferred (Comments)
Tags:
Comment:
Commit new extension - Allows making the default sortkey part of the subpage instead of the full page name

Its meant as a way towards fixing bug 22911
Modified paths:
  • /trunk/extensions/SubpageSortkey (added) (history)
  • /trunk/extensions/SubpageSortkey/SubpageSortkey.i18n.php (added) (history)
  • /trunk/extensions/SubpageSortkey/SubpageSortkey.php (added) (history)
  • /trunk/extensions/SubpageSortkey/SubpageSortkey_body.php (added) (history)

Diff [purge]

Index: trunk/extensions/SubpageSortkey/SubpageSortkey_body.php
@@ -0,0 +1,80 @@
 2+<?php
 3+class SubpageSortkey {
 4+ /**
 5+ * The GetDefaultSortkey hook.
 6+ * Basically prefixes the normal sortkey with some of the subpage
 7+ * parts of the current title.
 8+ *
 9+ * Example, if configured with "1,3..5,7" and given the
 10+ * page 0/1/2/3/4/5/6/7/8 it would prefix the sortkey with
 11+ * 1/3/4/5/7. Adding it to the normal sortkey,
 12+ * resulting in "0/1/3/4/5/7\n0/1/2/3/4/5/6/7/8".
 13+ * From there it might be further prefixed with whatever the
 14+ * {{DEFUALTSORT for a page is.
 15+ *
 16+ * Another example: Configuration -3..-1 turns 1/2/3/4/5 -> 3/4
 17+ * and -3.. turns 1/2/3/4/5 -> 3/4/5
 18+ */
 19+ public static function onGetDefaultSortkey( $title, &$unprefixed ) {
 20+ global $wgSubpageSortkeyDefault,
 21+ $wgSubpageSortkeyByNamespace,
 22+ $wgSubpageSortkeyIfNoSubpageUseFullName;
 23+
 24+ $newSortkey = array();
 25+
 26+ $ns = $title->getNamespace();
 27+ if ( !MWNamespace::hasSubpages( $ns ) ) {
 28+ // Do nothing
 29+ return true;
 30+ }
 31+
 32+ if ( isset( $wgSubpageSortkeyByNamespace[$ns] ) ) {
 33+ $descript = $wgSubpageSortkeyByNamespace[$ns];
 34+ } else {
 35+ $descript = $wgSubpageSortkeyDefault;
 36+ }
 37+
 38+ $elms = explode( ',', $descript );
 39+ foreach( $elms as $item ) {
 40+ $ranges = explode( '..', $item, 2 );
 41+ $start = intval( $ranges[0] );
 42+ if ( count( $ranges ) === 1 ) {
 43+ $count = 1;
 44+ } elseif ( $ranges[1] === '' ) {
 45+ $count = false;
 46+ } else {
 47+ $count = intval( $ranges[1] );
 48+ }
 49+ $newSortkey = array_merge( $newSortkey,
 50+ self::getSubpage( $start, $count, $title ) );
 51+ }
 52+ $newPrefix = implode( '/', $newSortkey );
 53+
 54+ // Don't prefix an extra \n if the prefix is empty.
 55+ if ( $newPrefix !== ''
 56+ || $wgSubpageSortkeyIfNoSubpageUseFullName
 57+ ) {
 58+ $unprefixed = $newPrefix . "\n" . $unprefixed;
 59+ }
 60+ return true;
 61+ }
 62+ /**
 63+ * @param $index Int starting index of subpage.
 64+ * @param $count Int how many elements, or false to denote all
 65+ * @param $title Title
 66+ * @return array of subpages (strings)
 67+ */
 68+ private static function getSubpage( $index, $count, $title ) {
 69+ $subpages = explode( '/', $title->getText() );
 70+ $numb = count( $subpages );
 71+
 72+ if ( $index > $numb ) {
 73+ return array();
 74+ }
 75+
 76+ if ( $count === false || $index + $count > $numb + 1 ) {
 77+ $count = $numb + 1 - $index;
 78+ }
 79+ return array_slice( $subpages, $index, $count );
 80+ }
 81+}
Property changes on: trunk/extensions/SubpageSortkey/SubpageSortkey_body.php
___________________________________________________________________
Added: svn:eol-style
182 + native
Index: trunk/extensions/SubpageSortkey/SubpageSortkey.i18n.php
@@ -0,0 +1,10 @@
 2+<?php
 3+$messages = array();
 4+
 5+$messages['en'] = array(
 6+ 'subpagesortkey-desc' => 'Change the default sortkey of a page to be based on its subpage name instead of its full name.',
 7+);
 8+
 9+$messages['qqq'] = array(
 10+ 'subpagesortkey-desc' => '{{desc}}',
 11+);
Property changes on: trunk/extensions/SubpageSortkey/SubpageSortkey.i18n.php
___________________________________________________________________
Added: svn:eol-style
112 + native
Index: trunk/extensions/SubpageSortkey/SubpageSortkey.php
@@ -0,0 +1,60 @@
 2+<?php
 3+/**
 4+ * Really simple extension to change the default sortkey to have something
 5+ * to do with the subpages. See bug 22911. Requires at least MediaWiki 1.18
 6+ *
 7+ * This program is free software; you can redistribute it and/or modify it
 8+ * under the terms of the GNU General Public License as published by the Free
 9+ * Software Foundation; either version 2 of the License, or (at your option)
 10+ * any later version.
 11+ *
 12+ * This program is distributed in the hope that it will be useful, but WITHOUT
 13+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
 14+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
 15+ * more details.
 16+ *
 17+ * You should have received a copy of the GNU General Public License along with
 18+ * this program; if not, write to the Free Software Foundation, Inc., 59 Temple
 19+ * Place - Suite 330, Boston, MA 02111-1307, USA.
 20+ * http://www.gnu.org/copyleft/gpl.html
 21+ *
 22+ * @addtogroup Extensions
 23+ *
 24+ * @author Brian Wolff
 25+ * @copyright Copyright © 2011 Brian Wolff
 26+ * @license http://www.gnu.org/copyleft/gpl.html GNU General Public License 2.0 or later
 27+ */
 28+$wgExtensionCredits['other'][] = array(
 29+ 'path' => __FILE__,
 30+ 'name' => 'Subpage Sortkey',
 31+ 'author' => array( '[http://mediawiki.org/wiki/User:Bawolff Brian Wolff]' ),
 32+ 'descriptionmsg' => 'subpagesortkey-desc',
 33+ 'url' => 'http://www.mediawiki.org/wiki/Extension:SubpageSortkey',
 34+ 'version' => 0.1,
 35+);
 36+
 37+// Syntax is as follows:
 38+// * A positive number n represents the nth subpage section
 39+// * A negative number -i represents the ith subpage section counting backwards
 40+// * A range a..b represents all sections a to b inclusive
 41+// * Ranges can be open.
 42+// Examples:
 43+// "0" would be just the base page name (a/b/c/d -> a).
 44+// "0..2" would be the first 3 sections (a/b/c/d -> a/b/c)
 45+// "0,2" would be a/b/c/d -> a/c
 46+// "1.." would be a/b/c/d -> b/c/d
 47+// "0,-2" would be a/b/c/d -> a/c
 48+
 49+// The default sortkey subpage descriptor (aka "1,2,3..6" )
 50+$wgSubpageSortkeyDefault = false;
 51+// Per namespace sortkeys (array, each key is a namespace number)
 52+// For example $wgSubpageSortkeyByNamespace[NS_TALK] = '1..';
 53+$wgSubpageSortkeyByNamespace = array();
 54+// If the subpage descriptor is empty, use the normal page name?
 55+$wgSubpageSortkeyIfNoSubpageUseFullName = true;
 56+
 57+$wgHooks['GetDefaultSortkey'][] = 'SubpageSortkey::onGetDefaultSortkey';
 58+$dir = dirname( __FILE__ ) . '/';
 59+$wgAutoloadClasses['SubpageSortkey'] = $dir . 'SubpageSortkey_body.php';
 60+$wgExtensionMessagesFiles['SubpageSortkey'] = $dir . 'SubpageSortkey.i18n.php';
 61+
Property changes on: trunk/extensions/SubpageSortkey/SubpageSortkey.php
___________________________________________________________________
Added: svn:eol-style
162 + native

Sign-offs

UserFlagDate
Renklaufinspected20:13, 24 July 2012

Follow-up revisions

RevisionCommit summaryAuthorDate
r95384Add new extension (r95382) to translatewiki.net...raymond07:37, 24 August 2011
r95437(Follow-up r95382) Missed a !bawolff20:01, 24 August 2011

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r91510(bug 29680, bug 22911) Add GetDeaultSortkey hook in order to override default...bawolff00:38, 6 July 2011

Comments

#Comment by Renklauf (talk | contribs)   20:15, 20 July 2012

Hi bawolff,

I'm reviewing this code. In SubpageSortkey_body.php there doesn't seem to be any exception handling (http://php.net/manual/en/language.exceptions.php). Can we reasonably expect that the input is valid in all cases?

For security reasons it may also be sensible to validate and escape the method args. I'm not entirely certain however of the entry point of these methods so there may be reason to assume that this is not a significant risk. It would be great if we could chat a bit about this. You can reach me at rfaulkner@wikimedia.org or find me on IRC under 'rfaulk' - I can typically be found in #wikimedia-e3.

#Comment by Bawolff (talk | contribs)   14:32, 23 July 2012

Thanks for taking the time to look at this.

Sorry I can't come on irc right now (I do use the nick #bawolff and hang out in #mediawiki and #wikimedia-dev on occasion). I'm responding here, hope that is ok.

>I'm reviewing this code. In SubpageSortkey_body.php there doesn't seem to be any exception handling (http://php.net/manual/en/language.exceptions.php). Can we reasonably expect that the input is valid in all cases?

What sort of input are you referring to? $wgSubpageSortkeyDefault could of course be set to something non-sensical, however that variable isn't controlled by the user, so in general we don't have to worry quite as much about it having a stupid value. With that said, this code behaves reasonably (imo) for any possible value of $wgSubpageSortkeyDefault. If it gets something totally non-sensical, php's intval will treat it as a 0.

If one wants to get super paranoid, I suppose we could check that $title is actually a Title object, but most MW code makes the assumption that calling conventions of hooks do not change and are passed appropriate things.

>For security reasons it may also be sensible to validate and escape the method args. I'm not entirely certain however of the entry point of these methods so there may be reason to assume that this is not a significant risk. It would be great if we could chat a bit about this

This method is called only from Title::getCategorySortkey (~line 4477 of includes/Title.php )

There's really nothing to escape against. The results don't end up in html (well it can be queried by the api, but that's all escaped fine), the results do end up in the db (See LinksUpdate.php), but they are escaped properly by MW's standard db functions. I don't believe there's any illegal characters for the collation algorithm (We used to use NULL as a separator. if NULL's don't cause problems, I can't imagine anything else does. The entire notion of a collation algorithm requires it to work on anything that's valid utf8) Not to mention that MW's input sanitization methods strip out any such weird characters that are inserted into an article's text.

#Comment by Renklauf (talk | contribs)   20:00, 24 July 2012

My pleasure to have a look.

Sounds like we're good from a security perspective, mitigating any XSS risks. As to the first point, I was referring to the paramaters passed to each function - if the expectation is that these params are provably well formed and/or can't break the existing functionality I think that's fine.

I just missed you on IRC, I'll be there on and off all afternoon.

Status & tagging log