Page MenuHomePhabricator

Use dependency injection in the campaign system
Closed, ResolvedPublic

Description

This should be mostly doable, having the new config handling classes (T275989: Rework MediaUploader config handling).

Probably the main issue here will be campaign config validation which should be rewritten to not rely on merging the campaign config with the global config prior to validation.

Some loose notes:

  • CampaignContent will use a setServices public method for unit testing, due to the lack of a better alternative.
  • CampaignContentHandler can use ObjectFactory: T243560: Use ObjectFactory to construct ContentHandlers
  • DB code should be moved out to something like CampaignStore. This can also expose a custom SelectQueryBuilder, if there is a need for it.
  • Constructing future Campaign or CampaignRecord (?) objects worries me a bit. Currently UploadWizardCampaign when constructed will retrieve the config from CampaignContent which is then validated. This content validation is okay for things like loading the campaign for use in the uploader, but is absolutely not okay when called repeatedly on Special:Campaigns or in ApiQueryAllCampaigns. I see two possible solutions, for now I'd go with the first one:
    • Store the entire campaign config in the campaign DB table, as serialized JSON. This will help reduce the number of queries to the DB. We probably also would have to store some indication of whether the campaign is valid or not.
    • Or decouple CampaignRecord from CampaignContent entirely and force callers interested in the full content to construct the WikiPage themselves and then handle the content. This would need some changes to the DB table and the API, which would have the option to retrieve the full campaign config marked as expensive or something like that. Seems a bit more complicated.

Event Timeline

The docs don't mention it, but there actually is support for DI in ContentHandlers – T243560: Use ObjectFactory to construct ContentHandlers. This should come in handy for CampaignContentHandler.

Not sure about CampaignContent, though. I'll have to poke around the code and see what can we do with the constructor. It would be certainly nice if we could inject e.g. RawConfig for obtaining a list of available licenses that would be used for validation.

After a lot of head scratching, I've concluded that allowing returning campaign configs en masse via an API is a really bad idea and there is no reasonable way to make it fast. So, the API will be stripped down a bit so as to only use data from the campaigns DB table.

The actual config retrieval should be moved to a separate API endpoint anyway, see: T275988: Allow MediaUploader to be embedded in a dialog. This is not critical however, I think we can ship an MVP without any programmatic way to retrieve the config.

Special:Campaigns does not need the full config, only the title and description of the campaign – we can add these as additional fields in the DB table.

Change 700444 had a related patch set uploaded (by Ostrzyciel; author: Ostrzyciel):

[mediawiki/extensions/MediaUploader@master] WIP: Rewrite DB campaign storage

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

Change 700444 merged by jenkins-bot:

[mediawiki/extensions/MediaUploader@master] Rewrite DB campaign storage

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

Change 700983 had a related patch set uploaded (by Ostrzyciel; author: Ostrzyciel):

[mediawiki/extensions/MediaUploader@master] WIP: Add Title retrieval to CampaignSelectQueryBuilder

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

Change 700983 merged by jenkins-bot:

[mediawiki/extensions/MediaUploader@master] Add Title retrieval to CampaignSelectQueryBuilder

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

Change 720476 had a related patch set uploaded (by Ostrzyciel; author: Ostrzyciel):

[mediawiki/extensions/MediaUploader@master] Introduce CampaignStats class

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

Change 720476 merged by jenkins-bot:

[mediawiki/extensions/MediaUploader@master] Introduce CampaignStats class

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

Ostrzyciel claimed this task.