Page MenuHomePhabricator

Skins clutter Debug output since they aren't in the AutoLoader
Closed, DeclinedPublic

Description

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" );
				}

..
}


Version: 1.18.x
Severity: minor

Details

Reference
bz26753

Event Timeline

bzimport raised the priority of this task from to Medium.Nov 21 2014, 11:24 PM
bzimport set Reference to bz26753.
bzimport added a subscriber: Unknown Object (MLST).

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.

ed.wunch wrote:

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]) ) {

ed.wunch wrote:

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.

attachment SkinValid.patch ignored as obsolete

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.

ed.wunch wrote:

This is why I thought I shouldn't be trying to contribute here. I'll leave bugs be.

Comment on attachment 9339
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.