Page MenuHomePhabricator

Member variable visibility changes broke compatibility for extensions
Open, NormalPublic

Description

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.


Version: 1.24rc
Severity: normal

Details

Reference
bz67984

Event Timeline

bzimport raised the priority of this task from to Normal.
bzimport set Reference to bz67984.
bzimport added a subscriber: Unknown Object (MLST).
JoelKP created this task.Jul 14 2014, 11:25 AM

[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.

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.

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptSep 23 2015, 10:24 PM