Page MenuHomePhabricator

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

Description

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.

[0] https://github.com/wikimedia/mediawiki/commit/17d5e809d9e64edd1eeebcad018b068ca5b86e17
[1] https://github.com/SemanticMediaWiki/SemanticInterlanguageLinks/blob/1.3.0/src/Category/ByLanguageCategoryPage.php#L21-L24
[2] https://travis-ci.org/SemanticMediaWiki/SemanticInterlanguageLinks/jobs/258804178
[3] https://travis-ci.org/SemanticMediaWiki/SemanticInterlanguageLinks/jobs/258804176
[4] https://www.mediawiki.org/wiki/Deprecation

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

https://gerrit.wikimedia.org/r/368594

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

https://gerrit.wikimedia.org/r/368594

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.