r85296 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r85295‎ | r85296 | r85297 >
Date:23:32, 3 April 2011
Author:dantman
Status:resolved (Comments)
Tags:
Comment:
Add a getSkin that returns a dummy linker for BC with extensions still abusing it to access the linker.
Modified paths:
  • /trunk/phase3/includes/parser/ParserOptions.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/parser/ParserOptions.php
——@@ -90,6 +90,16 @@
9191 function getUser() { return $this->mUser; }
9292 function getPreSaveTransform() { return $this->mPreSaveTransform; }
9393
 94+ /**
 95+ * @param $title Title
 96+ * @return Skin
 97+ * @deprecated Use Linker::* instead
 98+ */
 99+ function getSkin( $title = null ) {
 100+ wfDeprecated( __METHOD__ );
 101+ return new DummyLinker;
 102+ }
 103+
94104 function getDateFormat() {
95105 $this->optionUsed('dateformat');
96106 if ( !isset( $this->mDateFormat ) ) {

Follow-up revisions

RevisionCommit summaryAuthorDate
r86511Changes for deprecated ParserOptions::getSkin() from r85296 and keeping backw...siebrand17:37, 20 April 2011
r90987Per siebrand on r85296, the proper path to deprecation is:...demon18:20, 28 June 2011

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r85247Followup r85244; Define all methods as static, implement a DummyLinker to for...dantman12:04, 3 April 2011

Comments

#Comment by Happy-melon (talk | contribs)   23:34, 3 April 2011

This probably supercedes r85257?

#Comment by Dantman (talk | contribs)   23:36, 3 April 2011

No... that's the right way to do it...... ;) I just inadvertently forced you to fix it now instead of letting the wfDeprecated calls prod you.

#Comment by Nikerabbit (talk | contribs)   07:18, 4 April 2011

You sure didn't give a lot of time to fix this...

#Comment by Siebrand (talk | contribs)   07:22, 4 April 2011

Isn't it good practice to support this without warnings for at least one release (1.18), have it whine the next release (1.19), and remove it in the 3rd (1.20)?

#Comment by 😂 (talk | contribs)   18:20, 28 June 2011

I agree, and I've fixed REL1_18 in r90987.

#Comment by Tbleher (talk | contribs)   06:10, 5 April 2011

Surely getSkin() should return a skin object rather than a Linker object?

I use an old version of SMW on my wiki[1] that does 

$parser->getOptions()->getSkin()->makeSpecialUrl()

This is broken now.

[1]: I know I should upgrade, but my version contains some custom code that will have to be forward ported.

#Comment by Dantman (talk | contribs)   06:19, 5 April 2011

Why do we have to support old versions of extensions in new versions of core?

#Comment by Tbleher (talk | contribs)   06:23, 5 April 2011

We don't have to, but if we don't, why add a b/c function at all?

At the very least, the function is very confusing, because both name and documentation imply that a Skin object is returned, but the function in fact returns a linker object.

#Comment by Siebrand (talk | contribs)   08:07, 5 April 2011

This is not a reason for marking an issue as FIXME. If the change broke the trunk version of an extension, it might be.

Leaving this at FIXME because of #c15686.

#Comment by Tbleher (talk | contribs)   08:50, 16 April 2011

Sorry, I think my messages were worded poorly. To clarify: the main "fixme" issue I see here is that the function added doesn't do what it says, because it doesn't actually return a Skin object.

Either the function should be changed to return a Skin object, or it should be removed. The second option would at least generate a clear error message for any code calling this function (I had to stare at the SMW code for a while until I understood that it broke because getSkin() did no longer return a Skin object)

I should also add that I appreciate Dantmans work to make the code clearer and refactor it.

#Comment by Happy-melon (talk | contribs)   09:49, 16 April 2011

You can't rename it, that makes no sense at all. It's the existence of the function ParserOptions::getSkin() which is needed for B/C. Equally it can't return a Skin object because Skin no longer has all of the functions which legacy code might call, making it useless for B/C.

Status & tagging log