Improve the Search API admin UI for adding/editing fields

Just WIP, haven't started nesting... will I am sure need test updates. But in case anyone has any early feedback:

Before:

After:

Note last commit fixes https://www.drupal.org/project/search_api/issues/3022881#comment-15842781 🐛 Clicking on + icon makes the page scroll to the top Fixed

This is now ready for review:

1. Screencast below
2. Explanations that aren't suitable as comments in the code are added as comments in the MR

This should bring it much more in line with Drupal and automatically inherit theming from Gin/Claro/etc given it uses nothing bespoke to search api any more.

Referencing issues this affects around Search API & AI modules (which has AI Search using this) becoming part of Drupal CMS.

I haven't looked at the code yet, but the screencast here looks super impressive, I think this is a huge upgrade.

Hi @scott_euser, I’ve applied MR !182 as a patch.

The UI changes are reflecting perfectly as expected in the issue description; I’m attaching screenshots for both the Gin and Claro themes.

@borisson_ I’ve also reviewed the code changes. Here are some key points regarding the MR:

1. This MR addresses the following issue 🐛 Clicking on + icon makes the page scroll to the top Fixed and resolved successfully.
2. Previously, fields with only one nested item appeared as nested fields. However, due to logic implemented in the following
   lines, these are no longer treated as nested fields. @scott_euser, you mentioned the reasoning in the doc comment, so I’ve highlighted it here for quick reference.

I’m moving this to RTBC. Thanks!

I've now read the code, and tested it locally as well. I have one question, but I think it looks ok so leaving at rtbc.

I think it would be a good idea to add this to the next release as well, so that hopefully drupal cms will start with this new more better user experience.

drunken monkey → made their first commit to this issue’s fork.

Thanks a lot, this indeed looks great!

I’ve added some comments and changes to the MR, please test/review. (I didn’t get round to actually testing the functionality, will do that tomorrow. Sorry if I broke anything.)

Thanks!

• I ran through the process again after your changes, all works fine still, attempted nested fields, rendered items, custom values, base fields all fine.
• I checked Search API SOLR and the method it extends is untouched (just buildForm) but good spot with the method name! I'll create a follow-up to make it easier for SOLR to avoid having to do that in the future
• I also checked the two elasticsearch modules + the typesense one; only typesense does a form alter, but only to add a new separate warning element and so is unaffected
• Then in AI Search (submodule of AI) we extend a fair bit but just calling parent buildForm, and not changing the add field process + anyways I maintain that so if there is anything eventually I can adapt

So looks good to me, thank you! Also thank you @rajdip_755 and @borisson_ for reviewing.

I checked Search API SOLR and the method it extends is untouched (just buildForm) but good spot with the method name! I'll create a follow-up to make it easier for SOLR to avoid having to do that in the future

Actually follow-up not needed here, SOLR can be refactored to not need to extend the IndexAddFieldsForm because its in control of the entity->getDataSources() itself which is the only thing it changes in the one method it overrides as far as I can see. Anyways unaffected either way now that you have reverted the method name change.

Looks awesome! Only thing I wonder if we should mark the template deprecated [#3385127] ?

No contrib modules seem to alter the template, but some custom projects might.

I now tested the UI and it indeed looks great, thanks again!

I did find one bug, however:

As you can see, it is now not possible anymore to add nested fields of linked taxonomy terms (or other entity reference fields). I already wondered about the count($nested_properties) > 1 condition when going through the code changes, seems that’s not entirely correct. I assume this is used for cases where a complex data type just has a single, unexpandable main property which can just be indexed by adding the complex property itself to the index. However, you do still have to check whether that single property has a complex data type itself.

Seems it would also be good to have a test for that. \Drupal\Tests\search_api\Functional\IntegrationTest already vists the “Add fields” page but then does nothing on it – maybe we can just change that to add some nested property to the index to ensure this works.

I also see that you removed the explicit sorting of the fields, those are now displayed in the order in which they are returned by the entity type manager. Could you elaborate on your reasoning for that change?
Above, you write, “Now that the order is fixed above by adding rows in the correct nested order in the first place, a sort is no longer needed”, but I don’t think that was the reason that the sort was placed there. Rather, we wanted to make it easier for people to find the properties they were looking for. While I concede that this new ordering also has a certain logic to it, I’m not sure it’s an improvement for users, and the probably closest analog in Core, Views, does just sort the fields alphabetically.
So, maybe we should keep the fields sorted alphabetically for now and, if you want, create a new issue for changing the order to the built-in one?

@sokru: That’s also a good point. Probably, it would be good practice to do it wth deprecation and change record, even though it is pretty unlikely that this will actually affect anyone (and even in that case it would just result in a useless template file lying around, no actual bugs or anything).

Thanks for the feedback!

1. count($nested_properties) > 1: Yes I can see this issue. I updated it to revert back to the status quo. I raised 🐛 Expanding some field types in the add index form result in nothing getting expanded Active as a follow-up as its a bug unrelated to the UI change as its also existing in search_api dev branch
2. Sorting: yes I can see this too, completely right! The old method does not work for table rows though since the nested properties are rows just like any other. So I updated it to sort the properties before creating the rows instead.
3. Added this for SOLR 📌 Prepare for updated add fields to index UI Active as per slack thread.
4. Updated test coverage to change for expand buttons (x2 levels) and visits those expanded levels.

On the deprecation, it seems you can only deprecate within hook_theme but search_api does not implement hook theme as its theming list_item which is a hook_theme coming from elsewhere, but as noted in #18 its probably never touched and should not be harmful since now we use elements that are heavily covered by admin themes. A release note is probably all we can do.

I will test the proposed patch.