r111185 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r111184‎ | r111185 | r111186 >
Date:19:26, 10 February 2012
Author:werdna
Status:fixme (Comments)
Tags:
Comment:
(bug 21256) Add "islqttalkpage" parameter to prop=info. Patch is by John Du Hart, with minor style and formatting tweaks by me.
Modified paths:
  • /trunk/extensions/LiquidThreads/LiquidThreads.php (modified) (history)
  • /trunk/extensions/LiquidThreads/classes/Hooks.php (modified) (history)

Diff [purge]

Index: trunk/extensions/LiquidThreads/LiquidThreads.php
——@@ -154,6 +154,9 @@
155155 // JS variables
156156 $wgHooks['MakeGlobalVariablesScript'][] = 'LqtHooks::onMakeGlobalVariablesScript';
157157
 158+// API
 159+$wgHooks['APIQueryAfterExecute'][] = 'LqtHooks::onAPIQueryAfterExecute';
 160+
158161 // Special pages
159162 $wgSpecialPages['MoveThread'] = 'SpecialMoveThread';
160163 $wgSpecialPages['NewMessages'] = 'SpecialNewMessages';
Index: trunk/extensions/LiquidThreads/classes/Hooks.php
——@@ -862,4 +862,21 @@
863863 $list[NS_LQT_SUMMARY_TALK] = 'Summary_talk';
864864 return true;
865865 }
 866+
 867+ public static function onAPIQueryAfterExecute( &$module ) {
 868+ if( $module instanceof ApiQueryInfo ) {
 869+ $result = $module->getResult();
 870+ $data = $result->getData();
 871+
 872+ foreach( $data['query']['pages'] as $pageid => $page ) {
 873+ if( $page == 'page' ) continue;
 874+
 875+ if( LqtDispatch::isLqtPage( Title::newFromText( $page['title'] ) ) ) {
 876+ $result->addValue( array( 'query', 'pages' ), $pageid, array( 'islqttalkpage' => '' ) );
 877+ }
 878+ }
 879+ }
 880+
 881+ return true;
 882+ }
866883 }

Comments

#Comment by MZMcBride (talk | contribs)   22:32, 10 February 2012

Minor nitpick: "islqttalkpage" could be misleading if someone thinks it only refers to talk namespaces. Maybe "islqtpage" would be better (from LqtDispatch::isLqtPage)?

#Comment by Duplicatebug (talk | contribs)   18:15, 11 February 2012

Please do not add unconditionally new output to a api module. It is better, to add new option to prop=info and check, if the option is set and than add the information. That avoid a waste of performance/time, when the caller does not need that information. With the param you have a documentation at the api help page, which is also a nice to have.

You should not create your own title objects (A function you call, need the page id, which is not set in your case and that means a seperate query to the database).

Better it is to get the PageSet and operate on the "GoodTitles" (that all pages from the query, which are valid and exist) and maybe on the "MissingTitles" (if you want support this information for non-existing pages).

#Comment by MZMcBride (talk | contribs)   21:12, 21 March 2012

Roan: Did you read the comments here?

#Comment by Catrope (talk | contribs)   21:22, 21 March 2012

I did, but I think these concerns aren't pressing enough to justify reverting this revision for the git switchover.

#Comment by MZMcBride (talk | contribs)   21:34, 21 March 2012

I'm very worried that the haste with which code review is being done leading up to the Git migration is compromising the integrity of the code base.

The reality is that a revision like this, if marked "ok", is never going to be re-reviewed. It hasn't been touched in six weeks and it's not as though the workload is suddenly decreasing for those who do code review work.

Accepting problematic (or mediocre or subpar or...) code due to an arbitrary deadline seems incredibly dangerous and, frankly, stupid.

#Comment by Reedy (talk | contribs)   21:29, 10 April 2012

Bug 35865

Status & tagging log