- 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 1:33pm 14 March 2024 - 🇺🇸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 8:54am 15 March 2024 - 🇧🇪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.
- Merge request !119Optimize config export of aggregated fields → (Merged) created by drunken monkey
- Status changed to Needs review
9 months ago 10:01am 17 March 2024 - 🇦🇹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 9:40am 19 March 2024 - 🇧🇪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...
-
drunken monkey →
committed b35a3fec on 8.x-1.x
Issue #3425506 by drunken monkey, msnassar: Fixed unnecessary storing of...
-
drunken monkey →
committed b35a3fec on 8.x-1.x
- Status changed to Fixed
9 months ago 12:32pm 24 March 2024 - 🇦🇹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.