The intended deprecation mechanism was not approved because it duplicated (multiplicated, rather) code. I have now implemented a simpler patch, that just reverts the class members from protected/private to public, but -- keeping the spirit of the patch it fixes -- declaring them public explicitly. Properly deprecating the class variables will have to wait until we can use traits (PHP 5.4), as that seems to be the consensus of how it should be done.
I would appreciate if somebody could finally merge https://gerrit.wikimedia.org/r/#/c/151370/.
Stephan
On 2014-08-23 00:33, Stephan Gambke wrote:
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+bran... [1] https://gerrit.wikimedia.org/r/#/c/151370/ [2] http://php.net/manual/en/language.oop5.overloading.php#language.oop5.overloa... [3] https://www.mediawiki.org/wiki/Deprecation [4] https://www.mediawiki.org/wiki/Special:Code/MediaWiki/85288#c17504