Last modified: 2014-09-29 08:55:39 UTC
The following change made some member variables of the EditPage class protected: https://gerrit.wikimedia.org/r/#/c/133484/ The change seems to have been intended simply as a cleanup, but it broke compatibility with at least one extension, Semantic Forms. (See bug: https://bugzilla.wikimedia.org/show_bug.cgi?id=67522 ) I don't know whether or not any other extensions are affected. Extension compatibility might not be such a high priority, but given the nature of the commit (just a cleanup), I file this in case making EditPage::$mTokenOk public again is agreeable. (I also saw that some other visibility changes from the above commit, to another class, were adjusted when they caused another, more serious problem: https://bugzilla.wikimedia.org/show_bug.cgi?id=65665 ) If you want to keep EditPage::$mTokenOk private, an alternative proposal would be to add a method to return the $mTokenOk status. That would still require changing any code that now relies on reading $mTokenOk, but it would make for the cleanest change.
[CC'ing author and reviewer of the patch that introduced this problem]
Any preferences regarding how to solve it? Either of the two solutions I mentioned would make for a trivial patch. If there's no comment in a while, I'll probably just go ahead with submitting the first alternative for review, and then if that isn't accepted, I'll go for the second alternative.
To be honest I'd go for reverting that patch altogether. It broke the interface without properly deprecating the variables first disregarding all dependencies. http://www.mediawiki.org/wiki/Deprecation
Siebrand: ping. Do you plan to fix that patch? Or need the extensions code changes instead? Replying to comment 3 welcome.
Patch: https://gerrit.wikimedia.org/r/#/c/151370/
Change 151370 had a related patch set uploaded by Legoktm: Fix for Ia9baaf0b: Make previously public variables public again https://gerrit.wikimedia.org/r/151370
(In reply to Joel K. Pettersson from comment #2) > Any preferences regarding how to solve it? Either of the two solutions I > mentioned would make for a trivial patch. > > If there's no comment in a while, I'll probably just go ahead with > submitting the first alternative for review, and then if that isn't > accepted, I'll go for the second alternative. The visibility change is now reverted. Going forward we should probably investigate if the way SemanticForms is using EditPage is appropriate, as well as if a differing deprecation practice for member variables is appropriate (comment 3).
Change 151370 merged by jenkins-bot: Fix for Ia9baaf0b: Make previously public variables public again https://gerrit.wikimedia.org/r/151370
This bug should stay open, Ia9baaf0b is only one of many patches changing variable visibility. However given the time and effort this one took, I will not try to fix up any of the other patches. For a full list of candidates in need of fixing see here: https://gerrit.wikimedia.org/r/#/q/owner:siebrand%2540kitano.nl+topic:phpcs,n,z Siebrand claims he checked all the extensions on WMF's gerrit and I believe him. Only 1. he missed at least one, so maybe others and 2. there is more than 5000 extensions out there according to Wikiapiary, WMF's gerrit knows only 2242.
(In reply to s7eph4n from comment #9) > This bug should stay open Errm, you wrote that comment while closing this ticket. :) Or you could create a followup ticket with lower priority for remaining work.
Did I? Darn... Got confused with the SemanticForms specific bug, I guess.
