Author: mccormack
Description:
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.
Version: unspecified
Severity: enhancement
URL: http://www.mediawiki.org/wiki/Extension:SubPageList3
mccormack wrote:
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).
mccormack wrote:
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.
mccormack wrote:
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
mccormack wrote:
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.
mccormack wrote:
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.
mccormack wrote:
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.
mccormack wrote:
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.
mccormack wrote:
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.
bastique.bz wrote:
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.
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?
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:SubPageListThis 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...