My opinion on this matter is that if you were using a variable prefixed with “m”, which is
clearly one of our conventions for declaring variables private, you are asking for
trouble. Just recently when the password hashing API patch was merged, it caused problems
with CentralAuth since it tried accessing mPassword directly.
In cases like this, I don’t think there’s any need for a deprecation period, because you
are playing with fire in the first place.
Obviously, for other variables that don’t start with “m” and are not documented as
@private or @protected, then we need the grace period.
--
Tyler Romeo
0x405D34A7C86B42DF
From: Stephan Gambke <s7eph4n(a)gmail.com>
Reply: Wikimedia developers <wikitech-l(a)lists.wikimedia.org>>
Date: August 22, 2014 at 18:33:24
To: Wikimedia developers <wikitech-l(a)lists.wikimedia.org>>
Subject: [Wikitech-l] Using magic functions (e.g. __get) to deprecate class members
TL;DR: Please review [1]
I was asked to discuss the topic on the mailing list, so here goes.
Since some time Siebrand is making an effort to improve code quality by
making it phpcs-strict compliant [0]. This involves explicitly declaring
the visibility of class members.
Alas, intended or not, in very many cases he did not only explicitly
declare the class variable's visibility, but he also changed it from the
implicit 'public' to explicit 'protected' or 'private', thus
introducing
a major API change without a proper deprecation period. Apparently this
was not noticed (or at least not challenged) by the reviewers. I checked
a few of his commits and they were all merged within hours.
Now, to be clear about that, I appreciate both changes - the phpcs
compliance as well as the more limited accessibility of class variables.
This was long overdue. However, to introduce something like this a
proper deprecation period is in my opinion essential, in particular
considering the extent of the changes. I do believe that Siebrand
checked that the variables he declared protected or private were not
used by extensions. However, he missed one (EditPage::mTokenOk) which
subsequently resulted in a bug in an extension (SemanticForms). Given
the number of the now newly hidden variables I am quite sure, that there
are other cases. If only because many extensions are not hosted on WMF
servers, so they can not be checked.
To keep the changes and still be able to properly deprecate the direct
access to the member variables I submitted a patch [1] that makes use of
PHP magic functions [2]. They provide access to the class members and
issue a deprecation warning. The intention is to keep these functions
for the custom two releases [3], i.e. until 1.26, and then remove them.
This patch was shot down for three reasons:
<quote>
* it duplicates code
* there is no tests
* our code base barely use the magic methods and I am pretty sure Tim
Starling commented a while back about them being nasty.
</quote>
When I pointed out, that these functions are not re-entrant and thus
Tims warning [4] did not apply the answer was "I have merely copy pasted
Tim comment without even attempting to understand what it means". I was
subsequently asked to present this to the mailing list which I do with
this mail.
I would appreciate comments on the patch (preferably constructive), that
would allow us to not revert Siebrand's changes and still properly
deprecate formerly public class members.
Thanks.
Stephan
[0]
https://gerrit.wikimedia.org/r/#/q/status:merged+project:mediawiki/core+bra…
[1]
https://gerrit.wikimedia.org/r/#/c/151370/
[2]
http://php.net/manual/en/language.oop5.overloading.php#language.oop5.overlo…
[3]
https://www.mediawiki.org/wiki/Deprecation
[4]
https://www.mediawiki.org/wiki/Special:Code/MediaWiki/85288#c17504
_______________________________________________
Wikitech-l mailing list
Wikitech-l(a)lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/wikitech-l