Wrong meta description markup

mazzech commented 3 years ago

screen-meta-output

The current version seems to provide a false meta description text markup <meta property="description"> instead of <meta name="description">

This might affect every website using the module. There's a good workaround (using the meta seo module instead, but unfortunately without support for tokens). Thank you @indigoxela for confirmation of the bug

indigoxela commented 3 years ago

Another screenshot for comparison, I'm not using metatag module with any D7, but there's a D8 - and it sets the correct attribute (name, not property):

screenshot-meta-d8

Mozilla page for the meta tag: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/meta

Wikipedia page about RDFa - these do have property attributes: https://en.wikipedia.org/wiki/RDFa

BTW: the same is true for generator (wrong attribute "property" instead of "name")

indigoxela commented 3 years ago

And this is the commit that broke it: https://github.com/backdrop-contrib/metatag/commit/0ffd42573c21fae2dbbc9e3236263bc52515381f

This changed the attribute from hardcoded "name" to hardcoded "property", both doesn't seem right.

The D7 code looks pretty different: https://git.drupalcode.org/project/metatag/-/blob/7.x-1.x/metatag.inc#L164

And here's a quick and dirty workaround in metatag.inc (beginning with line 132):

        // Workaround, see backdrop-contrib/metatag/issues/56
        $has_name = array(
          'description',
          'generator',
        );
        $attribute = 'property';
        if (in_array($this->info['name'], $has_name)) {
          $attribute = 'name';
        }
        // Combine the base configuration for this meta tag with the value.
        $element = $base_element + array(
          '#tag' => 'meta',
          '#attributes' => array(
            $attribute => $this->info['name'],
            'content' => $value,
          ),
          '#weight' => $this->getWeight(),
        );

Note: it's no real solution. What it really needs is to switch back to the way D7 does it (with theme functions).

Note 2: twitter also wants the "name" attribute: https://developer.twitter.com/en/docs/twitter-for-websites/cards/guides/getting-started

neessen commented 3 years ago

This is a major issue with this module that makes it useless unless a solution can be found to this problem. All Backdrop sites using this module will not have their meta description indexed by Google until this is solved.

Building on the solution by @indigoxela I have made this workaround, but I think it is still a quick and dirty workaround:

if(in_array($this->info['group'], array("basic", "advanced"))){
  $meta_content_attribute = "name";
}else{
   $meta_content_attribute = "property";
}
 // Combine the base configuration for this meta tag with the value.
$element = $base_element + array(
  '#tag' => 'meta',
  '#attributes' => array(
    $meta_content_attribute => $this->info['name'],
    'content' => $value,
  ),
  '#weight' => $this->getWeight(),
);

The difference is, that i think all meta tags located in the basic and advanced tabs on the node edit page should be outputted with name instead of property: <meta name="resume".. <meta name="keywords".. <meta name="icbm" ... and so on. However the open graph tags (if used) shoud be outputted with property: <meta property="og:title".. <meta property="og:description".. and so on. Luckily these are categorized with their group name (basic, advanced, open_graph etc..) so this approach should output name= instead of property= for the basic and advanced group. However I would imagine that there would be better solutions than this, but with my limited skills in module development this was the solution I could think of, so any review, comments and suggestions would be appreciated :)

indigoxela commented 3 years ago

This is a major issue with this module

@neessen I agree.

Using the groups to distinguish the attributes is for sure worth a try. Do you happen to know in which group the twitter meta tags are? The twitter tags need "name".

Trying to ping @jenlampton (the current maintainer), as I suspect, she missed this new issue.

jenlampton commented 3 years ago

Thanks for the ping @indigoxela. I don't think the intent was for that commit to change all name attributes to properties, just some of them. I'm going to revert that and re-open the related issue.

neessen commented 3 years ago

Hey @indigoxela

Below is a list of all info on all possible fields at the moment. Looks like all the twitter stuff is placed in the "twitter-cards" group.

So I guess we could just add that to the group checkup array (see below)

@jenlampton thank you for looking into this :)
It is better to revert this (changing property to name), but we would still have issues with the tags that should be outputted with property then. (e.g open graph tags)

Updated code

if(in_array($this->info['group'], array("basic", "advanced", "twitter-cards"))){
  $meta_content_attribute = "name";
}else{
  $meta_content_attribute = "property";
}
// Combine the base configuration for this meta tag with the value.
$element = $base_element + array(
  '#tag' => 'meta',
  '#attributes' => array(
    $meta_content_attribute => $this->info['name'],
    'content' => $value,
  ),
  '#weight' => $this->getWeight(),
);

List of all tags:

indigoxela commented 3 years ago

@neessen many thanks for providing all the info.

I get the impression that only two groups need the property attribute:

[group] => facebook
[group] => open-graph

The rest needs name. If we can agree on that, a fix should be fairly easy.

@jenlampton I suppose you prefer to continue on that other (now re-opened) issue?

indigoxela commented 3 years ago

Anyway, here's a PR that adopts @neessen's suggestion to use the group info to distinguish the attribute.

Besides the hardcoded attribute problem, this PR also fixes some (but not all) functional tests. They didn't break because of this PR, but must have been broken for quite a while. (Tested with php 7.4)

At least, the relevant tests for this issue here are passing.

@jenlampton I'll open a new issue for the two (unrelated) failing tests.

indigoxela commented 3 years ago

Another update for the PR fixes the caching test - it was just an oversight in var_export(), everything seems to work fine.

How to test:

If you're not planning to check the functional tests (Simpletest), it's sufficient to download the first commit patch to the main directory of your Metatag install.

Then apply the change (via command line) with: patch -p1 < c018c025bf8728b4575bbfd564f12cc562c2c5ea.patch and flush caches.

The attributes of all the meta tags added by this module should be correct then. For instance "name" for "description" and "property" for "og:image". Feedback is welcome.

Here's a screenshot of markup of a site this patch has been applied: screenshot-metatags

indigoxela commented 3 years ago

Any progress? Reminder: there's a PR available, which needs testing and review. It fixes a major bug.

I wonder if the @backdrop-contrib/bug-squad team could help here?

jenlampton commented 3 years ago

Bug squad can help if the issue is rtbc for 2 weeks or more.

On Wed, Nov 4, 2020, 11:29 PM indigoxela notifications@github.com wrote:

Any progress? Reminder: there's a PR available, which needs testing and review. It fixes a major bug.

I wonder if the @backdrop-contrib/bug-squad https://github.com/orgs/backdrop-contrib/teams/bug-squad team could help here?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/backdrop-contrib/metatag/issues/56#issuecomment-722197019, or unsubscribe https://github.com/notifications/unsubscribe-auth/AADBERZH7QRMS72X4SW7R6TSOJH47ANCNFSM4SHIUVHQ .

jenlampton commented 3 years ago

@indigoxela the PR looks good. Testing the group isn't ideal, but it fixes the issue so I've merged the PR. :)

backdrop-contrib / metatag

Wrong meta description markup #56