Last modified: 2014-04-29 16:13:15 UTC
In trunk I've enabled the Debug variables and the following line showed up among others: 0.0271 Class SkinVector not found; skipped loading Originated from AutoLoader.php static function autoload( $className ) { ... if ( !$filename ) { if ( function_exists( 'wfDebug' ) ) { wfDebug( "Class {$className} not found; skipped loading\n" ); } .. }
Caused by Skin::newFromKey. It calls class_exists() on the current skin name, and then requireOnce's the skin file if the class doesn't exist. class_exists causes the autoloader to try to load the SkinVector class, which it can't since the skin classes aren't loaded by the autoloader, but by Skin::newFromKey. Thus the debug message is harmless, albeit a bit confusing. Could probably be fixed by passing false as a second parameter to the class_exists.
Not an issue in resource loader or Vector. This has been cluttering my debug output since time immemorial.
I previously said >Could probably be fixed by passing false as a second parameter to the >class_exists. Actually, that'd probably mess things up if a skin type extension tries to use the autoloader to load an extension skin. I have no idea if any skin extensions try to do that, but its conceivable they might. Maybe output a wfDebug line just before doing the check saying to ignore the next line, but then that is more clutter. Maybe just wontfix?
That's the exact same conclusion I came to when looking at this last night. Right now (I believe) the official skin docs say to throw your custom skin in /skins with all the others. But why should you have to? Assuming the skin names are *always* called SkinName with Name being the key, there's zero reason you can't keep your skin file in /extensions/whatever and just register in the autoloader. We should confirm this behavior works.
Moving to General/Unknown, this problem plagues all skins because none of them are in the autoloader.
Changed the description
First off, sorry that I can't post a diff, as I don't have a copy of mediawiki off of trunk. Basing all of this off of the svn.mediawiki.org. Hopefully the code is clear, but here's the overview. Skin extensions are registered through $wgValidSkinNames. All default skins are added to this array. As the function loops through, if the skin is not yet in the array (likely a default skin), the skin is added to the array, but in the revision the value is another array with the skin Name, and false (for passing to classExists) which will avoid the autoloader. Elseif condition picks up Skins that are registered (likely by the user) and then checks to see if the key matches an array. If so, the user's already familiar with this revision. If not, retains the original value, adds it to the array with true so that Skin uses the autoloader. At that point, you just need to modify Skin::newFromKey as below in order to have $skinName stay a string, and pass the appropriate value from $skinNames. And update the docs on $wgValidSkinNames of course. Runs fine on my version of 1.17, and I'm hoping that this patch addresses concerns regarding Skin Extensions as well as backward compatibility. This is my first attempt at a patch, so please let me know if I horribly screwed up. Revision 100445: includes/Skin.php Ln 49 - $wgValidSkinNames[strtolower( $aSkin )] = $aSkin; New Version: Ln 49 + if (!$wgValidSkinNames[strtolower( $aSkin )]) { Ln 50 + $wgValidSkinNames[strtolower( $aSkin )] = array( $aSkin, false); Ln 51 + } elseif (!is_array($wgValidSkinNames[strtolower( $aSkin )])) { Ln 52 + $wgValidSkinNames[strtolower( $aSkin )] = array( Ln 53 + $wgValidSkinNames[strtolower( $aSkin )] , true); Ln 54 + } Skin::newFromKey Ln 131 - $skinName = $skinNames[$key]; Ln 132 + $skinName = $skinNames[$key][0]; Ln 135 - if ( !MWInit::classExists( $className ) { Ln 135 + if ( !MWInit::classExists( $className , $skinNames[$key][1]) ) {
Created attachment 9339 [details] Proposed Patch Sorry about the double post. Couldn't resist making the patch. That said, I noticed in trunk that newfromkey uses MWInit::classExists, and it looks like my solution just overloads that function. Additionally, I don't think this even goes through Autoloader anymore, which means the problems gone. Whether this still needs patching, or it doesn't because the problems already been eliminated, I'm guessing this bug can be closed.
I don't want to see any of that patch going into core: - The way it modifies a core config global at runtime to hack things in is absolutely horrible - MWInit::classExists doesn't have a second argument so this code doesn't do anything - If class_exists were being used this would disable the ability to override built-in skins simply by defining just an autoloader entry. One possible acceptable way to fix this would be to change the code so that we actually autoload the built in skins by registering them in the autoloader and skins array dynamically instead of using a require fallback.
This is why I thought I shouldn't be trying to contribute here. I'll leave bugs be.
Comment on attachment 9339 [details] Proposed Patch Marking obsolete for that reason
Reply to Ed in comment 10: > This is why I thought I shouldn't be trying to contribute here. I'll leave > bugs be. Please don't stop looking for bugs to fix. MediaWiki isn't an easy codebase to understand rightaway. We all have a significant learning curve at the beginning.
Latest master: Start request GET / HTTP HEADERS: HOST: alpha.wikipedia.krinkle.dev CONNECTION: keep-alive ACCEPT: text/html,application/xhtml+xml,application/xml;q=0.9,image/webp,*/*;q=0.8 USER-AGENT: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_9_2) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/34.0.1847.131 Safari/537.36 ACCEPT-ENCODING: gzip,deflate,sdch ACCEPT-LANGUAGE: en,nl;q=0.8,en-US;q=0.6,de;q=0.4 COOKIE: * [caches] main: SqlBagOStuff, message: SqlBagOStuff, parser: SqlBagOStuff [caches] LocalisationCache: using store LCStoreCDB Fully initialised Connected to database 0 at localhost Connected to database 0 at localhost MessageCache::load: Loading en... got from local cache Dependency triggered: mediawiki/extensions/VisualEditor/modules/ve-mw/i18n/en.json changed. LocalisationCache::isExpired(en): cache for en expired due to FileDependency LocalisationCache::recache: got localisation for en from source DatabaseBase::query: Writes done: DELETE FROM `msg_resource` Title::getRestrictionTypes: applicable restrictions to [[Main Page]] are {edit,move} [ContentHandler] Created handler for wikitext: WikitextContentHandler User: got user 1 from cache User: loading options for user 1 from override cache. User: logged in from session OutputPage::sendCacheControl: private caching; Tue, 29 Apr 2014 16:10:39 GMT ** DatabaseBase::query: Writes done: DELETE FROM `objectcache` WHERE keyname = 'alphawiki:jobqueue:queueshavejobs:1' AND exptime = '2014-04-29 11:50:05' LoadBalancer::reuseConnection: this connection was not opened as a foreign connection Request ended normally wfClientAcceptsGzip: client accepts gzip. wfGzipHandler() is compressing output Start request GET /wiki/Main_Page HTTP HEADERS: HOST: alpha.wikipedia.krinkle.dev CONNECTION: keep-alive ACCEPT: text/html,application/xhtml+xml,application/xml;q=0.9,image/webp,*/*;q=0.8 USER-AGENT: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_9_2) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/34.0.1847.131 Safari/537.36 ACCEPT-ENCODING: gzip,deflate,sdch ACCEPT-LANGUAGE: en,nl;q=0.8,en-US;q=0.6,de;q=0.4 COOKIE: * IF-MODIFIED-SINCE: Fri, 25 Apr 2014 23:45:04 GMT [caches] main: SqlBagOStuff, message: SqlBagOStuff, parser: SqlBagOStuff [caches] LocalisationCache: using store LCStoreCDB Fully initialised Connected to database 0 at localhost Connected to database 0 at localhost Title::getRestrictionTypes: applicable restrictions to [[Main Page]] are {edit,move} [ContentHandler] Created handler for wikitext: WikitextContentHandler User: got user 1 from cache User: loading options for user 1 from override cache. User: logged in from session User: loading options for user 1 from override cache. MessageCache::load: Loading en... got from local cache Unstubbing $wgParser on call of $wgParser::firstCallInit from MessageCache::getParser Parser: using preprocessor: Preprocessor_DOM Unstubbing $wgLang on call of $wgLang::_unstub from ParserOptions::__construct OutputPage::checkLastModified: client sent If-Modified-Since: 2014-04-25T23:45:04Z OutputPage::checkLastModified: effective Last-Modified: 2014-04-25T23:45:04Z OutputPage::checkLastModified: NOT MODIFIED, page=2014-04-02T23:37:57Z, user=2014-04-25T23:45:04Z, epoch=2003-05-16T00:00:00Z OutputPage::sendCacheControl: private caching; Fri, 25 Apr 2014 23:45:04 GMT ** wfClientAcceptsGzip: client accepts gzip. wfGzipHandler() is compressing output Article::view: done 304 Request ended normally
Looks like it's not there anymore.