Re: [Wikitech-l] Using magic functions (e.g. __get) to deprecate class members

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 &lt;s7eph4n(a)gmail.com&gt; Reply: Wikimedia developers &lt;wikitech-l(a)lists.wikimedia.org&gt;&gt; Date: August 22, 2014 at 18:33:24 To: Wikimedia developers &lt;wikitech-l(a)lists.wikimedia.org&gt;&gt; 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