MediaWiki r88113 - Code Review

Jump to: navigation, search
Repository:MediaWiki
Revision:r88112‎ | r88113 (on ViewVC)‎ | r88114 >
Date:17:11, 14 May 2011
Author:ialex
Status:resolved (Comments)
Tags:
Comment:
Rewrote the article counting code and related:
* (bug 26033, bug 24754) Added $wgArticleCountMethod to have a more flexible way to define which method to use to define if a page is an article or not and deprecated $wgUseCommaCount. There is now a new 'any' method to count any article that is in a content namespace and not a redirect.
* (bug 11868) If using links to count articles, Article::isCountable() will now use the ParserOutput to check if there's a link instead of checking for the "[[" string. Changed Article::isCountable() to take a stdObject or false for the first parameters. If false is passed, the result will be based on the current article's state (i.e. database). The only call outside of the Article class is in DeleteAction (including extensions).
* Removed this horror of Article::$mGoodAdjustment and Article::$mTotalAdjustment, replaced by the new $created parameter on Article::editUpdates(); simplified Article::createUpdates()
* Updated Import.php to take advantage of the new parameter and make a single call to Article::editUpdates()
Modified paths:

Diff [purge]

Loading diff…

Follow-up revisions

Rev.Commit summaryAuthorDate
r91180Fixes for r88113 and some realted changes:...ialex15:26, 30 June 2011

Past revisions this follows-up on

Rev.Commit summaryAuthorDate
r53027* Refactor all of InitStats crap into SiteStatsInit class. Update all code us...demon00:27, 10 July 2009

Comments

#Comment by Nikerabbit (talk | contribs)   06:06, 15 May 2011

/me dislikes the new (and old) unreadable boolean parameters.

#Comment by Aaron Schulz (talk | contribs)   06:12, 24 June 2011

1) If you have the 'comma' counting method set, and do something like:

$a = new Article( $title_of_existing_page );
$a->doEdit( ... );

...nothing will get called that process caches the text getRawText() needs (I don't see anything that sets contentLoaded yet). Then updateRevisionOn() will get called and a COMMIT done to the DB before $this->editUpdates(). This means that page_latest is already updated (even if we didn't commit, if we happened hit the same DB, a MySQL session will "see" it's own uncommitted changes).

editUpdates() will get called and cause some lines of code to be hit:

1. $good = (int)$this->isCountable( $editInfo ) - (int)$this->isCountable();
2. $text = $this->getRawText(); [in isCountable]
3. $rev = Revision::newFromTitle( $this->mTitle ); [in getRawText]

...this will query the DB based on page_latest. This will fetch the new text, not the old text. Even when/if this gets the proper old text, it's still an extra hit (to memcached) to get the text. You should pass $oldtext (from $oldtext = $this->getRawText(); // current revision) down to the editUpdates somehow.

2) I grepped and don't see any extensions called editUpdates(). I suggest refactoring the params. I'd combine $minoredit, $created, and $changed into one $flags param using constants instead of booleans. $timestamp_of_pagechange could also be removed for being unused. If we really need b/c, then we can make a doEditUpdates() like this and leave editUpdates() as wrapper around it.

#Comment by IAlex (talk | contribs)   15:27, 30 June 2011

Done in r91180.

Status & tagging log