Page MenuHomePhabricator

Convert remaining ancient configuration type classes to modern modular type classes
Open, LowPublic


The original bug has been fixed by modernizing infrastructure, but this modernization isn't yet complete.

Original Description

Steps to repro:

  1. Using the Config UI, create a JSON config for maniphest.priorities
  2. Save the config
  3. Using the UI, blow away that JSON string and save the form with an empty config
  4. Observe that the config is now set to null, which breaks lots of pages:

Screen Shot 2017-06-14 at 12.40.16 PM.png (136×298 px, 14 KB)

Event Timeline

epriestley added a subscriber: epriestley.

I think this is a general problem with all JSON config types. The code for this is a bit messy and mixes "builtin" and "custom" types, but should probably just be rebuilt to not have all that weird magic.

I'm going to modularize this, but not before the release cut.

This is "fixed" by the associated series of diffs, in the sense that the actual problem (being unable to delete web UI config) is fixed. This is remaining work:

First, convert the remaining older option types to modern types. They are:

  • custom:PhabricatorKeyringConfigOptionType
  • custom:PhabricatorConfigRegexOptionType
  • custom:PhabricatorCustomLogoConfigType
  • custom:PhabricatorCustomUIFooterConfigType
  • custom:PhabricatorCustomFieldConfigOptionType
  • custom:PolicyLockOptionType

I believe this list is exhaustive. Some of these are probably easy (Lock, Keyring, Footer, Regex). Some of these are probably quite a bit more tricky (FieldConfig, Logo) since they use custom controls.

Once they're all converted, remove isCustomType() and related methods, and all callsites. Then make newOptionType() throw if it fails to find an appropriate type object and delete the if ($type) { code at callsites to finish cleaning this up.

I'm not going to tackle these for now since there's no real priority on doing this work and it won't fix any bugs, although we'll be better positioned to make improvements to Config after this completes. If anyone else wants to grab it, D18165/D18166 are fairly reasonable templates for the easy cases, I think.

epriestley renamed this task from Setting and removing a config for maniphest.priorities results in a null config instead of reverting to the default to Convert remaining ancient configuration type classes to modern modular type classes.Jun 27 2017, 7:12 PM
epriestley triaged this task as Low priority.
epriestley updated the task description. (Show Details)
epriestley edited projects, added Config, Infrastructure; removed Bug Report.

See PHI372 -- setting user.custom-field-definitions (or, probably, any similar config) to a string will pass validation but then fatal in use.