On 8/22/14, Stephan Gambke <[email protected]> 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+branch:master+topic:phpcs,n,z > [1] https://gerrit.wikimedia.org/r/#/c/151370/ > [2] > http://php.net/manual/en/language.oop5.overloading.php#language.oop5.overloading.members > [3] https://www.mediawiki.org/wiki/Deprecation > [4] https://www.mediawiki.org/wiki/Special:Code/MediaWiki/85288#c17504 > > _______________________________________________ > Wikitech-l mailing list > [email protected] > https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Well that's probably the first compelling use case I've heard for upping our version requirements to php 5.4. (yeah yeah, not going to happen until wmf cluster changes). Its not like SMW is the only thing that's been hit by changing member visibility, we've also got things like bug 65733 and db7be31e31987c6124 (albeit the second one was caught extremely quickly). It seems natural that we're not going to be able to catch every single possible use of such variables in existence no matter how hard we try. The most major objection I would have, is that the code would make all private/protected properties accessible, not just the recently deprecated. Otherwise the code kind of seems like a lesser evil. --bawolff _______________________________________________ Wikitech-l mailing list [email protected] https://lists.wikimedia.org/mailman/listinfo/wikitech-l
