This doesn't break anything though, right? It's just a nice benefit to extend it to customization? Or does this path make customization not possible?

This doesn't break anything, but I don't want to take a step backwards here by hard-coding this behavior. If you don't want to fully integrate this change, I can commandeer it and integrate it when I have time.

oh, i see what you're saying - add ours to iconset too

Specifically, we currently specify the default icons like this, in PhabricatorProjectIconSet:

array(
  'key' => 'tag',
  'icon' => 'fa-tags',
  'name' => pht('Tag'),
),

...and a user might specify an icon in JSON like this:

{
  "key": "tag",
  "icon": "fa-tags",
  "name": "Tag"
}

We should add a key like "image" to the defaults, and allow users to specify an "image" as well. For example, this would let an install rename the "Tag" icon to "Label" with internal key "label" if they think that word is more clear (or "Label, Eh?" if they primarily speak Canadian) , but keep the actual FontAwesome icon () and new illustrated image, since both are appropriate for an icon called "Label".

Ideally, we should let users pick any image and not expose file path internals to the user. For example, if users specify "/v3/" paths and we later replace these with "/v4/", the mapping will break. We don't have a great way to do (or, generally, to formalize builtins and make them extensible) right now so this is likely to require migrations and guidance eventually, but I suppose we can cross that bridge when we come to it.

I think I got everything covered here, other than adding a "uiexamples" page of the built-in images. I can build that out later once I clean up that folder, also having another 8 icons produced currently.

Yeah, although you'll need a parameter or something if you want to restrict selections to just intended project icons (but maybe you don't).

I think this is all square but feels a little fragile.

I think this might break with these values:

'image' => ''
'image' => '0'

It looks like they'll evade the setup check, but fail to fall back to the default icon.

Doing this purely in a setup check (instead of also when accessing the image) is a tiny bit flimsy: it's possible for configuration values to change without the configuration setup checks running on them. Two examples are: an administrator editing conf/local/local.json, and a dynamic check (like a conf.php file or a SiteSource) making a runtime decision to produce a different value. We don't perform config setup checks every time a value is read, for performance reasons, and we can't detect when a value changes if it's changed outside of our control (editing a file, dynamic source). We could attempt to detect these changes by hashing config, but there'd be some level of performance/correctness tradeoff.

As written, there's currently no way to do anything dangerous with this minor flimsiness, since we always actually load the file through loadBuiltin() now, which does a check that is slightly too loose but would still catch anything dangerous. You can also only change config outside of our control with root access.

We could clean this up in the future, e.g. when UIExamples gets extended or when we run into a use case for third-party builtin files (it would be nice to let extensions add more project icons today if then want, for example, but they can't do it without us making some upstream changes).