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
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@gmail.com Reply: Wikimedia developers wikitech-l@lists.wikimedia.org> Date: August 22, 2014 at 18:33:24 To: Wikimedia developers wikitech-l@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+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
_______________________________________________ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
On 8/22/14, Tyler Romeo tylerromeo@gmail.com wrote:
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
$mFoo was a historic coding convention for all member variables (including public variables). There are many explicitly public variables starting with $m:
bawolff@Bawolff-L:/var/www/w/git/includes$ git grep 'public $m' |wc -l 266
I don't think we should treat $m variables any different from other variables.
--bawolff
On 8/22/14, Stephan Gambke s7eph4n@gmail.com 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
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org 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
On 23 August 2014 01:56, Brian Wolff bawolff@gmail.com wrote:
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.
I thought about introducing arrays of allowed variables for that case, but decided against it.
Currently there should be no code in existence that accesses previously private variables. If somebody now starts to access private variables through this mechanism that were not accessible before, it is their own fault. They would have to specifically find the __get/__set methods and decide to use them in spite of them being clearly marked as deprecated and in the full knowledge of them being removed in 12 months latest.
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
wikitech-l@lists.wikimedia.org