r96546 MediaWiki - Code Review archive

{{GENDER}} is not optimised for this use case. It does three database queries per log row when you use it like this.

What queries? I only see few bugs like User::loadOptions called repeatedly for user which has no options.

This queries are used for each gender parser function:

<!--SELECT  *  FROM `user`  WHERE user_id = '1'  LIMIT 1   (MediaWiki::run/MediaWiki::main/MediaWiki::performRequest/SpecialPageFactory::executePath/SpecialLog::execute/SpecialLog::show/IndexPager::getBody/LogPager::formatRow/LogEventsList::logLine/LogFormatter::getActionText/Message::escaped/Message::toString/Message::transformText/MessageCache::transform/Parser::transformMsg/Parser::preprocess/Parser::replaceVariables/PPFrame_DOM::expand/Parser::braceSubstitution/call_user_func_array/CoreParserFunctions::gender/User::getOption/User::loadOptions/User::load/User::loadFromId/User::loadFromDatabase/DatabaseBase::selectRow/DatabaseBase::select)-->
<!--SELECT  ug_group  FROM `user_groups`  WHERE ug_user = '1'   (MediaWiki::run/MediaWiki::main/MediaWiki::performRequest/SpecialPageFactory::executePath/SpecialLog::execute/SpecialLog::show/IndexPager::getBody/LogPager::formatRow/LogEventsList::logLine/LogFormatter::getActionText/Message::escaped/Message::toString/Message::transformText/MessageCache::transform/Parser::transformMsg/Parser::preprocess/Parser::replaceVariables/PPFrame_DOM::expand/Parser::braceSubstitution/call_user_func_array/CoreParserFunctions::gender/User::getOption/User::loadOptions/User::load/User::loadFromId/User::saveToCache/User::loadGroups/DatabaseBase::select)-->
<!--SELECT  *  FROM `user_properties`  WHERE up_user = '1'   (MediaWiki::run/MediaWiki::main/MediaWiki::performRequest/SpecialPageFactory::executePath/SpecialLog::execute/SpecialLog::show/IndexPager::getBody/LogPager::formatRow/LogEventsList::logLine/LogFormatter::getActionText/Message::escaped/Message::toString/Message::transformText/MessageCache::transform/Parser::transformMsg/Parser::preprocess/Parser::replaceVariables/PPFrame_DOM::expand/Parser::braceSubstitution/call_user_func_array/CoreParserFunctions::gender/User::getOption/User::loadOptions/User::load/User::loadFromId/User::saveToCache/User::loadOptions/DatabaseBase::select)-->

More information: The performer of the logs makes now problems. They are handled by one query (the LinkBatch in LogPager::getStartBody). The problem are links to user pages inside the comments which are not performer (Linker::commentBlock). Than each link result in an own query to get the gender state. But that sounds hard to do at that place.

For block log entries the method Linker::userToolLinks is using the user talk page, which is not inside the LinkBatch and Linker::getTitleLink is calling User::idFromName for each entry.

There is not for every user the need to add usernames of (partial) rev deleted log entries to the linkbatch, but that sounds not so easy to do.

Here is the query log for Special:Log/move on my test wiki: http://paste.tstarling.com/p/rQxouc.html

Disabled the relevant feature in

Disabled the relevant feature in r110909.

If you have memcached enabled, you get less queries: one per row for users with default options before r110910, and zero per row after r110910. I think a single DB query would be faster than 50 memcached queries, so that may be the best option, unless BagOStuff were given support for MWMemcached::get_multi() or similar. Loading from the database is tricky because User::newFromResult() doesn't support loading data from user_properties, so the interfaces would have to be changed.

Correct. I have some ideas how to query them at once, but that can probably be done after 1.19 and release 1.19 with your workaround?

Yes, but I'm still waiting for the code around line 293 of LogPage.php to be fixed. LogPage::actionText() is a public function, it should do something sensible if it's called for delete/delete or the like. I thought that's what Aaron meant when he complained about the code in his comment below.

In LogPage we have:

$rv = call_user_func_array( $wgLogActionsHandlers[$key], $args );

But $wgLogActionsHandlers is being set to uses class names as callbacks, not functions. I get things like: Warning: call_user_func_array() expects parameter 1 to be a valid callback, function 'DeleteLogFormatter' not found or invalid function name in D:\www\MediaWiki\phase3\includes\logging\LogPage.php on line 296

Maybe the easiest fix is to have LogPage::addEntry() wrap ManualLogEntry and its insert() function.

This broken code is still present, even after r110893. Leaving status as fixme because of that.

Yeah, I was fixing other breakage first. I just fixes that problem now.

Anything using LogPage::actionText should use the Formatter and the that function should also be deprecated.

See bug 33385.

I don't see how that is related.

Should be bug 33167.

So who should see it? You know I've already seen it.

It's mostly there for reference (saw the bug # is on CR).