A separator is being set when adding aggregated field of type other than "Concatenation"

Created on 4 March 2024, 10 months ago
Updated 7 April 2024, 9 months ago

Problem/Motivation

The separator value is being set when adding an aggregated field that is using aggregation type other than the "Concatenation". It also adds two extra empty lines to the configs (See below). The separator functionality has been introduced in ✨ Customize aggregate field fulltext separator Fixed

Steps to reproduce

- Having a search api index
- Add agg field to the index use "Union"
- Set the Contained fields to Content > ID and Media > ID

after exporting the configs, you will see the following:

  agg_id:
    label: 'Agg ID'
    property_path: aggregated_field
    type: string
    configuration:
      type: union
      separator: |+


      fields:
        - 'entity:media/mid'
        - 'entity:node/nid'
đź“Ś Task
Status

Fixed

Version

1.0

Component

Plugins

Created by

🇧🇪Belgium msnassar

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @msnassar
  • 🇺🇸United States RichardDavies Portland, Oregon

    I'm experiencing another issue related to the `separator: |+` config line and extra rows. I'm finding that `drush cex` often wants to insert additional blank lines under the separator line. It doesn't seem to matter how many blank lines are already there, it keeps adding another one every time I export the configuration. Interestingly, when it lists the config that will be exported, search_api.index.my_index isn't listed as having changed, but the yml file is still modified with an additional blank line when the export is executed.

  • 🇦🇹Austria drunken monkey Vienna, Austria

    Thanks for reporting this!
    However, in what way is this a bug or a problem? I think it is common throughout Drupal to keep config values as set by the user even when they are not used. That way, if (in this case) the type of aggregation is changed to “Concatenation”, any previous separator set by the user will still be used.
    We could potentially skip saving this value when it is the same as the default, but I’m still not sure why this poses a problem in the first place.

    @RichardDavies: That does sound like a bug indeed, but (pretty surely) one in Drush, not in this module.

  • Status changed to Postponed: needs info 9 months ago
  • 🇦🇹Austria drunken monkey Vienna, Austria
  • 🇺🇸United States RichardDavies Portland, Oregon

    FYI I filed a bug report with Drush for the extra blank lines: https://github.com/drush-ops/drush/issues/5911

  • Status changed to Active 9 months ago
  • 🇧🇪Belgium msnassar

    @drunken monkey, Thanks for your feedback...
    You are right this is not a bug...Config values could be set while not used... The thing is, setting the separator might be confusing, specially when doesn't know why a separator is set when you have aggregated field of types other than "Concatenation". I think, maybe it would be nicer in such a case to set it to ''?

    @RichardDavies I have encountered the same issue. As a workaround, I set "Value separator" to nothing. For sure this doesn't work in case you have "Concatenation" field and you would like to set it to "\n\n" (There is no problem with other values e.g. "\n", "\t"). I am not sure if it is drash problem, because I see the diff in the UI as well.

  • Status changed to Needs review 9 months ago
  • 🇦🇹Austria drunken monkey Vienna, Austria

    I think we can just remove the setting from the config if it’s equal to the default value and everything should keep working exactly as-is.
    I chose to still save the value if it is used (that is, if the aggregation type is “Concatenation”), in case someone is looking for it in the config export, but not even that would be necessary.
    Anyways, created an MR for this. Please test/review.

  • Status changed to RTBC 9 months ago
  • 🇧🇪Belgium msnassar

    @drunken monkey Thanks, works like a charm if the aggregation type is NOT “Concatenation”. However the issue reported by @RichardDavies still exists if the type is “Concatenation” (This is expected). I believe we should create a follow up, so moving this to RTBC...

  • Status changed to Fixed 9 months ago
  • 🇦🇹Austria drunken monkey Vienna, Austria

    Good to hear, thanks for reporting back.
    Committed.

    Yes, please do create a follow-up in the Core issue queue. I don’t think this is related to this module at all, it might just be specific to config values only containing multiple line breaks (which is probably rather rare).

    Anyways, thanks again!

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

Production build 0.71.5 2024