Last modified: 2011-06-28 16:12:04 UTC
Following IRC discussion and code updates, this is now on SVN. Please activate for English and Greek Wikiversity (possibly the others too - I think they all want it). Thanks.
I guess a link to some kind of poll among local communities is needed.
English WV vote: http://en.wikiversity.org/wiki/Wikiversity:Colloquium#Discussion_and_voting Greek WV vote: http://el.wikiversity.org/wiki/%CE%91%CE%B3%CE%BF%CF%81%CE%AC#.CE.A0.CF.81.CF.8C.CF.84.CE.B1.CF.83.CE.B7_.CE.B3.CE.B9.CE.B1_.CF.84.CE.B7.CE.BD_.CF.87.CF.81.CE.AE.CF.83.CE.B7_.CF.84.CE.B7.CF.82_.CE.B5.CF.80.CE.AD.CE.BA.CF.84.CE.B1.CF.83.CE.B7.CF.82_Sub_Page_List_2 The Greeks reported the result in English on the English WV at the above URL (for those who don't speak Greek).
This should probably be restricted to the English and Greek Wikiversities, who produced good and unanimous votes. Further discussion has indicated that interest is not very high at the other Wikiversities at the current time.
As regards code review, I *think* it's passed, but not quite sure. A number of devs took "a look" at it (including Werdna, Tim Starling, Splarka), of whom Werdna did the most thorough job, Tim made a quick requirement and Splarka discussed the purpose and intent. I'm not sure if this amounts to what would be called "code-review", but it certainly resulted in some changes, and Werdna uploaded it to SVN when he was happy with it. Is this what you call code-review? -- McCormack
Don't turn it on unless/until I've okayed it.
Incidentally, when this was uploaded to SVN, somebody seems to have improved my code and introduced a small bug while doing so. Their line: $nsi = $this->title->getNamespace(); Should be: $nsi = $this->ptitle->getNamespace(); (introduce a "p" to fix the bug) See report by PaulHat at http://www.mediawiki.org/wiki/Extension_talk:SubPageList3 and my response to this.
#6 fixed in r31535
The following comment was added by Brion Vibber to Bug ID 13066 but applies here. --------------------------------------------------------------------------------- Made some minor code tweaks in r32559. Some fundamental issues which need to be fixed before any Wikimedia deployment: Sorting: The code causes an inefficient filesort operation by sorting on UPPER(page_title) instead of page_title. Depending on how smart the database is, that may be a non-issue (small result data sets) or it may be wildly inefficient, but it's a red flag. More generally, note that the case-insensitive intention of this sort will fail on non-ASCII characters in MySQL 4 and default compat configurations, as the transformation applied at the DB won't be correct. Recommend sorting simply on page_title, as elsewhere in the system (eg Special:Prefixindex). Caching: Currently the extension outright disables caching on any page using the <splist/> tag. While this ensures that up-to-date page lists are displayed, it could make a site widely using it very expensive indeed -- the entire page, however large, needs to be reparsed on every view -- even under a load spike like a Slashdotting, where the same page is hit over and over. To correctly handle cache updates, uses on a page should be registered at links-update time, and creation/deletion of a subpage can trigger update jobs for registered pages.
*** Bug 13066 has been marked as a duplicate of this bug. ***
In version 1.05 both of Brion's recommendations have been addressed. Sorting: the "upper" has been removed. Caching: the cache disabling in the constructor (which TimStarling and Werdna had recommended adding) has been removed so that the extension content is no longer dynamic. I discussed Brion's comment here at length with Duesentrieb, Dantman and Darkcode, all of whom see a "render dependency table" as the best way forward - but this is probably a major step which would have to wait for a future version. Even Duesentrieb's functionally related CategoryTree extension is not dynamic, so I think we can probably go initially for a simple cache-friendly user-not-quite-so-friendly version. An SVN commit request has been made (separate bug). Activation is requested.
The Wikiversity voting for activation has moved around because this request is now aging. The votes are currently at: http://en.wikiversity.org/wiki/Wikiversity:Colloquium#Discussion_and_voting Reminder: both English and Greek Wikiversities have unanimously requested activation.
I discussed the removal of cache disabling with Brion on IRC at the beginning of April - i.e. taking a provisional simple-safe method on the caching issue. Brion asked questions and then agreed with this route. Activation was requested. As of 29th April, the situation seems to be that the extension is now queued for "full review". Please review and activate. Thank you.
I've had a discussion on IRC with some of the community members and they do seem quite anxious for the bug to be implemented. I guess it's just a matter of Brion's okaying it.
Assigning this to myself for review so it's not forgotten forever. :)
There are some style issues which anyone could fix up, like using English for variable names: $parlv = substr_count($this->ptitle->getPrefixedText(), '/'); whitespace, lack of line breaks where there should be line breaks: switch( strtolower( $options['showpath'] ) ) { case 'no': $this->showpath = 'no'; break; ... if( $this->mode != 'bar' ) $pn = $this->token . $pn; Also, it uses non-portable, unnecessary SQL identifier quoting: $options['ORDER BY'] = '`page_title`' . $order; ... $conditions[] = '`page_title` ' . $dbr->buildLike( $parent . '/', $dbr->anyString() ); It uses Parser::parse() when it should be using Parser::recursiveTagParse(). This is an efficiency issue, since parse() does link existence checks immediately, whereas recursiveTagParse() defers them until the next batch. It's also a correctness issue, since recursiveTagParse() guards certain syntax elements against reinterpretation, whereas parse() does not. Otherwise it should be OK to enable.
r85590, r85593, r85594 does all the style cleanup r85596 does the parser issue Tim, if this is clean now, can it be pushed out?
Done. It's enabled on all Wikiversities, not just English.
I actually forked the extension and rewrite most of it at the beginning of January. See https://secure.wikimedia.org/wikipedia/mediawiki/wiki/Extension:SubPageList This extension fixes a number of bugs in SubPageList3, cleans up the code, addresses some layout issues and is actually maintained. I (obviously) suggest using this one instead.
(In reply to comment #18) > I actually forked the extension and rewrite most of it at the beginning of > January. See > https://secure.wikimedia.org/wikipedia/mediawiki/wiki/Extension:SubPageList > > This extension fixes a number of bugs in SubPageList3, cleans up the code, > addresses some layout issues and is actually maintained. I (obviously) suggest > using this one instead. Validator has to be reviewed aswell to be able to do that
Ah, I suspected this was the issue. Fair enough - as long as you know there is a "newer version".
(In reply to comment #20) > Ah, I suspected this was the issue. Fair enough - as long as you know there is > a "newer version". Not really, AFAIK it wasn't even considered to be installed as the request was for Subpages3, and no one commented before...
