Process '#attached' key in metatag data before adding tags to the head

[argiepiano]

Backdrop's version of metatag is different from D7's version in one important way: B's version directly adds the head tags in metatag_preprocess_page(). This creates serious problems for metatag data that make use of the #attached key.

One example of that is the data provided by "Canonical URL" metatag in class BackdropLinkMetaTag. The data returned by getElement in this class looks a bit like this:

$element['#attached'] = array(
  'backdrop_add_http_header' => array(
    0 => array(
      'Link', 
      '<' . $value . '>;' . backdrop_http_header_attributes(array('rel' => $element['#attributes']['rel'])), 
      TRUE
    ),
  )
);

This data fails to have any effect when it's run without any preprocessing through backdrop_add_html_head($data, $tag); in metatag_preprocess_page(). The result of doing it this way (i.e. ADDING the data to the head BEFORE running the #attached function) is that, in this case, the Canonical URL Link header is never sent to the browser, as reported in #109.

There are TONS of other metatags, provided by a module I'm porting called schema_metatag, that make use of the [#attached] key. Because of the current approach taken by Backdrop (i.e. using a "shortcut" and adding the tags directly in metatag_preprocess_page(), instead of doing it the way D7 does it), those tags fail to be added, and in fact crash the rest of the tags added later, because they result in malformed <meta> tags being added.

There is a solution that avoids completely rewriting metatag_preprocess_page(). The solution is to check if the metatag data contains [#attached] as the top-most element. If it does, instead of directly adding the "raw" data, you call backdrop_render() for that data. Backdrop is smart enough to call the attached function with the arguments provided in the data, AND adds the tag to the head without the need of manually using backdrop_add_html_head().

This solution solves not only the Canonical URL problem, but also all the schema_metatag malfunctioning tags.

I'll provide the PR later tonight. It's a one-liner.

[argiepiano]

PR ready for review. This is a site that uses Canonical URL metatag. Unlike other basic metatags, Canonical URL is supposed to be sent to the browser in the headers. Before the patch, inspecting the headers, Link is not included:

After the patch:

I've tested the patch with some of the tags provided by schema_metatag, and they work as expected.

[dgbruns]

I've been testing a patched version of Metatag (#110) with Metatag schema, with the 'article' schema type.

In general, it's working well and I see schema tags in json-ld format appear in the of pages configured to use the article schema type. However, while testing yesterday, I ran into a new error:

Warning: strtolower() expects parameter 1 to be string, array given in strtolower() (line 2237 of /var/www/html/web/modules/contrib/metatag/metatag.module).

This happens because the $attr_value delivered to strtolower can be an array for things like the 'organization' and 'ImageObject' that are part of the 'article' spec. I'm not sure why I didn't see this before, but I noticed after clearing caches, the error doesn't always appear right away. If I hack the code to check for a string:

          if (is_string($attr_value)) {
            $attr_value = strtolower($attr_value); // Account for G in generator.
          }

The error goes away.

Let me know if you need more details or if I should open a separate issue.

[quicksketch]

@argiepiano Thanks for the PR! I'll be taking a look at this shortly. Can either you or @dgbruns provide some steps to reproduce the problem?

[dgbruns]

@quicksketch - just sent over some info and json config. The gist is that I am testing the 'Article' type only at the 'Content' level in Metag with the patched version of Metatag that uses backdrop_render.

It seems that the error above happens when pieces of the json-ld array (the sub-arrays for persons and organizations) hit strtolower.

[quicksketch]

Thanks @argiepiano and @dgbruns. I've reproduced the problem with the schema_metatag module.

@dgbruns I think your little fix is also totally okay.

I'd like to have automated test coverage for the variety of situations we're putting forward, but it seems like the Metatag module tests need some work. I'll put up a combined PR that includes #111 plus @dgbruns's fix, and if possible, some test coverage.

[quicksketch]

@argiepiano I see your point about needing to support #attached in BackdropLinkMetaTag and possibly other places to add HTTP headers, but I'm also looking at schema_metatag and it looks as though the pull request here is not required for that module to function properly? I filed a PR at backdrop-contrib/schema_metatag#1 that makes schema_metatag return the element directly, without using #attached at all. It seems to work just fine. And while I think it's still important we figure this out for other tags, I think schema_metatag might be better off if it uses the simplified syntax.

[quicksketch]

I checked metatag's handling of #attached and it looks like it already works correctly. There was a separate issue with Canonical not working #109, but with that issue fixed I have confirmed that the HTTP Link header already works properly. The X-Generator header is another example of a metatag that exists both in HTML and in the HTTP header. I added test coverage for both in a PR at #117.

So in summary to fix this issue, I think we need to do both of these things:

1. Merge Metatag Issue #110 alternative: Return element without using #attached. schema_metatag#1 to make schema_metatag act like other metatags.
2. Merge Issue #110: Add test coverage for canonical link. #117, which includes @dgbruns's fix and test coverage.

[argiepiano]

OK. I'll close my PR for this issue then.

[quicksketch]

Perfect, and I see you merged backdrop-contrib/schema_metatag#1. I've merged #117 as well and I'll make a new release shortly.