Last modified: 2011-03-13 18:05:20 UTC
Methods of the Linker class can and should be static, as Linker 1) is an utility class and 2) stores no state information. See comments for bug 7357.
Created attachment 2449 [details] patch for phase3 Makes all of the methods of the Linker class static and updates calls to those methods accordingly. In addition, I've tried to remove as many references to Skin, $skin, ->getSkin(), ->skin, and $sk (did I miss any?) as possible. These are the major changes: * removed $skin argument from ImageHistoryList constructor (ImagePage.php) * removed PageHistory::mSkin (PageHistory.php) * removed $sk argument of ucListEdit (SpecialContributions.php) * removed $sk argument of wfSpecialSpecialpages_gen (SpecialSpecialpages.php) * removed WhatLinksHerePage::skin (SpecialWhatlinkshere.php) * removed CategoryViewer::getSkin() (CategoryPage.php) * removed $skin argument from ChangesList/EnhancedChangesList/OldChangesList constructor (ChangesList.php) * removed $skin parameter to FetchChangesList hook (ChangesList.php) * removed ImageGallery::getSkin() and ImageGallery::useSkin() (ImageGallery.php) * made LogPage::actionText, LogPage::makeParamBlob, LogPage::extractParams static (LogPage.php) * LogPage::actionText takes parameter $forContent instead of $skin (only occurrences of $skin were for checking whether $skin was null) (LogPage.php) * removed LogViewer::skin (SpecialLog.php) * removed IndexPager::getSkin() (Pager.php) * removed ParserOptions::mSkin, ParserOptions::getSkin(), ParserOptions::setSkin() (Parser.php) * removed argument $skin from QueryPage::formatResult and from all subclasses (QueryPage.php and many special pages) * made Skin::drawCategoryBrowser static (was already being called statically) (Skin.php) * removed RevisionDeleteForm::skin (SpecialRevisiondelete.php) I checked /extensions for stuff that these changes might break. Another patch coming for that.
Created attachment 2450 [details] patch for extensions Suggested changes for extensions. Many of these (the ones for Semantic MediaWiki) are just a result of QueryPage::formatResult taking one less argument.
Make sure you haven't broken those extensions on older MW versions.
Created attachment 2528 [details] patch (phase3) Updated patch to latest revision.
Created attachment 2529 [details] patch (extensions) Patch for extensions that preserves backwards compatibility, I believe.
Created attachment 2650 [details] updated patch (extensions) Updated patch for extensions to current revision.
Created attachment 2651 [details] updated patch (phase3) Updated patch for phase3. Ugh, this patch is hard to maintain.
1) Please don't make unrelated changes to whitespace, comments, etc. when submitting a patch. It just makes it harder to review. 2) I don't think @static is necessary for functions marked as static in the PHP code. 3) How much have you tested this? I'd be willing to commit it if you've poked things and nothing's broken, after some testing by me.
(In reply to comment #8) > 1) Please don't make unrelated changes to whitespace, comments, etc. when > submitting a patch. It just makes it harder to review. Sorry. I thought that since I was changing so much of Linker.php anyways, I might as well. (But at this point, I hope it's all right if I leave them.) > 2) I don't think @static is necessary for functions marked as static in the PHP > code. OK...should I remove those from the patch? They should eventually be removed from the current MediaWiki code as well, then. > 3) How much have you tested this? I'd be willing to commit it if you've poked > things and nothing's broken, after some testing by me. My low-activity wiki runs. Editing/viewing pages and special pages seems OK. After all, all I've really done in the patch is refactoring. Thanks for your interest in this enhancement!
Patches applied by Nick Jenkins in me, r17479 and r17480.
Minor update applied in r17481 to prevent parserTests regressions.
Breaks HTML dump, reverting. Skins are polymorphic, you need a factory.
Summarising info from TimS on IRC in regards to his concerns about this: When the user runs "php maintenance/dumpHTML.php", it looks like the overridden makeBrokenLinkObj() function isn't called. The result is that broken links in the static dump will show up as red links, instead of plain text as per normal (i.e. there should be no red links). Would prefer to not use static functions, and would prefer to use a factory function for best flexibility. An example of this is getSkin() in includes/User.php.
So basically the answer to this is we want Linker methods to be overridable by skins, and specifically we want them overridden transparently to the caller, so that when some random code calls makeBrokenLinkObj() or anything else it calls it from Monobook or whatever instead of Linker, without the main code knowing or caring what class it's calling. Thus you cannot call the methods with anything like Linker::makeBrokenLinkObj(), since that won't be overridden. Thus we use non-static calls. Thus this is WONTFIX. Correct?
Yea..I thought this thing was closed a long time ago.
