Page MenuHomePhabricator

Convert ApiPageSet to a service rather than a subclass of ApiBase
Open, Needs TriagePublic

Description

ApiPageSet uses a lot of different services, and does not work as its own api module, but rather is used to help other api modules. I propose that it be converted to a dedicated service where all dependencies can be injected.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript

@BPirkle can you clarify if Platform Engineering would support such a change? I'm happy to try and write the code, but would prefer to get signoff that it would be approved before investing the time

We discussed this briefly at the tech planning meeting.

My take is that ApiPageSet should indeed not be a subclass of ApiBase, since it's not an API module. It can't be a service object though, since it is stateful. It's basically a fancy container object that wraps database logic for looking up and normalizing titles.

My proposal is to create a factory service for ApiPageSets, and treat the ApiPageSet itself like (rather complex) a query builder / result set.

An own service for this seems overkill, because it is only usable in context of Api.

It also not belongs to the ModuleManager, because it is not a module

Maybe a factory function on ApiMain (ApiMain::createPageSet( ... )) could be used here, but ApiMain itself is not ready for injection (T265644)

I have moved some of the services into the constructor with https://gerrit.wikimedia.org/r/c/mediawiki/core/+/703064
HookRunner and DB is missing and still used from ApiBase

Needs fallback in the constructor for all ideas - but it seems ApiPageSet is not part of the stable interface!?

Change 703064 had a related patch set uploaded (by Umherirrender; author: Umherirrender):

[mediawiki/core@master] api: Get all services for ApiPageSet in constructor

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

Change 703064 merged by jenkins-bot:

[mediawiki/core@master] api: Get all services for ApiPageSet in constructor

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

Change 703064 merged by jenkins-bot:

[mediawiki/core@master] api: Get all services for ApiPageSet in constructor

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

The patch set does not make the injection, but that could be easier now.