Problem/Motivation

In #3067803: Use custom delimiter instead of commas for multiple values the configurable separator/delimiter for text was introduced. This allows splitting up string values based on an arbitrary string. The same separator is currently used to join values together if the final value is a string.

This means, that we can't separate values in metatags fields by a special string and still produce "nice" results in html.
With a separator of "||" a taxonomy reference token would result in <meta name="NAME" content="term1||term2||term3">. This should be <meta name="NAME" content="term1,term2,term3">

Proposed resolution

Additional, separate, configurable combinator string used to combine results into a final string falling back to defaultSeparator value if not set.

Remaining tasks

- Review the proposed code
- Improve the test coverage

User interface changes

- Additional field to define the combinator

API changes

- New method MetatagSeparator::getCombinator()

Data model changes

- Additional setting "combinator"

Issue fork metatag-3367071

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:

Comments

pvbergen created an issue. See original summary.

pvbergen’s picture

I've already added my working code to the fork branch. I also included a test method analogous to testSeparator, but I couldn't find a good test case for creating metatags without having tokens. Maybe somebody has an idea.

pvbergen’s picture

Status: Active » Needs review

damienmckenna’s picture

Thank you for pointing out this flaw, and yes, there needs to be a distinction between the two operations.

I turned your issue fork into a merge request and it has triggered the testbot, let's see what the tests say.

damienmckenna’s picture

Title: Add separate combinator » Custom separator also used for string concatenation
Category: Feature request » Bug report
Parent issue: » #3272774: Plan for Metatag 2.0.0

Changing this to a bug, because it's a regression on existing functionality, and this blocks the 2.0.0 release.

damienmckenna’s picture

I wonder if it'd be better to just identify where values are combined and change those lines to use commas to join the strings instead of using the custom separator?

damienmckenna’s picture

Status: Needs review » Needs work

Let's go with my comment above and fix the output so strings are concatenated the old way.

pvbergen’s picture

Going with a fixed comma to combine results into a string is a valid option, indeed. I may have gotten carried away making it configurable.
Of the top of my head, I can't think of a case, where we actually need a configurable value. Especially, as it is a global setting.

I would still use MetaTagManagerMetatagSeparator::getCombinator or similar to get the value, even if it is fixed. This would allow to easily add a configurable option or alter it in the future.

damienmckenna’s picture

I think using a method to get a fixed value is overdoing it, we can just do commas for now.

pvbergen’s picture

I've added a second branch where the combinator is a hardcoded comma, see MR: https://git.drupalcode.org/project/metatag/-/merge_requests/85

damienmckenna’s picture

Status: Needs work » Needs review

Thank you. I'll review that today.

damienmckenna’s picture

Status: Needs review » Fixed

Committed. Thank you!

Status: Fixed » Closed (fixed)

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