Page MenuHomePhabricator

Rework MediaUploader config handling
Closed, ResolvedPublic

Description

When investigating T228771 and T228701 I've discovered a lot of issues with how UploadWizard handled its config. In short – it's a mess.

The default settings were defined in UploadWizard.config.php which besides returning the config array also does a bunch of static calls, relies on the global context and is generally awful. This means that any UW-related code needs the main request context, which in turn prohibits any kind of config reading from the load.php endpoint, which is a requirement to get T228701 to work. The config is then accessed through the UploadWizardConfig static class that has some logic for merging the default config with settings specified locally. Configs for campaigns are handled by UploadWizardCampaign that does retrieves the campaign config, merges it with the global config, parses some fields of and caches. The cache varies by language only, which breaks any {{GENDER: }} tags. It is also supposed to handle campaign start and end dates, but the feature is horribly buggy and doesn't work. The main "global" config is never parsed on its own, which is just... wat.

So, I attempted to resolve all that and make it all unit-testable. I ended up with an uhhh... slightly complicated setup:

  • ConfigBase is an abstract class representing any kind of MediaUploader config. It exposes getConfigArray and getSetting methods along with protected utilities for merging config arrays. All *Config classes inherit from it.
  • RawConfig represents the unparsed global config. This only relies on configuration settings and does not require a RequestContext to function. To slightly improve performance, the global raw config is also "cached" in a static variable.
  • RequestConfig represents the unparsed global config, but in the context of the request. This allows us to modify the config at this stage for the current language. This class is only intended to be used internally by GlobalParsedConfig and CampaignParsedConfig.
  • ParsedConfig is an abstract extension of ConfigBase. It represents a parsed config, global or for a campaign. It provides its children with some convenience methods for caching parsed configs.
  • GlobalParsedConfig represents the global parsed config. It automatically handles config parsing and caching, varying by language and the user's grammatical gender. The cache is invalidated on the basis of the hashed value of the raw config.
  • CampaignParsedConfig represents a campaign's parsed config. It similarly handles parsing and caching. The cache is invalidated on the basis of the last campaign revision timestamp.

Additionally:

  • ConfigFactory constructs the GlobalParsedConfig and CampaignParsedConfig.
  • ConfigParser parses configuration arrays.
  • ConfigParserFactory constructs the above.

That's 9 classes in total, whoo. I've got this more-or-less implemented and mostly covered by unit tests. I've decided to do the refactor as one huge commit, as MediaUploader is still unreleased anyway and untangling this one class at a time would take significantly more effort. The only thing left now is introducing the new classes to the rest of the code and testing.

Revisions and Commits

Event Timeline