Metatags have encoded HTML entities instead of plain text content

I am also looking in to this issue and think it is a bug (but so far have not found out the cause) -- it appears to be related to the new Metatag 8.x-1.15 release, specifically changes made in #2994433: Automatically parse URLs from image field tokens.

Downgrading to Metatag 8.x-1.14 is a workaround for now.

Ok, so in Metatag 8.x-1.15 the PlainTextOutput::renderFromHtml processing on the metatag value was moved from Drupal\metatag\MetatagManager::generateRawElements to Drupal\metatag\Plugin\metatag\Tag\MetaNameBase::output. This change made it so that Drupal\schema_metatag\Plugin\metatag\Tag\SchemaNameBase::output no longer receives the plain text value.

The SchemaNameBase class duplicates a lot of code from Metatag's MetaNameBase and I am not entirely sure why, so I have opened a small MR to add a new PlainTextOutput::renderFromHtml call to SchemaNameBase::processItem. This change should get the metatag values back to plain text strings without encoded HTML entities.

I'm curious to know why SchemaNameBase doesn't inherit more of Metatag's MetaNameBase fucntionality, in case anyone has context. It looks like it may be because this module has to do some extra de/serialization on values? If it is indeed necessary to override so many of the base methods, SchemaNameBase should probably get a thorough review to account for any other changes stemming from Metatag 8.x-1.15 (there appear to be quite a few that could be relevant -- e.g. handling of image URLs from fields).

Updating the title of this issue to clarify the bug.

Tested the patch on a local production, seems to be fixing this issue.

I have the same problem on my sites. I have tested the code, and it works as expected. Would it be possible to merge it?

Just fixing the title.

Also a problem with all tokens
\u003Ca href=\u0022\u0022 hreflang=\u0022ru\u0022\u003E \u003C/a\u003E \u003Ca href=\u0022/

@ScorpionGhost does the patch on this issue resolve the token encoding or does it need changes?

wells, Please give me a link to this patch

Here is the plain diff from #2 — https://git.drupalcode.org/project/schema_metatag/-/merge_requests/2.diff

Wells, Many thanks. We need to add this patch to the module. He saved me from all the problems and made the conclusion clean. Thank you!

Patch works perfectly, thank you!

Confirming RTBC! Just ran into this! Any active maintainer here? Perhaps someone could contact KarenS or apply for Co-maintainership?

Sadly, no. I tried to get myself added as a co-maintainer but was denied for unknown reasons ):

#3208730: Offering to maintain/co-maintain Schema.org Metatag

#3 Works for me too. I was trying to strip html from the tokens but this fix is ways better. Thank you @wells for the fix and the offer to co-maintain. Hopefully Karen find some time to merge the changes or to rethink her desision. Would be great to have you both working on that project.

Patch in #3 works for me. Hopefully this will get merged and a new version released soon.

P.S. @wells, I support your request to become a co-maintainer. It would be great to have more than one maintainer on such a module as this.

Thanks, @markconroy. If anyone is interested we’re discussing a tangent of the maintainer absence issue (the too broad scope of this module) over at #3224351: Define Schema.org Metatag project scope.

Patch from #3 attached for composer.

@reevo - you can use the merge request for composer without problems. just add .patch at the end of the merge request, it will look like this: https://git.drupalcode.org/project/schema_metatag/-/merge_requests/2.patch

you can add this path to your composer.json and install it without problems. you can do this with all merge requests if patches were released this way.

Thanks, @zcht - that was my initial plan, but the project I'm working on has a policy to include patch files instead of pointing to the merge request. Similar to pointing to a specific commit on dev instead of just the latest version, I suppose, so that we know we're only adding code which has been tested internally.

Edit for clarity: I'm assuming this is because new commits can be added to a branch but it'll still live under the same merge request, at the same URL.

As I also wasn't successful contacting KarenS, I created this issue: #3264134: Merge into Metatags? Maintainer KarenS inactive since 2020-10. If someone has any further information about KarenS, please reply there. I'll also contact @DamienMcKenna to ask him about that.

I think it's important to get the RTBC'd issues fixed.

I think it would be good to add test coverage to this to make sure it works as advertised, and make sure the functionality isn't broken later.

I agree this should have a test. I spent a little time today poking around the current test structure and its somewhat complex -- I didn't see an obvious place to add one for this change.

Also running the full tests on my computer takes a little over an hour even with in-memory SQLite ):

I'm hesitant to send this back to NW though... I'll let this one sit for a few days in case anyone wants to assign it and take a stab at adding a test (or feels strongly enough that it should be set back to NW).

@wells perhaps have a look at the tests in the Metatag module? Are there useful examples?

This really needs test coverage - I can't get it to work on the URL value for a Logo object that should output "https://example.com/image.png?w=1000&h=500", even with the patch it's garbled into "https://example.com/image.png?w=1000\u0026h=500"; I confirmed the token output is correct using metatag_generate_entity_metatags(), so maybe it's something in the JSON encoding?

the patch works for me thank you!

Adding to plan as we are using this patch though unfortunately I'm not sure the symtoms that caused it for test

Here is a tests only patch that I think is the simplest way to test for this issue and make some sense placement wise (among other module tests). This test should fail.

Assume this does not address #28 but flipping to NR for tests.

Alright it looks like the test idea from #31 doesn't actually make sense. #28 (and ultimately this issue, to a certain degree) appears to be expected behavior. The character encoding happens at src/SchemaMetatagManager.php#L60 where flags are passed to json_encode specifically to encode certain special characters.

Going all way back to my #2 comment if I downgrade Metatag to 8.x-1.14 with the latest Schema.org Metatag I get something like this for the JSON-LD structure:

    <script type="application/ld+json">{
        "@context": "https://schema.org",
        "@graph": [
            {
                "@type": "Article",
                "description": "This is an article.",
                "about": [
                    "Ohio",
                    "Utah",
                    "Omaha"
                ],
                "image": {
                    "@type": "ImageObject",
                    "representativeOfPage": "False",
                    "url": "https://i.picsum.photos/id/870/200/300.jpg?blur=2\u0026grayscale\u0026hmac=ujRymp644uYVjdKJM7kyLDSsrqNSMVRPnGU99cKl6Vs"
                }
            }
        ]
    }</script>

• description is plain text (without any encoded content).
• about is a collection of plain text tags (without any encoded content).
• image.url is a URL with encoded ampersands.

Then if I bump Metatags to 8.x-1.15 I get this for the same node and data:

<script type="application/ld+json">{
        "@context": "https://schema.org",
        "@graph": [
            {
                "@type": "Article",
                "about": [
                    "\u003Ca href=\u0022/taxonomy/term/1\u0022 hreflang=\u0022en\u0022\u003EOhio\u003C/a\u003E",
                    "\u003Ca href=\u0022/taxonomy/term/2\u0022 hreflang=\u0022en\u0022\u003EUtah\u003C/a\u003E",
                    "\u003Ca href=\u0022/taxonomy/term/3\u0022 hreflang=\u0022en\u0022\u003EOmaha\u003C/a\u003E"
                ],
                "description": "\u003Cp\u003E\u003Cem\u003EThis\u003C/em\u003E is \u003Ca href=\u0022https://www.drupal.org\u0022\u003Ean\u003C/a\u003E \u003Cstrong\u003Earticle\u003C/strong\u003E.\u003C/p\u003E",
                "image": {
                    "@type": "ImageObject",
                    "representativeOfPage": "False",
                    "url": "https://i.picsum.photos/id/870/200/300.jpg?blur=2\u0026grayscale\u0026hmac=ujRymp644uYVjdKJM7kyLDSsrqNSMVRPnGU99cKl6Vs"
                }
            }
        ]
    }</script>

• description is HTML with encoded characters. (changed)
• about is a collection of HTML tags with encoded characters. (changed)
• image.url is a URL with encoded ampersands.

Mostly I think this just further illustrates the issue, confirms my belief that MR!2 addresses the change in behavior itself, and clarifies how #28 is not related.

Still need to figure out where to test for this, though!

Ok! Here's another tests only patch. This should fail.

In my local testing this test passes with Metatag 8.x-1.14 but fails with 8.x-1.15+.

This test uses Drupal\schema_metatag\Plugin\schema_metatag\PropertyType\Text::testValue to provide a test value with HTML and Drupal\schema_metatag\Plugin\schema_metatag\PropertyType\Text::processedTestValue to provide the test value without HTML (ensuring that text property type tags are converted to plain text).

Although I feel like this fits in well enough I do have two concerns with the approach:

1. I don't know if this behavior is explicitly stated/documented anywhere. If it's not -- where should it be documented?
2. This test isn't necessarily broad enough. It covers the simplest text property type but it reality any string-based property type will have the same behavior. Does it need a more explicit (and broader) test, instead? Actually I'm not concerned about this now -- string-like property types use the text property type for sub properties so this test does cover all those cases.

Ok test from #34 is added to MR!2. Flipping this to NR for review. See notes in #34.

@wells, thanks for the test patch, the one in #34 doesn't look like a tests only patch, #31 does. Maybe that was a mistake?

Sorry I'm used to test only patches being in test classes, I don't know enough about this module to comment. Ignore me :D

No worries. I also was very confused by this until I dug through things enough to understand the pattern.

To be clear — #34 is a tests only patch in that it adds methods to a regular class that are used by the current tests to retrieve values for testing.

Made one last push to add some documentation. My concerns from #34 are addressed so this just needs review now.

@DamienMcKenna appreciate if you have a chance to review, or at least confirm my response to #28 in #33.

Verified at PHP 8.1 & MySQL 5.7, D9.5 RC version

We are using the MR as a patch and it works great.

Committed. Thanks everyone!