Problem/Motivation

When you create an Aggregate field Data alteration Filter on an Index, you can choose an Aggregation type. One of the Aggregation types is Fulltext which processes the data as follows...

return isset($a) ? $a . "\n\n" . $b : $b;

... this code is in the function \SearchApiAlterAddAggregation::reduce().

An administrator may want to use a separator other than two newline characters.

Proposed resolution

Turn the separator into a configuration option, expose the configuration option to the administrator.

Remaining tasks

  1. Determine if it is possible to access configuration options in \SearchApiAlterAddAggregation::reduce().
  2. Write a patch that adds the configuration option to \SearchApiAlterAddAggregation::configurationForm() and company, and uses the configuration option in \SearchApiAlterAddAggregation::reduce().
  3. Review and feedback
  4. RTBC and feedback
  5. Commit

User interface changes

New configuration option when adding an aggregate field.

API changes

New configuration option in \SearchApiAlterAddAggregation.

Fulltext aggregate field could separate items with something other than two newlines.

Data model changes

None.

CommentFileSizeAuthor
#14 2855447-14.patch8.1 KBalexdoma
#13 2855447-13.patch8.2 KBalexdoma
#12 2855447-11--concat_aggregation_separator.patch7.98 KBdrunken monkey
#11 2855447-11--concat_aggregation_separator--interdiff.txt8.44 KBdrunken monkey
#4 2855447-4--fulltext_aggregation_separator.patch4 KBdrunken monkey
#4 2855447-4--fulltext_aggregation_separator--interdiff.txt3.92 KBdrunken monkey
#3 2855447-3-custom-aggregate-field-fulltext-separator.patch2.69 KBmparker17

Issue fork search_api-2855447

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

mparker17 created an issue. See original summary.

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented
Related issues: +#1357070: Allow multiple value string aggregated fields

Note that #1357070: Allow multiple value string aggregated fields is similar (it adds a List reductionType). This solution is different because it still produces a Fulltext reductionType, it just lets the admin choose the string between the first and second value.

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented
Assigned: mparker17 » Unassigned
Status: Active » Needs review
StatusFileSize
new 2855447-3-custom-aggregate-field-fulltext-separator.patch2.69 KB

Here's a simple patch. Feedback welcome.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Component: General code » Plugins
Issue tags: +needs port to Drupal 8
StatusFileSize
new 2855447-4--fulltext_aggregation_separator--interdiff.txt3.92 KB
new 2855447-4--fulltext_aggregation_separator.patch4 KB

Thanks a lot for this suggestion! Seems like a simple and nice-to-have feature.
The patch also looks quite good, thanks!
A few suggested improvements:

  • Show the "Separator" field under the type description, and only when type "Fulltext" is actually selected.
  • Use a text field instead of a text area, and escape sequences for including newlines or tab characters. (At least in my browser, the "\n\n" default value wasn't correctly displayed as the default, instead being transformed to a single newline.)

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Version: 7.x-1.x-dev » 8.x-1.x-dev
Status: Needs review » Patch (to be ported)
Issue tags: -needs port to Drupal 8

Committed.
Thanks again for your work!

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Issue tags: +Novice

edurenye made their first commit to this issue’s fork.

edurenye opened merge request !42

edurenye’s picture

edurenye commented
Status: Patch (to be ported) » Needs review

I mage a merge request, I'm not sure how to deal well with the configuration, but I think this relates more to a more general issue, maybe we should add the aggregate_field types as plugins by themselves or move all that into a sub-module like is proposed here: https://www.drupal.org/project/search_api/issues/3220868

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Issue tags: -Novice
StatusFileSize
new 2855447-11--concat_aggregation_separator--interdiff.txt8.44 KB

Thanks a lot for that, already looks quite good!
Unfortunately, testing for our MRs still doesn’t seem to work, so switching to patches. The attached one includes a few minor updates to your code style, a description for the form field (non-trivial, after all), escaping for the form field values and tests.
Please test/review!

Also, stumbled upon #3280916: Field configurations do not correctly include defaults while testing – apparently, we do need those ?? "\n\n" snippets for now, so leaving those in.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
StatusFileSize
new 2855447-11--concat_aggregation_separator.patch7.98 KB
alexdoma’s picture

alexdoma commented
StatusFileSize
new 2855447-13.patch8.2 KB

re-roll for drupal 9.5.9

alexdoma’s picture

alexdoma commented
StatusFileSize
new 2855447-14.patch8.1 KB

fix previous patch - set correct paths, re-roll for drupal 9.5.9

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Status: Needs review » Fixed

Looks good, thanks for re-rolling!
I assume this means the patch was also working for you, so I think we can proceed. I just fixed the method signature of testAggregation() and removed the empty testPropertyConfigurationForm() method and then merged.
Thanks again!

Status: Fixed » Closed (fixed)

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