Add a Views-like UI for adding fields

Gonna work on this now, in conjunction with #2044287: Introduce field aliases. This is definitely gonna be a major change, so I guess better sooner than later.

OK, I've finally got a working version! Please see the attached small patch. (What's 300 kB in our age of terabyte HDDs?)

To summarize the more substantial changes:

• The "Fields" tab only lists the indexed fields now, there's a separate "Add fields" page (linked as a local action) to add fields (nested or not).
• Therefore, the whole concept of "Additional fields" is dead and buried.
• It's also possible to add the same property twice (e.g., to index it with different data types – if still necessary, even though fulltext fields can be sorted).
• This necessitated, of course, that field identifiers become decoupled from their actual property path (and datasource). They are now more like Views' field aliases – including that they'll always just contain "nice" characters (alphanumerics and underscores), hopefully allowing Solr to return to readable field names.
• On the storage side, since the field IDs don't identify the property anymore, all of that information is now stored in the index itself: each entry in $index->options['fields'] can now contain label, datasource_id, property_path, type, boost, indexed_locked, type_locked and hidden, and must contain the first four (except datasource_id for datasource-independent (processor-added) properties).
• Storing the label there means it can now be changed by the user, which makes sense since otherwise adding the same field twice would unavoidably result in the same label.
• Speaking of processors adding fields: That is now also done a bit differently. PropertyInterface is gone – since properties don't have a 1:[0-1] relationship to fields anymore marking them as "locked" or "hidden" makes even less sense now than it did before. Processors just add normal data definitions now and have a new preIndexSave() method now for forcing fields on an index.
• However, I did not change that for all processors, just those with configuration or another effect than just adding a field – the others, I just marked locked and hidden, so their property will just automatically pop up in the "Add fields" form. (We might be able to get rid of the small performance overhead in a later step, see #2575003: Make adding new properties/fields more a first-class operation of processors.
• Despite all these changes, a lot about how you handle fields has actually stayed the same, though. So you can still just use $index->getFields() and do most of the same stuff as before. However, if you use Utility:[create|split]CombinedId() with fields you are probably doing something wrong. (On the other hand, some of the code actually still does it, since it's a pretty handy way of getting a single string containing all the information about where to get a property from. I call it "combined property path" now – quite a mouthful, though.)

OK, that all is done, and even all of the tests are passing locally.

However, there's still a lot of TODOs here, unfortunately – but I think we should move most of those into follow-up issues to have the basic new API ASAP and don't develop against the old one unnecessarily long.
Remaining TODOs are:

1. I think saving the index each time a field is added in the GUI (even though the user might want to change its type, or machine name), thus marking the index for reindexing, is pretty wasteful and unnecessary. Therefore, I actually copied all that fancy code from Views that makes it have an unsaved view configuration in parallel with the active one, which you can then edit to your heart's content without affecting the actual view, and later save permanently or discard. Would be quite a good fit, I'd say.
   Only problem: it's not working. Like, not at all. And I don't have any idea why. However, since it's not at all critical I'd just leave it there for now and fix it in a follow-up (it's only in the UI, so it's not even really a beta blocker).
2. Speaking of things stolen from Views but not yet working: the end goal is that the "Add fields" page would actually just be a popup dialog, like in Views, work with AJAX magic (the "Remove" links, too, of course) and everything looking about ten times nicer than in the current so-called (not by myself) "Thomas UI". However, again I have prioritized on getting the basic functionality done, with this part I haven't even started yet. (And, of course, someone with more UI/UX prowess than me should probably take care of improving the "Add fields" page/popup.)
3. I said that labels and field IDs are now actually editable: they are, internally, but they aren't in the UI. Shouldn't be too hard to change, I imagine, though editing field IDs is of course a bit tricky (especially if you want to be clever about it in the database backend). Probably having #2317771-5: Move processor and/or field settings to top-level properties on the index? at that point would help, if we just add a getOriginalId() method to easily spot a change.
4. Currently, since we call the processors one by one to add fields, or mark them locked, etc., each of these changes will actually invalidate the index's caches (static and stored), which is of course a performance nightmare (even though it only happens when saving an index, so luckily not very often). #2317771-5: Move processor and/or field settings to top-level properties on the index? and/or #2638116: Clean up caching of Index class method results (especially fields) might already solve this, though.
5. I think we should really discuss whether we need the "Item language" field to always be present. Does that really get anyone anything? We might even want to drop it completely, since the "Aggregated fields" processor can do this already (I think), but in any case having it always there even on single-language sites seems unnecessary.
   Why am I bringing this up? Because even now, with this patch, the field's ID can be changed, so no-one can actually rely on a field with that field ID being present anyways. Especially, it makes the "Content access" processor potentially dangerous, since it now has no reliable way anymore to force the query to not return any results.
   Long story short: we should add something like a $query->dontReturnAnyResultsFromThis() method. (There's something like it already in the Views query plugin, but it would make sense outside of Views, too.)

OK, that's about it.

Once everyone (including the test bot) is happy and we have decided which of these should be done in follow-up issues (and fixed the others, of course), I'll commit here and create said follow-ups. (And, in doing the former, ruin everyone's current setup. Hohoho.)

Thanks for your review and comments, really awesome!

I think it'd make sense to move these methods next to each other, the UpdateFieldsIndex method in between confused me at first. Very, VERY minor though. ;)

OK, let's do that. updateFieldsIndex was in between there previously because it's only ever used in getCache(), but I guess it still makes more sense to have those other methods grouped.

Should we note that this method is used recursively when the $fields variable contains an array?

We already do.

Can we reword this comment? We're touching it anyway and I dislike the "Obviously" in there because at first it wasn't very obvious for me.

You're right, sorry, that does sound a bit conceited. I should be more careful about such wording.

if this is so unlikely to happen, why do we even catch it? In what scenario can this happen and shouldn't we just let the exception trough if it does? It's not like this will happen when running, only when setting up an index, right?

I think this can only happen if it couldn't acquire a lock for the index, which I don't think is likely to happen ever, and especially not when you're deleting it. And we catch the exception because leaving that data in the temp store, in the worst case scenario, doesn't really do any harm either, so I don't think it's worth failing over.
Also, since the interface doesn't declare any thrown exceptions, we are technically not even allowed to let an exception escape from this method.

This is wrong, it's not an IndexFieldsForm. I'm starting to get in the habit of just writing Constructs a new instance of the class. to prevent just those kinds of mistakes, this is usually forgotten during refactors as well.

You're right, the current system doesn't make much sense. However, I think this was once in the coding standards, that's why I still always add the line like that.
But I just checked and it seems this is gone again from the standard. Also, #2140961: Allow constructor methods to omit docblocks wants to completely drop the line, so it might not be an issue at all in the future.
For now, I just corrected the line, though, to stay consistent within the module.

DateFormatter is the only service that has this pattern, the other service getters just return $this->service. Is there a reason for that?

Ah, yes, thanks for catching that and all the other DI irregularities. I'm just used to the other DI pattern, using the setters in create(), so I slipped up a few times with this different system we use (for some reason – does anyone actually know?) for forms.

I think we can replace the username here with a call to $account->getDisplayName. That way we shouldn't need the renderer.

I don't think so, since that just returns the stored name (or "Anonymous"), while I'm pretty certain the user name needs to be themed if it will be displayed to the user.
But actually, I don't really have an idea myself, that's just one more field in Drupal where there's simply too many ways to do something to know which is the right one. I just copied all of that code from the Views UI, so I'm gonna assume it's more or less correct.

Does this serve the same purpose as the $indexId? Should we add the same comment about __sleep / __wakeup?

No, the datasource ID actually has a getter and setter, that's a normal property.

This should probably be fixed in this patch.

No, it shouldn't, it's completely unrelated and was broken before. The difference is just that we previously explicitly asserted that it's still broken, which is of course nonsensical.

The attached patch fixes those and all your other comments, and should also fix the test fails.

Created #2640982: Fix "unsaved changes" code in the new Fields UI as one of the follow-ups (since I was able to solve it mostly, but then got stuck on kind of a circular dependency).

Excellent, great to hear! Test bot is happy, too – so let's do this!
Committed.
Thanks again for your great reviewing work here!

I'll create the follow-ups now.