Use custom delimiter instead of commas for multiple values

Seeing as use of comma as the delimiter causes other problems, e.g. #2761909: og:image does not parse url from image field properly when there is a comma in the alt field, maybe we should deprecate use of commas an change to something else?

I can verify the patch in #11 is working for me.

Patch in #11 is also working fine for me.

Rerolled.

This is really good, thank you.

I see one issue with this: backwards compatibility for all sites that have multiple values entered. While the existing tests work, we need additional test coverage to ensure backwards compatibility. I'd also like to see this made configurable, maybe a global configuration to choose between the two options, and for new installs it would use the new value but for older sites it would use commas.

This makes a minor change to verify $item['#attributes']['content'] exists before using it.

Back to 'needs work'.

I have this issue on Drupal 7. We have images that have a comma in the URL and the OpenGraph image link is trucated at the comma. Does anybody know of a fix for this?

I created a patch for this issue with the Drupal 7 branch (this is my first time doing this)

Didn't mean to hide Damien's files

Thank you for the backport to D7.

Thanks for reviewing DamienMcKenna

WIP on adding a setting form to control the delimiter.

This fixes a submission error on the settings page when trying to save the separator value.

Reroll #34.

I've tried patch from 35 bit get this error.

Message Error: Call to a member function get() on null in Drupal\metatag\Plugin\metatag\Tag\MetaNameBase->getSeparator() (line 296 of /app/public_html/modules/contrib/metatag/src/Plugin/metatag/Tag/MetaNameBase.php)

Patch #35 is incompatible with the Metatag Views submodule as well:

ArgumentCountError: Too few arguments to function Drupal\metatag\MetatagManager::__construct(), 5 passed in /app/web/core/lib/Drupal/Component/DependencyInjection/Container.php on line 259 and exactly 6 expected in Drupal\metatag\MetatagManager->__construct() (line 78 of /app/web/modules/contrib/metatag/src/MetatagManager.php)

#0 /app/web/core/lib/Drupal/Component/DependencyInjection/Container.php(259): Drupal\metatag\MetatagManager->__construct(Object(Drupal\metatag\MetatagGroupPluginManager), Object(Drupal\metatag\MetatagTagPluginManager), Object(Drupal\metatag\MetatagToken), Object(Drupal\Core\Logger\LoggerChannelFactory), Object(Drupal\Core\Entity\EntityTypeManager))
#1 /app/web/core/lib/Drupal/Component/DependencyInjection/Container.php(173): Drupal\Component\DependencyInjection\Container->createService(Array, 'metatag.manager')
#2 /app/web/modules/contrib/metatag/metatag_views/src/Plugin/views/display_extender/MetatagDisplayExtender.php(47): Drupal\Component\DependencyInjection\Container->get('metatag.manager')
#3 /app/web/core/lib/Drupal/Core/Plugin/Factory/ContainerFactory.php(21): Drupal\metatag_views\Plugin\views\display_extender\MetatagDisplayExtender::create(Object(Drupal\Core\DependencyInjection\Container), Array, 'metatag_display...', Array)
#4 /app/web/core/lib/Drupal/Component/Plugin/PluginManagerBase.php(83): Drupal\Core\Plugin\Factory\ContainerFactory->createInstance('metatag_display...', Array)
...

The patch #35 no longer appears to be apply on version 1.15

I can confirm that latest version doesn't work, patch fails.

If the patch doesn't work why do the tests pass? 🤔

I've started the tests again.

Any updates on this?

Are there any plans for a working patch?

Hi,

I'd like to report that #35 works for me on the 8.x-1.x-dev branch.

Multiple field values are correctly joined together respecting the new delimiter and preserving commas.

The one issue I found with the patch, though, is that this delimiter is also used on tags where the comma delimiter rightly belongs, namely, the robots metatag.

After applying the patch, and setting the delimiter to ||, this is what the robots metatag looks like:

<meta name="robots" content="index|| follow">

but it should be of course:

<meta name="robots" content="index, follow">

Merge request 20 applies for me as patch on 8.x-1.16 and it does the job, although the test fails.

Merge Request 20 also applied cleanly for me and worked as expected. I believe the issue with the failed test is because there are two images defined in the og_image_url tag

'og_image_url' => 'https://www.example.com/example/foo.png,https://www.example.com/example/foo2.png',

And the test was missing the second image from the expected tags, I added in the second image and the tests pass locally.

Moving this to Needs Review and waiting for the test bot results to come back.

It did seem like the tests were overlapping each other quite a bit. MetatagManagerTest::testMetatagOrder() and MetatagManagerTest::testMetatagMultiple() were doing exactly the same thing after my change. MetatagManagerTest::testSeparator() was also doing some of the same checks.

I thought maybe MetatagManagerTest::testMetatagOrder() should just have a single value for 'og_image_url' and MetatagManagerTest::testMetatagMultiple() should test for a multi-value.

I've updated the merge request to revert the test back to how it was before my previous commit, and change the value of og_image_url to be a single value in MetatagManagerTest::testMetatagOrder()

Updated the D7 patch in #21 so it uses the configured delimiter in DrupalDefaultMetaTag as well.

It may just be me for some reason, but I've had no luck getting any of these D8 patches to apply.
I've seen comments of people applying it to 8.x-1.16, which is also my current version, but I've been unsuccessful.
It seems that this definitely needs a re-roll against latest 8.x-1.19, but has anyone else been successful in applying the patches to any specific versions?

Update:
I've attempted a re-roll of this patch, but the functionality was not working as expected.

I set the custom delimeter to "^".

With the re-rolled patch applied, I was getting a metatag result like so:
"User A^ User B^ User X"

But it seems it was for some reason not then converting the character back to commas for the output.

Rerolled.

Rerolled.

Wrong file.

I think this cover it - there were a few additional locations that a comma was hardcoded in the logic.

A minor improvement on the settings form.

Some improvements, including removal of the global constant.

This might work better.

We now have a working solution for both D7 and D9, which should unblock some improvements elsewhere in the ecosystem.

Has anyone tried using the latest patch? Does it work for you?

The patch in #66 works as expected and it is quite elegant. I am going to RTBC, however, care should be given as to how this is released as it breaks and schema metatags that are installed and white screens the site. I assume the maintainers of both modules will need to work together and merge to dev/release at the same time.

After installing the patch, the user is presented a new option on the metatag settings page with excellent instructions.

After changing the default , to ^ I saved.

I then edited a metatag default and added an array of terms using the custom separator. Note that the description on the tags that allow multiple are dynamic and now list the custom separator instead of the comma.

After saving the default, I visited the page and was able to see the 3 different multiple tags in the page's source code.

As a reference, here is the error that happens if any schema metatags are on the node.

You can see the error is happening in metatag, but that it is caused by schema_web_page.

Thanks for all the great work on this!

I need to do more testing with Schema Metatag..

I had honestly forgotten that the schema_metatag module was no longer tested by Metatag, so I've rerolled #3123521 and will look into this problem to see if it's possible to avoid the compatibility break.

Moving this to Metatag v2.

@thejimbirch So you got an error/white screen, right?

So how did you resolve it? Thanks!

@AaronELBorg I was just testing the patch. The patch will work in you only use the Metatag module, and not the Schema.org Metatag module.

I believe you will have to wait for Metatag 2.0.0 https://www.drupal.org/project/metatag/issues/3272774 and Schema Metatag https://www.drupal.org/project/schema_metatag/issues/3272777 to catch up, but there has been no movement there it looks like.

Guys dont we have a quick solution or patch for this, which will work with schema metatag module too?

I cant afford to wait too long.

The patch is #66 doesn't seem to apply to latest Metatag anymore, seems like this needs a re-roll.

Reroll of patch #66 to be compatible with latest 8.x-1.x-dev.

MetatagSeparator.php class file was missing from #76, it's now included in the new patch file.

I have tried patch#77, after applying the patch I got bellow error

The website encountered an unexpected error. Please try again later.
Error: Call to a member function get() on null in Drupal\metatag\Plugin\metatag\Tag\MetaNameBase->getSeparator() (line 27 of modules/contrib/metatag/src/MetatagSeparator.php).
Drupal\metatag\Plugin\metatag\Tag\MetaNameBase->getSeparator() (Line: 780)
Drupal\metatag\MetatagManager->processTagValue(Object, Array, Array, , 'en') (Line: 628)
Drupal\metatag\MetatagManager->generateRawElements(Array, Object) (Line: 560)
Drupal\metatag\MetatagManager->generateElements(Array, Object) (Line: 528)
metatag_get_tags_from_route() (Line: 285)
_metatag_remove_duplicate_entity_tags(Array) (Line: 227)
metatag_entity_view_alter(Array, Object, Object) (Line: 562)
Drupal\Core\Extension\ModuleHandler->alter('node_view', Array, Object, Object) (Line: 305)
Drupal\Core\Entity\EntityViewBuilder->buildMultiple(Array) (Line: 239)
Drupal\Core\Entity\EntityViewBuilder->build(Array)
call_user_func_array(Array, Array) (Line: 101)
Drupal\Core\Render\Renderer->doTrustedCallback(Array, Array, 'Render #pre_render callbacks must be methods of a class that implements \Drupal\Core\Security\TrustedCallbackInterface or be an anonymous function. The callback was %s. See https://www.drupal.org/node/2966725', 'exception', 'Drupal\Core\Render\Element\RenderCallbackInterface') (Line: 772)
Drupal\Core\Render\Renderer->doCallback('#pre_render', Array, Array) (Line: 363)
Drupal\Core\Render\Renderer->doRender(Array, ) (Line: 201)
Drupal\Core\Render\Renderer->render(Array, ) (Line: 241)
Drupal\Core\Render\MainContent\HtmlRenderer->Drupal\Core\Render\MainContent\{closure}() (Line: 564)
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object) (Line: 242)
Drupal\Core\Render\MainContent\HtmlRenderer->prepare(Array, Object, Object) (Line: 132)
Drupal\Core\Render\MainContent\HtmlRenderer->renderResponse(Array, Object, Object) (Line: 90)
Drupal\Core\EventSubscriber\MainContentViewSubscriber->onViewRenderArray(Object, 'kernel.view', Object)
call_user_func(Array, Object, 'kernel.view', Object) (Line: 142)
Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch(Object, 'kernel.view') (Line: 164)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1) (Line: 81)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 58)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 48)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 50)
Drupal\ban\BanMiddleware->handle(Object, 1, 1) (Line: 48)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 51)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 23)
Stack\StackedHttpKernel->handle(Object, 1, 1) (Line: 709)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)

Yeah, the getSeparator() method needs to be rewritten to be more resilient in case the configuration isn't defined.

I've applied patch #77 and I get the same error: "Error: Call to a member function get() on null in Drupal\metatag\Plugin\metatag\Tag\MetaNameBase->getSeparator() (line 27 of modules/contrib/metatag/src/MetatagSeparator.php)." Debugging shows that MetatagSeparator.php is in the context of Drupal\metatag\Plugin\metatag\Tag\GeoPlacename and $this->configFactory() is null.
Debugging shows that MetatagSeparator.php is in the context of Drupal\schema_article\Plugin\metatag\Tag\SchemaArticleType and $this->configFactory is null.

Something like the below seems to work:

public function getSeparator() {
    $separator = '';
    if (isset($this->configFactory)) {
      $config = $this->configFactory->get('metatag.settings');
      $separator = trim($config->get('separator') ?? '');
      if ($separator === '') {
        $separator = $this::$default_separator;
      }
      return $separator;
    }
  }

This might need to be rerolled for the 2.0.x branch.

Rerolled.

Committed. Thanks everyone!

I tried to apply patch from #82 and got this error (while a different patch works flawlessly):
Could not apply patch! Skipping. The error was: Cannot apply patch https://www.drupal.org/files/issues/2023-01-05/metatag-n3067803-82.patch

/edit: so it has been committed I guess? But where is the setting even? :D

@castor-designs: This change was committed to the 2.0.x branch to be included in the new 2.0.0 release, it isn't in 8.x-1.x releases.

@DamienMcKenna can we port a fix to the 1.x branch?

This isn't a feature I'm intending to backport as it caused problems with submodules.

Thanks for the reply... just leaving a patch here for others to use selectively then.
With correct line endings this time.

Any possibilities to have separator+space?
To fix "Value1,Value2" with "Value1, Value2"

Unfortunately space is trimmed after saving form settings.

Please open a new issue and we'll be able to help. Thank you.