Last modified: 2014-04-29 16:13:15 UTC

Wikimedia Bugzilla is closed!

Wikimedia migrated from Bugzilla to Phabricator. Bug reports are handled in Wikimedia Phabricator.
This static website is read-only and for historical purposes. It is not possible to log in and except for displaying bug reports and their history, links might be broken. See T28753, the corresponding Phabricator task for complete and up-to-date bug report information.
Bug 26753 - Skins clutter Debug output since they aren't in the AutoLoader
Skins clutter Debug output since they aren't in the AutoLoader
Status: RESOLVED WORKSFORME
Product: MediaWiki
Classification: Unclassified
General/Unknown (Other open bugs)
1.18.x
All All
: Normal minor (vote)
: ---
Assigned To: Nobody - You can work on this!
:
Depends on:
Blocks: code_quality
  Show dependency treegraph
 
Reported: 2011-01-16 03:49 UTC by Krinkle
Modified: 2014-04-29 16:13 UTC (History)
8 users (show)

See Also:
Web browser: ---
Mobile Platform: ---
Assignee Huggle Beta Tester: ---


Attachments
Proposed Patch (1.10 KB, patch)
2011-11-01 08:05 UTC, Ed
Details

Description Krinkle 2011-01-16 03:49:03 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" );
				}
..
}
Comment 1 Bawolff (Brian Wolff) 2011-01-16 22:26:47 UTC
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.
Comment 2 Chad H. 2011-01-17 14:13:30 UTC
Not an issue in resource loader or Vector.

This has been cluttering my debug output since time immemorial.
Comment 3 Bawolff (Brian Wolff) 2011-01-17 14:54:26 UTC
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?
Comment 4 Chad H. 2011-01-17 14:58:23 UTC
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.
Comment 5 Roan Kattouw 2011-01-21 03:28:35 UTC
Moving to General/Unknown, this problem plagues all skins because none of them are in the autoloader.
Comment 6 Mark A. Hershberger 2011-01-28 00:37:31 UTC
Changed the description
Comment 7 Ed 2011-11-01 07:29:40 UTC
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]) ) {
Comment 8 Ed 2011-11-01 08:05:55 UTC
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.
Comment 9 Daniel Friesen 2011-11-01 08:40:45 UTC
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.
Comment 10 Ed 2011-11-01 08:43:02 UTC
This is why I thought I shouldn't be trying to contribute here.  I'll leave bugs be.
Comment 11 Sam Reed (reedy) 2011-11-01 08:47:23 UTC
Comment on attachment 9339 [details]
Proposed Patch

Marking obsolete for that reason
Comment 12 Mark A. Hershberger 2011-11-01 19:41:47 UTC
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.
Comment 13 Krinkle 2014-04-29 16:12:07 UTC
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
Comment 14 Krinkle 2014-04-29 16:13:15 UTC
Looks like it's not there anymore.

Note You need to log in before you can comment on or make changes to this bug.


Navigation
Links