r70809 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r70808‎ | r70809 | r70810 >
Date:12:10, 10 August 2010
Author:platonides
Status:ok (Comments)
Tags:
Comment:
Follow up r70783 CR.
Document getETag()
Modified paths:
  • /trunk/phase3/includes/parser/ParserCache.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/parser/ParserCache.php
——@@ -48,6 +48,12 @@
4949 return wfMemcKey( 'pcache', 'idoptions', "{$pageid}" );
5050 }
5151
 52+ /**
 53+ * Provides an E-Tag suitable for the whole page, even if $article is
 54+ * just the main wikitext. So it uses the complete set of user options.
 55+ * Most importantly, that includes the user language, but other options
 56+ * would give problems on some setups, too.
 57+ */
5258 function getETag( $article, $popts ) {
5359 return 'W/"' . $this->getParserOutputKey( $article,
5460 $popts->optionsHash( ParserOptions::legacyOptions() ) ) .
——@@ -74,7 +80,7 @@
7581
7682 // Determine the options which affect this article
7783 $optionsKey = $this->mMemc->get( $this->getOptionsKey( $article ) );
78 - if ( $optionsKey !== false ) {
 84+ if ( $optionsKey != false ) {
7985 if ( !$useOutdated && $optionsKey->expired( $article->mTouched ) ) {
8086 wfIncrStats( "pcache_miss_expired" );
8187 $cacheTime = $optionsKey->getCacheTime();

Follow-up revisions

RevisionCommit summaryAuthorDate
r71515Try to improve the funciton comment of getETag() per r70809 CR. Adaptation of...platonides21:54, 23 August 2010

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r70783Use only the page relevant pieces in the parser cache key. Eg. two users with...platonides21:53, 9 August 2010

Comments

#Comment by Nikerabbit (talk | contribs)   15:19, 10 August 2010 
+if ( $optionsKey != false ) {

That's same as !$optionsKey, not?

The comment for ETag just confuses me. For example even if $article is just the main wikitext.

When is the case it not just the main text?
  • So it uses
What uses? Article? This method? Uses for what?
  • that includes
What includes?
  • but other options would give problems on some setups, too.
What options? What problems? On which setups? What does the function caller need to know?
#Comment by Platonides (talk | contribs)   15:37, 10 August 2010

> That's same as !$optionsKey, not?

Yes.


You usually work on pages, and so does ParserCache and ParserOptions. For www.mediawiki.org/wiki/Foo you have several items being parsed (or retrieved from cache), each with its own Options: Foo, MediaWiki:sidebar, MediaWiki:sitenotice...

The E-Tag has to be unique to the whole page, so even if bodyArticle is the same (Foo, what I referred to as main wikitext, that will be the article hold in $article) so we spit the full list of options, as other pieces may differ, we don't want to give a Chinese interface to a user with English preferences (admitedly, that would require some caching proxy in between).

Other options are other things tracked by ParserOptions, but it would be more convoluted than the language example, eg. LaTeX in a sitenotice.


How do you recommend expressing this?

#Comment by Nikerabbit (talk | contribs)   08:18, 23 August 2010

Just make it over explicit. How about:

Provides an E-Tag suitable for the whole page. Note that $article is just the main wikitext. The E-Tag has to be unique to the whole page, even if the article itself is the same. For example we don't want to give a Chinese interface to a user with English preferences. That's why we take into account *all* user options, like the user language.

Status & tagging log