skins: Remove 'usemsgcache' and deprecate getDynamicStylesheetQuery

For several years now, action=raw urls are automatically purged
after edits, and receive the same cache allowance as page views.

The vast majority of action=raw requests come from the
importScript() function in wikibits.js, which only specififes
'title', 'action' and 'ctype'.

The 'usemsgcache' parameter is a relic (pre-2009) for optimising
something we haven't used in a very long time.  Nothing uses the
'usemsgcache' parameter, except for the one edge-case of
third-party wikis loading Filepage.css from Commons on local
file description pages. This commit removes the handling logic,
in favour of letting the normal Revision-logic happen instead,
which is compatible and produces the same response.

Before 2009, this probably made sense as an optimisation because
we didn't have Revision text cache in Memcached yet at that time,
to avoid DB-load from InstantCommons-based file page views.

The way FileRepo was using it needlessly allowed Filepage.css
to return stale responses (given non-canonical query parameters
variations are not purged on-edit). It also caused responses to
be cached longer or shorter than they should be, due to it being
based on the client wiki's configuration, instead of the repo's
configuration (e.g. Wikimedia Commons). By omitting these
'maxage' parameters, the $wgSquidMaxage config of the repo is
used instead, which makes more sense.

Bug: T193271
Change-Id: Ie52195b56b7f8fc17dec0cbedd6d77968e3bc040

diff --git a/RELEASE-NOTES-1.32 b/RELEASE-NOTES-1.32
index 60f5e6a..4ece115 100644 (file)
--- a/RELEASE-NOTES-1.32
+++ b/RELEASE-NOTES-1.32
@@ -466,6 +466,8 @@ because of Phabricator reports.
 * QuickTemplate::msgHtml() and BaseTemplate::msgHtml() have been deprecated
   as they promote bad practises. I18n messages should always be properly
   escaped.
+* Skin::getDynamicStylesheetQuery() has been deprecated. It always
+  returns action=raw&ctype=text/css which callers should use directly.
 
 === Other changes in 1.32 ===
 * (T198811) The following tables have had their UNIQUE indexes turned into

diff --git a/includes/actions/RawAction.php b/includes/actions/RawAction.php
index 463019f..b5a6d3a 100644 (file)
--- a/includes/actions/RawAction.php
+++ b/includes/actions/RawAction.php
@@ -163,64 +163,35 @@ class RawAction extends FormlessAction {
                $title = $this->getTitle();
                $request = $this->getRequest();
 
-               // If it's a page in the MediaWiki namespace, we can just hit the message cache
-               if ( $request->getBool( 'usemsgcache' ) && $title->getNamespace() == NS_MEDIAWIKI ) {
-                       // FIXME: The overhead and complexity of using MessageCache for serving
-                       // source code is not worth the marginal gain in performance. This should
-                       // instead use Revision::getRevisionText, which already has its own caching
-                       // layer, which is good enough fine given action=raw only responds with
-                       // a single page (no need for batch).
-                       //
-                       // Use of MessageCache:
-                       // - is unsustainable (T193271),
-                       // - can cause bugs due to "post-processing" (see MessageCache::get) not
-                       //   intending to apply to program source code,
-                       // - causes uncertaintly around whether or not localisation default
-                       //   placeholders are, can, and should be used, or not.
-                       $text = MessageCache::singleton()->get(
-                               $title->getDBkey(),
-                               // Yes, use the database.
-                               true,
-                               // Yes, use the content language.
-                               true,
-                               // Yes, the message key already contains the language in it ("/de", etc.)
-                               true
-                       );
-                       // If the local page doesn't exist, return a blank (not the default)
-                       if ( $text === false ) {
-                               $text = '';
-                       }
-               } else {
-                       // Get it from the DB
-                       $rev = Revision::newFromTitle( $title, $this->getOldId() );
-                       if ( $rev ) {
-                               $lastmod = wfTimestamp( TS_RFC2822, $rev->getTimestamp() );
-                               $request->response()->header( "Last-modified: $lastmod" );
+               // Get it from the DB
+               $rev = Revision::newFromTitle( $title, $this->getOldId() );
+               if ( $rev ) {
+                       $lastmod = wfTimestamp( TS_RFC2822, $rev->getTimestamp() );
+                       $request->response()->header( "Last-modified: $lastmod" );
+
+                       // Public-only due to cache headers
+                       $content = $rev->getContent();
 
-                               // Public-only due to cache headers
-                               $content = $rev->getContent();
+                       if ( $content === null ) {
+                               // revision not found (or suppressed)
+                               $text = false;
+                       } elseif ( !$content instanceof TextContent ) {
+                               // non-text content
+                               wfHttpError( 415, "Unsupported Media Type", "The requested page uses the content model `"
+                                       . $content->getModel() . "` which is not supported via this interface." );
+                               die();
+                       } else {
+                               // want a section?
+                               $section = $request->getIntOrNull( 'section' );
+                               if ( $section !== null ) {
+                                       $content = $content->getSection( $section );
+                               }
 
-                               if ( $content === null ) {
-                                       // revision not found (or suppressed)
+                               if ( $content === null || $content === false ) {
+                                       // section not found (or section not supported, e.g. for JS, JSON, and CSS)
                                        $text = false;
-                               } elseif ( !$content instanceof TextContent ) {
-                                       // non-text content
-                                       wfHttpError( 415, "Unsupported Media Type", "The requested page uses the content model `"
-                                               . $content->getModel() . "` which is not supported via this interface." );
-                                       die();
                                } else {
-                                       // want a section?
-                                       $section = $request->getIntOrNull( 'section' );
-                                       if ( $section !== null ) {
-                                               $content = $content->getSection( $section );
-                                       }
-
-                                       if ( $content === null || $content === false ) {
-                                               // section not found (or section not supported, e.g. for JS, JSON, and CSS)
-                                               $text = false;
-                                       } else {
-                                               $text = $content->getNativeData();
-                                       }
+                                       $text = $content->getNativeData();
                                }
                        }
                }

diff --git a/includes/filerepo/FileRepo.php b/includes/filerepo/FileRepo.php
index 858e124..455d38f 100644 (file)
--- a/includes/filerepo/FileRepo.php
+++ b/includes/filerepo/FileRepo.php
@@ -810,8 +810,9 @@ class FileRepo {
         */
        public function getDescriptionStylesheetUrl() {
                if ( isset( $this->scriptDirUrl ) ) {
-                       return $this->makeUrl( 'title=MediaWiki:Filepage.css&' .
-                               wfArrayToCgi( Skin::getDynamicStylesheetQuery() ) );
+                       // Must match canonical query parameter order for optimum caching
+                       // See Title::getCdnUrls
+                       return $this->makeUrl( 'title=MediaWiki:Filepage.css&action=raw&ctype=text/css' );
                }
 
                return false;

diff --git a/includes/skins/Skin.php b/includes/skins/Skin.php
index e426f7f..ed4045d 100644 (file)
--- a/includes/skins/Skin.php
+++ b/includes/skins/Skin.php
@@ -414,17 +414,13 @@ abstract class Skin extends ContextSource {
        /**
         * Get the query to generate a dynamic stylesheet
         *
+        * @deprecated since 1.32 Use action=raw&ctype=text/css directly.
         * @return array
         */
        public static function getDynamicStylesheetQuery() {
-               global $wgSquidMaxage;
-
                return [
                                'action' => 'raw',
-                               'maxage' => $wgSquidMaxage,
-                               'usemsgcache' => 'yes',
                                'ctype' => 'text/css',
-                               'smaxage' => $wgSquidMaxage,
                        ];
        }
 

diff --git a/maintenance/dictionary/mediawiki.dic b/maintenance/dictionary/mediawiki.dic
index ff06e49..a0177b1 100644 (file)
--- a/maintenance/dictionary/mediawiki.dic
+++ b/maintenance/dictionary/mediawiki.dic
@@ -4322,7 +4322,6 @@ useemail
 uselang
 uselivepreview
 usemod
-usemsgcache
 usenewrc
 user
 useragent