Last modified: 2006-09-25 02:47:09 UTC
Several functions in the Skin class are marked with /*static*/. These functions, however, are not actually static, and some of them will even result in errors if actually called statically.
Created attachment 2368 [details] patch Simple fix.
Changing summary -- in addition to the functions in the Skin class marked /*static*/, _all_ of Linker's methods can and should be static, as they are all utility methods and should not need an instance of Linker/Skin to be called.
Created attachment 2371 [details] patch Patch for both Skin and Linker. Makes /*static*/ Skin methods static and makes all of Linker's methods static.
Well, there is a difference. In Skin, they are already static, and are just not marked as such, and are called statically from most places (should be fixed in some places). In Linker, they are *not* static, and are never called statically. I agree that the Linker methods should be static, and we should not use the Skin object for them, but it's another bug, and is a bit harder to fix.
(In reply to comment #4) > Well, there is a difference. In Skin, they are already static, and are just not > marked as such, and are called statically from most places (should be fixed in > some places). Which functions are being talked about? Skin::makeNSUrl, for example, currently refers to $this-> (instead of self::) and will, I think, result in an error if called statically. > I agree that the Linker methods should be static, and we should not use the Skin > object for them, but it's another bug, and is a bit harder to fix. Yeah. My last patch could probably be a start, because it doesn't break any behavior, as far as I can tell. $wgUser->getSkin()->[Linker function] will still work.
(In reply to comment #5) > (In reply to comment #4) > > Well, there is a difference. In Skin, they are already static, and are just not > > marked as such, and are called statically from most places (should be fixed in > > some places). > > Which functions are being talked about? Skin::makeNSUrl, for example, currently > refers to $this-> (instead of self::) and will, I think, result in an error if > called statically. > Calling a static function as $foo->bar() will not result an error, but a notice. The ability to define functions as "static" is new in PHP 5, and before that you could call *any* function as both static (Foo::bar) and non-static ($foo->bar()). Now you should define the static functions, but because they are still not defined as static and it is not *required* to call them as static, people sometimes call them in the non-static form ($foo->bar()), and these places should be fixed to Foo::bar. I also think that we have to set them to static, but there are some places where you didn't change $foo->bar() to Foo::bar in your patch. I will fix these calls and set the functions as static as early as I have some time. > > I agree that the Linker methods should be static, and we should not use the Skin > > object for them, but it's another bug, and is a bit harder to fix. > > Yeah. My last patch could probably be a start, because it doesn't break any > behavior, as far as I can tell. $wgUser->getSkin()->[Linker function] will still > work. Calling static functions as $foo->bar() may work now, but it's not right and may not work in future PHP versions. If you want to set the Linker functions as static, you (or someone else) have to change all the calls for them.
Applied the first patch of the patch to r16601, along with changes in all the references to the functions. Please open another bug for the Linker functions.
(In reply to comment #7) > Applied the first patch of the patch to r16601, along with changes in all the > references to the functions. Thanks. > Please open another bug for the Linker functions. bug 7405
