Page MenuHomePhabricator

Visibility change to CategoryPage::mCategoryViewerClass breaks extension
Closed, ResolvedPublic


The [0] change arbitrarily modified the visibility of the mCategoryViewerClass property which in turn breaks extensions [1, 2] that rely on the existing visibility.

As for comparison, REL1_29 passes [3] all unit and integration tests while the MW master branch [2] returns with:

PHP Fatal error:  Access level to SIL\Category\LanguageFilterCategoryPage::$mCategoryViewerClass must be public (as in class CategoryPage) in /home/travis/build/SemanticMediaWiki/mw/extensions/SemanticInterlanguageLinks/src/Category/LanguageFilterCategoryPage.php on line 18

The argument in T166483 is understood but changing the visibility is not a solution as one cannot catch the issue while trying to maintain compatibility [4] with REL1_27.

I'd rather see that a new public method is introduced (e.g.CategoryPage::getCategoryViewerClass) which would allow subsequent implementations to override the return value without the need to modify the CategoryPage::mCategoryViewerClass property.


Event Timeline

mwjames created this task.Jul 29 2017, 12:41 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJul 29 2017, 12:41 AM

@Legoktm It would be marvellous if could take a peek at this.

mwjames updated the task description. (Show Details)Jul 29 2017, 5:47 AM
mwjames updated the task description. (Show Details)Jul 29 2017, 6:01 AM
Legoktm added subscribers: matmarex, Louperivois.

Since the purpose of T166483: allow CategoryPageView to set $mCategoryViewerClass was to make it modifyable via a hook, we probably want a getter and a setter.

Change 368594 had a related patch set uploaded (by Louperivois; owner: Louperivois):
[mediawiki/core@master] Implement a getter/setter for mCategoryViewerClass

Change 368594 merged by jenkins-bot:
[mediawiki/core@master] Implement a getter/setter for mCategoryViewerClass

matmarex closed this task as Resolved.Aug 1 2017, 5:17 PM
matmarex assigned this task to Louperivois.
matmarex removed a project: Patch-For-Review.

Sorry about the issue and thanks for the bug report, @mwjames. And thank you for stepping up to fix it, @Louperivois.