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
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
Comment #2
pvbergen commentedI'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.
Comment #3
pvbergen commentedComment #5
damienmckennaThank 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.
Comment #6
damienmckennaChanging this to a bug, because it's a regression on existing functionality, and this blocks the 2.0.0 release.
Comment #7
damienmckennaI 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?
Comment #8
damienmckennaLet's go with my comment above and fix the output so strings are concatenated the old way.
Comment #9
pvbergen commentedGoing 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.Comment #10
damienmckennaI think using a method to get a fixed value is overdoing it, we can just do commas for now.
Comment #12
pvbergen commentedI've added a second branch where the combinator is a hardcoded comma, see MR: https://git.drupalcode.org/project/metatag/-/merge_requests/85
Comment #13
damienmckennaThank you. I'll review that today.
Comment #16
damienmckennaCommitted. Thank you!