Page MenuHomePhabricator
Create Task
Maniphest T12845

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

Description

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)

Revisions and Commits

rP Phabricator
D18166rPb46e2bb4cc6f Convert cluster/projects config options to newer modular structure
D18165rP6984d239b02b Convert Maniphest custom config to new config types
D18164rPa14b82d4f466 Move "wild" config types to new code
D18160rPec2af086254b Move 'set' config option type to new structure
D18159rP0afdabff00ae Convert 'class' config options to new validation
D18158rP72119e786c79 Convert "bool" config values to new modular system
D18157rP467be5e53fe6 Convert the "list<string>" and "list<regex>" Config option types
D18156rP9d30d49cfc73 Convert "enum" and "string" config options to new modular option types
D18155rP03b6bdde1931 Begin modularizing config options in a more modern way

Related Objects
Search...

Event Timeline

amckinley created this task.Jun 14 2017, 7:41 PM
amckinley mentioned this in T12846: arc unit uses the config of the current installation when running tests.Jun 14 2017, 7:51 PM
epriestley mentioned this in T12851: The feed query '2HfUlTnoqZbw' causes an exception in PhabricatorSearchCheckboxesField.Jun 19 2017, 5:04 PM
epriestley claimed this task.Jun 19 2017, 5:06 PM
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.

epriestley added a comment.Jun 23 2017, 4:48 PM

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

epriestley mentioned this in T12870: Configuration check for "maniphest.priorities" does not validate that keywords are unique across priorities.Jun 23 2017, 6:19 PM
epriestley added a subtask: T12870: Configuration check for "maniphest.priorities" does not validate that keywords are unique across priorities.Jun 26 2017, 2:04 PM
epriestley added a revision: D18155: Begin modularizing config options in a more modern way.Jun 26 2017, 3:52 PM
epriestley added a revision: D18156: Convert "enum" and "string" config options to new modular option types.Jun 26 2017, 4:08 PM
epriestley added a revision: D18157: Convert the "list<string>" and "list<regex>" Config option types.Jun 26 2017, 4:34 PM
epriestley added a revision: D18158: Convert "bool" config values to new modular system.Jun 26 2017, 4:57 PM
epriestley added a revision: D18159: Convert 'class' config options to new validation.Jun 26 2017, 5:15 PM
epriestley added a revision: D18160: Move 'set' config option type to new structure.Jun 26 2017, 5:27 PM
epriestley added a revision: D18164: Move "wild" config types to new code.Jun 27 2017, 1:59 PM
epriestley added a revision: D18165: Convert Maniphest custom config to new config types.Jun 27 2017, 3:15 PM
epriestley added a revision: D18166: Convert cluster/projects config options to newer modular structure.Jun 27 2017, 5:05 PM
epriestley removed epriestley as the assignee of this task.Jun 27 2017, 7:11 PM

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.
Herald added a subscriber: eadler. · View Herald TranscriptJun 27 2017, 7:12 PM
epriestley added a commit: rP03b6bdde1931: Begin modularizing config options in a more modern way.Jun 27 2017, 7:12 PM
epriestley added a commit: rP9d30d49cfc73: Convert "enum" and "string" config options to new modular option types.
epriestley added a commit: rP467be5e53fe6: Convert the "list<string>" and "list<regex>" Config option types.
epriestley added a commit: rP72119e786c79: Convert "bool" config values to new modular system.
epriestley added a commit: rP0afdabff00ae: Convert 'class' config options to new validation.
joshuaspence added a subscriber: joshuaspence.Jun 27 2017, 7:24 PM
epriestley added a commit: rPec2af086254b: Move 'set' config option type to new structure.Jun 27 2017, 7:34 PM
epriestley added a commit: rPa14b82d4f466: Move "wild" config types to new code.
epriestley closed subtask T12870: Configuration check for "maniphest.priorities" does not validate that keywords are unique across priorities as Resolved.
epriestley added a commit: rP6984d239b02b: Convert Maniphest custom config to new config types.
epriestley added a commit: rPb46e2bb4cc6f: Convert cluster/projects config options to newer modular structure.
epriestley added a comment.Feb 15 2018, 5:14 PM

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

Phabricator · Built by Phacility