There are already several issues for improving the "Fields" tab (which has really horrible UX, of course), but I think there is now a consensus, more or less, to imitate Views' solution for this – only list the indexed fields and provide an "Add fields" button to add more. In a first step, this wouldn't even have to use a modal dialog, it could just be a normal form on a separate page, too. Changing it to a modal would just be polishing.

(I also don't know whether separate forms for our few settings for each field would make sense – probably not until we add more introspection to "Add fields" processors, and then this might still be incorporated into a single form much better.)

One thing that would be needed here, though, which probably isn't trivial, is a good way to add related fields. Probably some AJAX magic to load the related fields, up to arbitrary depths, on demand, also making the "additional fields" configuration for indexes unnecessary.

Estimated Value and Story Points

This issue was identified as a Beta Blocker for Drupal 8. We sat down and figured out the value proposition and amount of work (story points) for this issue.

Value and Story points are in the scale of fibonacci. Our minimum is 1, our maximum is 21. The higher, the more value or work a certain issue has.

Value : 13
Story Points: 21

Comments

drunken monkey created an issue. See original summary.

drunken monkey’s picture

Assigned: Unassigned » drunken monkey

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.

Nick_vh’s picture

Issue summary: View changes
Issue tags: +beta blocker
drunken monkey’s picture

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.)

Status: Needs review » Needs work

The last submitted patch, 4: 2574969-4--fields_aliases_and_ui.patch, failed testing.

The last submitted patch, 4: 2574969-4--fields_aliases_and_ui.patch, failed testing.

borisson_’s picture

I really like the Index::addField and Index::removeField methods, they make for an markable improvement in the rest of the code for readability. Very well done.

I also like that you added static caching in some places, ++.

In this patch, a couple of new exceptions are thrown, I would however prefer to have more specific exceptions - this can be a followup though.

I read trough the patch twice but I will probably have missed a couple of things, this patch is massive and some of the diffs are really hard to follow.

  1. +++ b/search_api_db/README.txt
    @@ -24,7 +24,7 @@ Supported optional features
    -  Allows you to create facetted searches for dynamically filtering search
    +  Allows you to create faceted searches for dynamically filtering search
    

    This should probably go into #2268867: Review README.txt files.

  2. +++ b/search_api_db/src/Tests/BackendTest.php
    @@ -464,20 +466,19 @@ class BackendTest extends EntityUnitTestBase {
    -    $keywords_field = $this->getFieldId('keywords');
    -    $results = $this->buildSearch()->addCondition($keywords_field, 'orange', '<>')->execute();
    +    $results = $this->buildSearch()->addCondition('keywords', 'orange', '<>')->execute();
    

    You remove the $keywords_field here and replace it with a hardcoded string trough the test.

  3. +++ b/search_api_db/src/Tests/BackendTest.php
    @@ -508,13 +509,13 @@ class BackendTest extends EntityUnitTestBase {
    -    $keywords_field = $this->getFieldId('keywords');
    +    $keywords_field = 'keywords';
    

    While here, you just change the value of $keywords_field. Can we do one or the other?

  4. +++ b/src/DataType/DataTypePluginBase.php
    index 8a07deb..8216ed4 100644
    --- a/src/Entity/Index.php
    
    --- a/src/Entity/Index.php
    +++ b/src/Entity/Index.php
    

    The diff for index is horrible, I'll probably miss half the changes in the red-green noise here :)

  5. +++ b/src/Entity/Index.php
    @@ -330,6 +278,9 @@ class Index extends ConfigEntityBase implements IndexInterface {
    +    if ($name == 'fields') {
    +      unset($this->cache);
    +    }
    

    what does this do? Can we add a comment?

  6. +++ b/src/Entity/Index.php
    @@ -1168,18 +917,93 @@ class Index extends ConfigEntityBase implements IndexInterface {
    +  protected function getCache($method, $static_only = TRUE) {
    ...
    +  protected function setCache($method, $value, $static_only = TRUE) {
    

    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. ;)

  7. +++ b/src/Entity/Index.php
    @@ -1168,18 +917,93 @@ class Index extends ConfigEntityBase implements IndexInterface {
    +   * This is important when loading fields from the cache, because their index
    +   * objects might point to another instance of this index.
    

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

  8. +++ b/src/Entity/Index.php
    @@ -1201,25 +1025,21 @@ class Index extends ConfigEntityBase implements IndexInterface {
    +    // Obviously, we should first check for locked processors, because their
    +    // preIndexSave() method should be called, too (if applicable).
    

    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.

    Is this better?

    If any of the configured processors implements the preIndexSave() method, that should be the first thing we call.<code>
    </li>
    
    <li>
    <code>
    +++ b/src/Entity/Index.php
    @@ -1469,6 +1274,18 @@ class Index extends ConfigEntityBase implements IndexInterface {
    + Catch (TempStoreException $ e) {
    +        // Can't really be helped, I guess. But is also very unlikely to happen.
    +        // Ignore it.
    +      }
    

    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?

  9. +++ b/src/Form/IndexAddFieldsForm.php
    @@ -0,0 +1,546 @@
    + * Provides a form for adding indexed fields to a search index.
    

    The fields are not indexed before being added so I think the comment should clearly reflect that.

    Provides a form for adding fields to a search index.

  10. +++ b/src/Form/IndexAddFieldsForm.php
    @@ -0,0 +1,546 @@
    +   * @var \Drupal\Core\Datetime\DateFormatter|null
    ...
    +  protected $dateFormatter;
    

    Dateformatter is the only service that can also be null.

  11. +++ b/src/Form/IndexAddFieldsForm.php
    @@ -0,0 +1,546 @@
    +   * Constructs an IndexFieldsForm object.
    

    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.

  12. +++ b/src/Form/IndexAddFieldsForm.php
    @@ -0,0 +1,546 @@
    +  public function setRenderer(RendererInterface $renderer) {
    +    $this->renderer = $renderer;
    +    return $this;
    +  }
    

    This setter is never used, let's get rid of it.

  13. +++ b/src/Form/IndexAddFieldsForm.php
    @@ -0,0 +1,546 @@
    +  public function getDateFormatter() {
    +    return $this->dateFormatter ?: \Drupal::service('date.formatter');
    +  }
    

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

  14. +++ b/src/Form/IndexAddFieldsForm.php
    @@ -0,0 +1,546 @@
    +  public function setDateFormatter(DateFormatter $date_formatter) {
    +    $this->dateFormatter = $date_formatter;
    +    return $this;
    +  }
    

    This setter is never used, let's get rid of it.

  15. +++ B / src / Form / IndexBreakLockForm.php
    @@ -0,0 +1,136 @@
    +   * @var \Drupal\Core\Entity\EntityManagerInterface
    ...
    + $ Entitymanager protected;
    ...
    +   * @param \Drupal\Core\Entity\EntityManagerInterface $entity_manager
    ...
    +  public function __construct(SharedTempStoreFactory $temp_store_factory, EntityManagerInterface $entity_manager, RendererInterface $renderer) {
    ...
    +    $this->entityManager = $entity_manager;
    ...
    +    /** @var \Drupal\Core\Entity\EntityManagerInterface $entity_manager */
    +    $entity_manager = $container->get('entity.manager');
    

    Can we use the entity_type.manager instead?

  16. +++ b/src/Form/IndexFieldsForm.php
    @@ -101,11 +145,102 @@ class IndexFieldsForm extends EntityForm {
    +  public function setRenderer(RendererInterface $renderer) {
    +    $this->renderer = $renderer;
    +    return $this;
    +  }
    

    This setter is never used, let's get rid of it.

  17. +++ b/src/Form/IndexBreakLockForm.php
    @@ -0,0 +1,136 @@
    +    $username = array(
    +      '#theme' => 'username',
    +      '#account' => $account,
    +    );
    +    return $this->t('By breaking this lock, any unsaved changes made by @user will be lost.', array('@user' => $this->renderer->render($username)));
    

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

  18. +++ b/src/Form/IndexFieldsForm.php
    @@ -101,11 +145,102 @@ class IndexFieldsForm extends EntityForm {
    +  public function setDateFormatter(DateFormatter $date_formatter) {
    +    $this->dateFormatter = $date_formatter;
    +    return $this;
    +  }
    

    This setter is never used, let's get rid of it.

  19. +++ b/src/Item/Field.php
    @@ -249,15 +494,10 @@ class Field implements \IteratorAggregate, FieldInterface {
    +    $out = $this->getLabel() . ' [' . $this->getFieldIdentifier() .
    +      ']: indexed as type ' . $this->getType();
    

    This looks weird and could use a reformatting.

  20. +++ b/src/Item/Field.php
    @@ -15,7 +18,87 @@ use Drupal\search_api\Utility;
    +   * This is only used to avoid serialization of the index in __sleep() and
    +   * __wakeup().
    

    Awesome comment.

  21. +++ b/src/Item/Field.php
    @@ -15,7 +18,87 @@ use Drupal\search_api\Utility;
    +  protected $datasourceId;
    

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

  22. +++ b/src/Plugin/search_api/processor/AggregatedFields.php
    @@ -303,87 +348,112 @@ class AggregatedFields extends ProcessorPluginBase {
    +          // (currently) the only way to include computed (processor-added)
    

    Can we remove the "(currently)" or add a link to an issue?

  23. +++ b/src/Plugin/search_api/processor/AggregatedFields.php
    @@ -303,87 +348,112 @@ class AggregatedFields extends ProcessorPluginBase {
    +          // If the field is not already on the item, we need to extract it. (We
    +          // set our own combined ID as the field identifier as kind of a hack,
    +          // to easily be able to add the field values to $property_values
    +          // afterwards).
    

    We can probably remove the parenthesis here.

  24. +++ b/src/Tests/IntegrationTest.php
    @@ -188,7 +188,8 @@ class IntegrationTest extends WebTestBase {
    +    // @todo Make this work correctly.
    +    // $this->assertUrl($this->getIndexPath('fields/add'), array(), 'Correct redirect to index page.');
    

    This should probably be fixed in this patch.

  25. +++ b/src/Tests/IntegrationTest.php
    @@ -469,71 +473,106 @@ class IntegrationTest extends WebTestBase {
    +    // Find the "Remove" Link for the "body" field.
    

    /s/Link/link

drunken monkey’s picture

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: [policy, no patch] Proposal: Omit first line documentation for __construct() and other magic methods 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-&gt;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.

drunken monkey’s picture

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).

borisson_’s picture

The new patch looks great and all my comments are resolved. Not sure what we should do about the comments you mentioned in #4.

  • drunken monkey committed bc3b994 on 8.x-1.x
    Issue #2574969 by drunken monkey, borisson_: Overhauled the "Fields" UI...
drunken monkey’s picture

Status: Needs review » Fixed

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.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.