AJAXify and generally improve the new Fields UI

Thanks a lot for your review! Some very good points in there, this will surely help a lot in making the page more usable.

* IIUC, we re trying to mimic something like core's "Add block" page, in that we can add items, you can add more than one of each 'type', and each item added has its own configuration/settings. If so, then I wonder if we should stick to more completely mimicking that workflow that is recognizable/known to users, vs. creating a new one. We would then not really have expandable items, since we would mimic the selector used in views, in blocks, etc. where we have things as categories and we can filter that way.

The problem I had to solve here is that we don't have a fixed list of possible properties, but people can, in theory, go arbitrarily deep for the properties they want to add. You could index a node author's taxonomy terms' parents' related nodes' comments' authors' roles, if you want to. So putting the available properties on one page just isn't possible – at least without some additional UI for choosing which properties should be expanded there, which we previously had. But I think the drill-down pattern makes more sense there, because otherwise you have to go between two different forms for getting to the properties you want. (Also, with too many properties enabled there, the list could still become too long on larger sites, maybe even crashing them.)

For the same reason, I couldn't use collapsed fieldsets – they also require the collapsed data to already be there. But I guess we can try to style the links to look like collapsed fieldsets – that should make it much clearer what this does.

Always shwoing the first level for each datasource makes sense in any case, of course, thanks!

* No indication of what I've already added.

That's an interesting one, didn't think of that.
As explained in the issue summary, though, the plan is to have this form as a modal popup on the "Fields" tab, so maybe that will already help somewhat. But we can then surely review whether an additional indicator for fields that were already added still makes sense. (Though Views, e.g., also doesn't have that.)

* Some items (like taxonomy terms) seem to have both a "+" and an "Add" button. Clicking on "+" does nothing.

That seems like a bug, then, and one I cannot reproduce. A taxonomy term reference field should expand to a single "Taxonomy term" property, which will in turn expand to the list of its properties. (That additional indirection is of course rather pointless, but dictateted by Drupal's inner structure. I plan to get rid of that eventually, too.

In any case, thanks again for your feedback!

Yes, that's become tricky, I fear. But I already had to solve it for our own integration test: see \Drupal\search_api\Tests\IntegrationTest::addField():

  protected function addField($datasource_id, $property_path, $label = NULL) {
    $path = $this->getIndexPath('fields/add');
    $url_options = array('query' => array('datasource' => $datasource_id));
    if ($this->getUrl() === $this->buildUrl($path, $url_options)) {
      $path = NULL;
    }

    // Unfortunately it doesn't seem possible to specify the clicked button by
    // anything other than label, so we have to pass it as extra POST data.
    $combined_property_path = Utility::createCombinedId($datasource_id, $property_path);
    $post = '&' . $this->serializePostValues(array($combined_property_path => $this->t('Add')));
    $this->drupalPostForm($path, array(), NULL, $url_options, array(), NULL, $post);
    if ($label) {
      $args['%label'] = $label;
      $this->assertRaw($this->t('Field %label was added to the index.', $args));
    }
  }

Thanks for the tip!
I think depending on it is out of the question, but it might provide hints how to best implement this ourselves.

I don't think we need that relationship in both directions.

Yes, that would be awesome!
Thanks a lot!

Sorry, work picked up faster then expected.

It always does …
But still pretty good response time – others have been known to sit on such unfinished patches for months. ;)

Gonna unassign for now, since juli is mostly holiday for me actually. Feel free to pick up, else i'll try to pick up further later.

OK, sure. We'll see whether someone else feels like it, otherwise it would of course be great if you could continue work on it at some point.
In any case, thanks a lot for your work so far!

To fix the problem of not seeing what fields are select I was thinking something in the line of a split screen popup where you can select the fields on the left and a list of selected fields on the right.

Doesn't Views already do something like that – listing the selected fields/filters/etc. once again at the bottom of the popup? (We'd just need to also add the previously added fields there.)

In any case, your suggestion sounds pretty good. Would be awesome if you could provide a patch for that!
(Probably we'll have to get rid of the "Add fields" action link anyways, since we might have a hard time AJAXifying that. In that case, having one link per datasource could be an improvement.)

I don't think you're working on this anymore? If so, I guess I'll have to take a shot.
Otherwise, please re-assign. I won't be able to start until next week anyways.

However, I did already manage to re-roll the patch from #17 and remove all the debugging code (at least as much as I could find). See attached patches.

I finally managed to find the time to tackle this and got it to work. Took a while, but it went remarkably smoothly, all in all. I ran into a few problems, some of which I couldn't resolve, but the main features all worked pretty much immediately.

The unresolved issues, input appreciated:

1. Can we make the "X" in the top-right corner of the modal refresh the page, too?
2. The AJAX detection code in IndexAddFieldsForm::addField() looks a bit hacky. Does anyone know a more elegant (ideally: the proper) way to detect whether we're inside an AJAX request? (Same for all other such conditions in the various form/controller classes.)
3. Originally, I had "nojs" and "ajax" paths for removing fields, too, but that didn't work properly with the CSRF tokens. Does anyone know a solution to that?
4. The expand/collapse links in the "Add fields" form has a similar problem: when I tried to switch to using a callback for only returning/replacing the affected part of the form (just the HTML for that property and its children), I had to switch the links to (appropriately styled) buttons, but then I couldn't get that to work in a no-JS browser. Probably would have worked in the end, but it seemed like too much trouble to fix something that was basically working anyways.

For the latter two, I'm also attaching the failed attempts, in case someone wants to take a look. But I'm pretty satisfied with this result even if we don't manage to resolve those last problems – it still works pretty well, and is a vast improvement over what we currently have.

In any case, please test and review, so we can finally check off this release blocker, too!

(Currently, the tests should already verify that this continues to work fine in a no-JS browser. Adding tests that also test the Javascript would of course be great, but that's probably a lot of effort, so I think we can skip that for now, too. Should be a follow-up, though.)

I think item 3 above is actually a Core bug: #2857412: AJAX links don't work with CSRF tokens.

This should fix the tests. Small oversight. (Although, if we end up with a different way of detecting AJAX vs. normal request, I guess the duplicated route doesn't really serve a purpose anyways.)

Thanks a lot for your review!

I also think we should change all the array calls that we touch here to short array syntax (now that we use those)

I thought about that, but decided against it. We'll just roll a single patch for all remaining places in the code during RC.

Why are we using FormattableMarkup here instead of $this->t that we use just a few lines above it? I also think that we shouldn't use the <h2> in the translated string, we should probably use #prefix and #suffix to add the header information.

It's formattable, not translatable. I hope that resolves all of these remarks.

Shouldn't this be !empty() instead?

I don't like using !empty() in cases where I don't need an isset(). Since that variable is always set, I prefer the plain if ($properties).

Let's add why we can ignore this in the comment here? Does this catch the $field->getDataDefinition call?

Yes, exactly. And since that shouldn't throw an exception for processor-added properties (unless the processor is removed), we can just ignore it. Added this to the comment.

The patch also adds an "empty" message for the datasource fields tables. (The issue title says to "generally improve the new Fields UI", after all.) I also wanted to add alphabetic sorting for fields (currently, the order is the one they were added in, which I don't think is very useful), but that would lead to messy diffs when re-saving the fields form the first time, so I guess we'd better wait for #2860167: Add "orderby" to our config schema definitions with that.

You're right, thanks!

Yeah, you're right, thanks! Speaking of which: opened #2865997: Fix behavior of the "Save and add fields" button as a follow-up.
Anyways, committed.
Thanks again to everyone involved!