Last modified: 2011-11-12 10:22:44 UTC
Created attachment 9254 [details] A patch, implementation of format `count'. This is an implementation of format "count" for extension SubPageList: {{ #subpages: Page | format = count }} returns number of subpages as a plain text, suitable for using in `#ifeq' and `#ifexpr' functions, for example: {{ #ifexpr: {{ #subpages: Page | format = count }} > 0 | <!-- There are subpages. --> | <!-- There is no subpages. --> }} Note that tricks with other formats will *not* work: {{ #if: {{ #subpages: Page | format = list }} | <!-- Always occurs. --> | <!-- Never occurs. --> }} Result of `#subpages' (with non-`count' format) is never empty, even if there are no subpages. Another example: Let us assume template `x' unconditionally returns `x', so {{ #subpages: Page | format = template | template = x }} we hope result of construct will be character `x' repeated as many times as subpages we have. But this does not work. Result of `#subpages' looks like `x...x' but it cannot be used in function `#if': {{ #ifeq: {{ #subpages: Page | format = template | template = x }} | x | <!-- Never occurs. --> | <!-- Always occurs. --> }} Format `count' is inspired by extension SemanticMediaWiki. Possible improvements: Currently `intro', `outro' and other parameters are ignored if format is `count'. They could be implemented (later).
Well, I agree it's nice to just get the number of pages, but that's not what this parser functions is intended for, so I don't think it's a good idea to go hack it in like this. A new parser function, such as #subpagecount or whatever would be more appropriate, and could make use of just doing a count query rather then selecting pages without introducing some very ugly format dependent handling. Let me know if you need more pointers on how to do that :) You have commit access right?
Hmmm... I said the function is inspired by `{{ #ask: … | format=count }}'. Why `ask' is intended for counting but `#subpages' is not? I see some advantages of format `count': 1. It utilizes similarity with the function `#ask': both functions recognize `intro', `outro', `default', `format' parameters. 2. No need on one more class => file, couple of hooks, etc. Regarding the db query… There is a comment in the code: // TODO: Can `getSubPages' be optimized for format `count'?" I think one more `if' in `getSubPages' is better than duplicating much of the infrastructure, including content of `getSubPages'. However, I will not insist on this approach. You are the master, you decide. > You have commit access right? Not yet. I requested it and received an answer, but something does not work. I need some time to get it really working.
I'm actually not convinced that having format=count in ask is the best thing to do either. There is more reason to do that in ask then in subpages though, since the query parsing part of the code is significant and the same in both situations. For subpages, getting the count just needs a count query which gets directly returned. If you want a list, the query needs to select the actual pages with some limit, which then need to be formatted in some way. So you have not only a different query, but also something different that's returned (a number vs a list). Furthermore I think you do not need the intro and outro params for numbers, you can just put them in between whatever you want. So in my eyes it's completely different code, not the same.
Created attachment 9265 [details] Version 2, not yet production. Ok, this is the attempt № 2. 1. Function `#subpagecount' is implemented. 2. Common code of `#subpages' and `#subpagecount' are moved to class `SubPageBase'. (BTW, probably we have to use a prefix for class names, like 'splSubPageList', `splSubPageCount', `splSubPageBase'.) 3. Implemented correct handling of namespaces where subpages are not enabled. 4. There is a problem: My original though was that $dbr->estimateRowCount is the right way to go. However, in my local MediaWiki server it produces strange results. It returns bigger count than I expect. Looks like it always returns number of subpages, direct and indirect, regardless of parameter `kidsonly'. However, code borrowed from `Database.php', works well, while code borrowed from `DatabaseNysql.php', returns that wrong result. (My server runs MySQL, so I think `wfGetDB' returns an instance of class `DatabaseMysql'.) Please look. Implementation of the query may be selected by parameter `method': {{ #subpagecount: page | method=mysql }}
BTW, class `Title' contains method `getSubpages' which returns list of all subpages. Don't you think to tune it for SubPageList's needs? It seems parameters `sort' and `kidsonly' can be easily added...
estimateRowCount is not accurate, it's just an estimation that can be off a lot. The code you copied with COUNT(*) is the way to go, and ought to work with most SQL DBs. I think having a method param that switched the db code is very odd. If you need different behavior for different types of DBs, switch on $wgDBtype. An prefix is best yes. I don't think it ought to be SPL though. 'SubPage' also works, and results in nicer class names. Well, you'd not even need to change them I guess :p
(In reply to comment #5) > BTW, class `Title' contains method `getSubpages' which returns list of all > subpages. Don't you think to tune it for SubPageList's needs? It seems > parameters `sort' and `kidsonly' can be easily added... Then SubPageList would require the new MW, which would be 1.19. Better to just have an extended copy of the code in the extension itself.
> estimateRowCount is not accurate, > The code you copied with COUNT(*) is the way to go, ?? Do not understand. The code I copied *is* the code of `estimateRowCount' from class `Database'. > I think having a method param that switched the db code is very odd. I wrote in the comments that it is not a production code. I added it just to see the difference between different implementations. Of course parameter should be removed, and only one implementation should remain. So, you think code $res = $dbr->select( 'page', 'COUNT(*) AS rowcount', $conds, __METHOD__ ); will work everywhere? I will drop other variants then.
Created attachment 9267 [details] Version 3. The third version. Temporary code removed.
Ok, looks fine. Can you just provide a copy of the modified files (ie trunk with this patch applied)? Merge is not going smoothly for me :/
Created attachment 9268 [details] Version 3, all the files. Sure.
-need-review, +reviewed
Applied in r100468
Verified on r102853.
