Page MenuHomePhabricator

Begin modularizing config options in a more modern way
ClosedPublic

Authored by epriestley on Jun 26 2017, 3:52 PM.

Details

Summary

Ref T12845. Config options are "modular", but the modularity is very old, half-implemented, and doesn't use modern patterns.

Half the types are hard-coded, while half the types are semi-modular but in a weird hacky way where you prefix the type with custom:....

The actual API is also weird and requires types to return a lot of array($stuff, $thing, $other_thing, $more_stuff) sorts of tuples.

Instead:

  • Add a new replacement layer which uses modern modularity patterns and overrides the older stuff if available, so we can migrate things one at a time.
  • New layer uses a more modern API -- no return array($thing, $other_thing, ...), and more modern building blocks (like AphrontHTTPParameterType).
  • New layer allows custom types to be deleted, which will ultimately let us deal with T12845.

Then, convert the 'int' type to use the new layer.

Test Plan
  • Set, edited, tried-to-change-in-an-invalid-way, and deleted an 'int' option from the web UI.
  • Same from the CLI.
  • Edited config.json to have an invalid value, verified that the error was detected and config was repaired.

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Edited config.json to have an invalid value, verified that the error was detected and config was repaired.

This only happens if you go back to the scene of the crime and attempt to edit the config again after making invalid config.json changes, right?

This revision is now accepted and ready to land.Jun 26 2017, 4:33 PM

This only happens if you go back to the scene of the crime and attempt to edit the config again after making invalid config.json changes, right?

If you restart Apache (or whatever your webserver is) or visit the "Setup Issues" page explicitly, that will also trigger it.

We could do something like hash local config files and re-validate them if they change, but it doesn't seem to be causing too many issues today, since you have to go manually mess with the file or manually mess with the database to create a problem in most cases. Usually, users hit this because they have something which used to be valid in local.json but no longer is after an upgrade (since validation got stricter), but they restart Apache as part of the process so the setup checks catch the issue.

This revision was automatically updated to reflect the committed changes.