Once #917998: Make data alter callbacks objects and let them have a configuration form is committed, it will be possible to let data alterations have configuration forms. The old "Fulltext field" data alteration with zero flexibility should then be scraped and replaced by the following approach:

By default, the callback does nothing. However, it provides a form, where one can create additional fields that will be added to indexed entities, along with the fields that should be aggregated. So it woul be possible, e.g., for a node index to have a fulltext field containing the title and all tags, and another one with title, body and author name — completely regardless of whether they are indexed and of their respective types.

Since there currently are no validation and submit hook methods for the configuration process (identical to processors), it will be a bit complicated to get the UI usable, but I think I've got a solution already: Fieldsets for all defined fields, plus an additional empty one for a new field (or several?), each containing the necessary fields for name and contained fields.

Extra question: Should the field ID be configurable, does that have any advantage? I don't really see an advantage, but changing a field ID might cause problems and I can't really think of any benefit this might have.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

drunken monkey’s picture

First problem I ran into: #1015798: Fieldsets inside vertical tabs have no title and can't be collapsed.
If this doesn't get fixed soon, I'll just fix it for now in search_api.admin.css. Can't imagine a clear way to layout this form without collapsible fieldsets …

drunken monkey’s picture

Status: Active » Needs review
FileSize
8.67 KB

The attached patch alters the data alteration as proposed, and seems to work fine. (I even fixed a nasty bug with multi-valued fields that I had previously overlooked …)

Since #1015798: Fieldsets inside vertical tabs have no title and can't be collapsed hasn't been fixed (or even commented) yet, the patch also contains a workaround, which at least should work for the core themes. (Obviously, this isn't the best time to post a D7 core patch …)

drunken monkey’s picture

Status: Needs review » Fixed

Committed.

I've also added a little extra to search_api_extract_fields() so when extracting fulltext data from fields with an option list, the option "labels" will be attached automatically.

fago’s picture

Status: Fixed » Active

I'd like to suggest the follow UI improvements:
* Select all properties of type text by default.
* Having to fiddle around with the select is cumbersome. E.g. forget to press Strg/Ctrl once and your selection is gone. I'd much prefer a scrollable list of checkboxes ala views. That way you could also easily include the property description in the checkbox description. For making the list of checkboxes scrollable CSS should work.

drunken monkey’s picture

Also, the general UI of adding a field, and the descriptions should be improved. Also, the alteration name should be changed to "Aggregated fulltext fields" to more clearly reflect what the alteration does. After all, "normal" fulltext fields are also directly available by setting a field to type "Fulltext" on the "Fields" page, so it isn't clear what the difference is.
The improved UI would probably require #1043456: Form validation/submission for plugins.

Another UI change here would be to slightly alter the workflow when creating an index, to first select the fields to index, and then the workflow. This might be more fitting for first-time users.

drunken monkey’s picture

Status: Active » Needs review

OK, I have now incorporated all the suggestions. With one exception, though: I really couldn't come up with a feasible way to make the UI more intuitive, regarding creating/deleting aggregated fields.
I hope with the new alteration name and the more detailed description users will have much less problems nevertheless. Please review!

Also, I'm of course still open for suggestions if you think the UI isn't at least adequate (no talking of "perfect" here …).

drunken monkey’s picture

To clarify: the changes are already committed, but should still be reviewed. Er, yes …

drunken monkey’s picture

Component: Code » Plugins
fago’s picture

I gave it a test, and with the improved descriptions and name it's already a lot better.

>I really couldn't come up with a feasible way to make the UI more intuitive, regarding creating/deleting aggregated fields.

Yep, the current way is still awkward.
What about adding buttons for actions as discussed? -> Remove the ugly "new field" fieldset and replace it with an "Add field" button, that rebuilds the form. Also, for deleting you could just show a button in a field's fieldset.

Beside that, it would be a nice UX improvement if you'd provide a default-configured field once the data-alteration thingie is activated the very first time and there are no settings for it. Thus, you could provide a default "Aggregated text" field for which all 'text' properties are checked. That's probably exactly what 90% need.

fago’s picture

Also, I just noted that the fields are listed in a strange order, e.g.:
>Language » Languages » Level
>General skills » General skills » Level
>IT skills » IT skills » Level
- instead of grouping fields of the same related "entity" together.

Then, at the bottom there were even some fields floating around like "Field-collection item ID" which obviously stem from a related entity, but don't are named that way?

drunken monkey’s picture

What about adding buttons for actions as discussed? -> Remove the ugly "new field" fieldset and replace it with an "Add field" button, that rebuilds the form. Also, for deleting you could just show a button in a field's fieldset.

Yes, I tried that of course, but with this being just a subform and JS-less users to keep in mind, this is really hard to get right, at least for me. Also, for the delete buttons: isn't there some Form API issue when several identical submit buttons are used in a form? Or is that already fixed?
I can try again, of course, but I really hoped that wouldn't be necessary. Or maybe you want to take a shot at it? ;)

Beside that, it would be a nice UX improvement if you'd provide a default-configured field once the data-alteration thingie is activated the very first time and there are no settings for it. Thus, you could provide a default "Aggregated text" field for which all 'text' properties are checked. That's probably exactly what 90% need.

Just tried it and that could be added really easily. However, I'm not so sure about doing this: For one, I don't think that will exactly fit the requirements most of the time — e.g., for nodes the "Revision log message" will be included, which I can hardly imagine many people might want to index, the same goes for the "Language" field. And additionally, this might confuse users again as to what is the general workflow for adding aggregated fields.
Eventually, they will have to review the contained fields anyways, and maybe also want to select an own name. Therefore, the use of this really doesn't seem that great. But you can look at the attached patch, where I implemented this as a test.

Also, I just noted that the fields are listed in a strange order, e.g.:

I cannot reproduce this. The fields should always be displayed exactly like they are in the "Fields" tab.

Then, at the bottom there were even some fields floating around like "Field-collection item ID" which obviously stem from a related entity, but don't are named that way?

Ah, this seems to be a small bug in the "Add related entity" code, which shouldn't really come up in normal situations. Just save the "Fields" form to fix this. But I also fixed the function now and will commit this as soon as, well, I can commit again. ;)

drunken monkey’s picture

Just fixed another little bug — i.e., that the genereated description for aggregated fields was incorrect.

fago’s picture

Just tried it and that could be added really easily. However, I'm not so sure about doing this: For one, I don't think that will exactly fit the requirements most of the time — e.g., for nodes the "Revision log message" will be included, which I can hardly imagine many people might want to index, the same goes for the "Language" field. And additionally, this might confuse users again as to what is the general workflow for adding aggregated fields.

Indeed, so probably that wasn't so a good idea. :/

Yes, I tried that of course, but with this being just a subform and JS-less users to keep in mind, this is really hard to get right, at least for me. Also, for the delete buttons: isn't there some Form API issue when several identical submit buttons are used in a form? Or is that already fixed?
I can try again, of course, but I really hoped that wouldn't be necessary. Or maybe you want to take a shot at it? ;)

This shouldn't be a problem with FAPI, also JS isn't required at all. You can check for the pressed button in a submit handler with $form_state['triggering_element'], just ensure the button name is unique for the whole page. Also limit the validation errors as needed. So in the submit handler, best add an empty aggregated field, enable $form_state['rebuild'], during rebuild properly show the form for empty aggregated fields and you are done.
Once you have that, you can optionally activate AJAX via the #ajax property for the button, such it would work both with JS per ajx and without JS as usual.

fago’s picture

Status: Needs review » Needs work

Setting to needs work due to the button.

drunken monkey’s picture

Status: Needs work » Needs review
FileSize
10.57 KB

Ah, that's the Form API expert recommended by Dries at work! ;)
With your tips, it really went quite smoothly. Patch attached.

drunken monkey’s picture

Previous patch causes problems with the vertical tabs. Took some time to figure out how to do this …
Also included is the removal of a doc comment in the callback interface saying I shouldn't do something I'm doing — not really best practice to change the guidelines instead of the code, but … *shrug*

drunken monkey’s picture

Status: Needs review » Fixed

Committed.

Status: Fixed » Closed (fixed)

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