Page MenuHomePhabricator

Push more functionality out of SiteConfig subclasses into the root SiteConfig class
Closed, ResolvedPublic

Description

During porting, we are noticing that many a time, a subset of these things happen (a) implement similar-looking functionality in all subclasses (b) we sometimes forget to implement the feature in extension/src/* (c) parser test and parse.php both test slightly different versions of SiteConfig and neither of which test extension/src/* which is the production version.

Of all these, (c) is perhaps the most concerning. While unit tests and roundtrip tests help us with this testing of the production version, we should nevertheless investigate to what degree it is possible to push the abstraction and move more functionality out to the parent class by relying on the DataAccess class to capture the variability. This will require some mapping objects to hide the variations, but could overall improve the testability as well keep the versions more in sync.

I don't expect substantial changes, but more tweaks to the SiteConfig interface to add more base class methods that rely on additional DataAccess methods to hide the variations.

This is not something for now, but potentially a post-port consideration.

Event Timeline

ssastry triaged this task as Medium priority.Aug 29 2019, 3:28 PM
ssastry moved this task from Backlog to Post-Port Work on the Parsoid-PHP board.

Change 531520 had a related patch set uploaded (by Arlolra; owner: Arlolra):
[mediawiki/services/parsoid@master] Fix special page name with default local name

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

Change 531520 merged by jenkins-bot:
[mediawiki/services/parsoid@master] Fix special page name with default local name

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

Dinoguy1000 renamed this task from Push more functionality out of SiteConfig sublasses into the root SiteConfig class to Push more functionality out of SiteConfig subclasses into the root SiteConfig class.Aug 29 2019, 10:35 PM

Change 538112 had a related patch set uploaded (by C. Scott Ananian; owner: C. Scott Ananian):
[mediawiki/services/parsoid@master] WIP: refactor solTransparentWikitext{,NoWs}Regexp() into base class

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

Change 538112 merged by jenkins-bot:
[mediawiki/services/parsoid@master] Refactor solTransparentWikitext{,NoWs}Regexp() into base class

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

Ideally I'd like a parserTests mode that uses the integrated SiteConfig, so that it was better tested. I think I've even file a phab task for that somewhere...

Change 585015 had a related patch set uploaded (by Arlolra; owner: Arlolra):
[mediawiki/services/parsoid@master] Add fix for interwiki hrefs missing a $1 placeholder to extension/

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

Change 585015 merged by jenkins-bot:
[mediawiki/services/parsoid@master] Add fix for interwiki hrefs missing a $1 placeholder to extension/

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

Change 593036 had a related patch set uploaded (by Subramanya Sastry; owner: Subramanya Sastry):
[mediawiki/services/parsoid@master] SiteConfig: DRY out protocol checking methods across subclasses

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

Change 593043 had a related patch set uploaded (by Subramanya Sastry; owner: Subramanya Sastry):
[mediawiki/services/parsoid@master] SiteConfig: DRY out computation of extResourceURLPatternMatcher

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

Change 593061 had a related patch set uploaded (by Subramanya Sastry; owner: Subramanya Sastry):
[mediawiki/services/parsoid@master] SiteConfig: DRY out magicword configurations

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

Change 593036 merged by jenkins-bot:
[mediawiki/services/parsoid@master] SiteConfig: DRY out protocol checking methods across subclasses

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

Change 593043 merged by jenkins-bot:
[mediawiki/services/parsoid@master] SiteConfig: DRY out computation of extResourceURLPatternMatcher

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

Change 593239 had a related patch set uploaded (by Subramanya Sastry; owner: Subramanya Sastry):
[mediawiki/services/parsoid@master] SiteConfig: DRY out computation of linkTrailRegex across subclasses

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

Change 593061 merged by jenkins-bot:
[mediawiki/services/parsoid@master] SiteConfig: DRY out computation of magicword config across subclasses

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

Change 593239 merged by jenkins-bot:
[mediawiki/services/parsoid@master] SiteConfig: DRY out computation of linkTrailRegex across subclasses

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

Change 593254 had a related patch set uploaded (by Subramanya Sastry; owner: Subramanya Sastry):
[mediawiki/services/parsoid@master] SiteConfig: DRY out language variant configuration code

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

I don't see any other clean opportunities for additional cleanup. But, leaving this open for someone else to audit and confirm.

Change 593254 abandoned by Subramanya Sastry:
SiteConfig: DRY out language variant configuration code

Reason:
Not worth it.

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

ssastry closed this task as Resolved.EditedMay 1 2020, 10:14 PM
ssastry claimed this task.
ssastry added a subscriber: Arlolra.

@cscott and @Arlolra: Feel free to audit SiteConfig code and submit any relevant patches. I think the original intent of this patch has been met.

Change 594256 had a related patch set uploaded (by Subramanya Sastry; owner: Subramanya Sastry):
[mediawiki/vendor@master] Bump Parsoid to 0.12.0-a12

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

Change 594256 merged by jenkins-bot:
[mediawiki/vendor@master] Bump Parsoid to 0.12.0-a12

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