The meta element supports several attributes which metatag currently ignores, including "scheme", "lang" and "dir".

These attributes are required for the proper output of Dublin Core metadata and other metadata standards, including AGLS.

This patch adds support for these elements using a new class, DrupalExtendedMetaTag, which implements these attributes. It provides an extended form for elements which can benefit from the properties, and translates values and attributes for rendering.

In addition, I've patched the DublinCore metadata set to use the new extended tag.

What this patch probably needs is a update hook in metatag_dc.install to update existing tags, however I'm not sure the best approach for that. If someone can suggest one, I'm happy to take a shot at that.

CommentFileSizeAuthor
#66 1970362-66-reroll-of-64.patch26.74 KBsonnykt
#64 1970362-64-reroll-of-58.patch26.76 KBwiifm
#60 metatag-n1970362-60.patch23.62 KBDamienMcKenna
#60 metatag-n1970362-60.interdiff.txt28.88 KBDamienMcKenna
#58 metatag-n1970362-58.patch26.81 KBDamienMcKenna
#56 1970362-56-extended-metatags.patch35.32 KBFeng-Shui
#54 1970362-54-extended-metatags.patch35.38 KBFeng-Shui
#51 1970362-51-extended-metatags.patch35.97 KBFeng-Shui
#48 1970362-48-extended-metatags.patch25.13 KBmstrelan
#44 1970362-all-attrs-44.patch24.42 KBkim.pepper
#44 interdiff.txt323 byteskim.pepper
#42 1970362-all-attrs-42.patch24.42 KBkim.pepper
#33 Screenshot 2014-10-14 11.15.09.png9.29 KBlarowlan
#33 support-all-attributes-1970362.pass_.patch37.97 KBlarowlan
#33 interdiff.txt11.32 KBlarowlan
#1 1970362-support-all-attributes.patch17.92 KBxtfer
#7 1970362-7-support-all-attributes.patch21.82 KBnick_schuch
#7 interdiff.txt8.05 KBnick_schuch
#8 1970362-8-support-all-attributes.patch23.71 KBnick_schuch
#8 interdiff.txt5.18 KBnick_schuch
#9 interdiff.txt7.63 KBxtfer
#9 1970362-9-support-all-attributes.patch31.13 KBxtfer
#14 interdiff.txt2.29 KBxtfer
#14 1970362-14-support-all-attributes.patch30.34 KBxtfer
#15 metatag-n1970362-15.patch0 bytesDamienMcKenna
#16 metatag-n1970362-16.patch29.58 KBDamienMcKenna
#29 support-all-attributes-1970362.29.patch30.87 KBlarowlan
#31 support-all-attributes-1970362.fail_.patch33.87 KBlarowlan
#31 metatag-defaults.patch3 KBlarowlan
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

xtfer’s picture

Status: Active » Needs review
FileSize
17.92 KB

Patch attached.

DamienMcKenna’s picture

Status: Needs review » Needs work

This is a great initial pass, but it needs some further work. If the 'direction' option only has three options ('ltr', 'rtl', empty) then it needs to be a selector. The 'scheme' option also needs some further work as there's no indication given for what it should be, and per the specs there may be specific connections between the 'schema' values and the 'name' values that could be further improved.

xtfer’s picture

Happy to add a selector for the 'dir' property, however the use of 'scheme' is essentially arbitrary.

The HTML specification does not provide any guidance on how 'scheme; is to be used, it is only for DC (in this instance) that there is guidance, but in that case its standard-specific. We could provide additional help text for DC values using this element, however I wouldn't want to hold this functionality up for what is essentially help text for a given implementation.

Thoughts?

DamienMcKenna’s picture

You'll also need to work on the default field handling in metatag.admin.js.

DamienMcKenna’s picture

Correction, metatag.vertical-tabs.js is the file that needs work to handle the new default values.

xtfer’s picture

Yep, found it. There's a couple of other minor adjustments to be made as well, in other parts of the patch. Coming soon...

nick_schuch’s picture

The following patch:

- Update metatag_dc defaults.
- Fixes form handling for the extended metatag.
- Filters out the extended metatag from the default check (for now).
- Dropdown for rtl and ltr setting.

The following needs to be discussed:

- Backwards compatibility. We need to handle old tags that we single "value" items opposed to a new array.
- How we should display this extended tag in the vertical tabs.

nick_schuch’s picture

Status: Needs work » Needs review
FileSize
23.71 KB
5.18 KB

The following patch addresses the remaining requirements outlined in #7

xtfer’s picture

This should be the final patch on this issue...

This resolves some outstanding issues Nick and I found in working with this latest patch.

- An extra "value" key creeping into extended tags
- Returning the correct values during migrations
- Updating any incorrect DublinCore properties
- Coding standards on our own work

DamienMcKenna’s picture

The functionality in its current for is very complete, so kudos for that.

However, given that, as you yourself mention, these are for attributes of the normal <meta> tag, might it not be worthwhile splitting off the functionality into a "Metatag: Extended attributes" submodule that uses hook_metatag_info_alter() to replace uses of the DrupalTextMetaTag class with DrupalExtendedMetaTag? Then all of the meta tags could take advantage of it, not just the Dublin Core meta tags.

DamienMcKenna’s picture

For completeness, some references about the new attributes:

As you said, there isn't anything in the specs about how 'scheme' should be used, though the XHTML 1.0 Strict specs do at least list it.

Two requested minor improvements:

  • The tidy-up code should be replaced with a call to tidyValue(), see the latest -dev codebase for details.
  • The 'scheme' attribute needs a description, if only to say e.g. "This field is rarely used and should only be used when specifically instructed to do so."
xtfer’s picture

However, given that, as you yourself mention, these are for attributes of the normal tag, might it not be worthwhile splitting off the functionality into a "Metatag: Extended attributes" submodule that uses hook_metatag_info_alter() to replace uses of the DrupalTextMetaTag class with DrupalExtendedMetaTag? Then all of the meta tags could take advantage of it, not just the Dublin Core meta tags.

I don't believe the extended functionality is valid for ALL tags, only those which are effectively supported by ontologies. That was the initial logic behind a separate tag type. Splitting it into a separate module seems like overkill to me though, and would effectively create two types of DC metatags (simple and extended). Some of those simple ones would actually be invalid if the extended module wasn't enabled. The corollary, is that if it is enabled for ALL tags, then that would just add extra UI craft that was of no use for some items anyway.

Also, this work has been so far been included in client projects, which are wrapping up, so I'm not sure when we'd get the time to rework and test as thoroughly as we have. The functionality works as is, and has been tested in production.

- The tidy-up code should be replaced with a call to tidyValue(), see the latest -dev codebase for details.
- The 'scheme' attribute needs a description, if only to say e.g. "This field is rarely used and should only be used when specifically instructed to do so."

That I can do.

xtfer’s picture

Status: Needs review » Needs work
xtfer’s picture

Status: Needs work » Needs review
FileSize
2.29 KB
30.34 KB

This patch:

  • Uses the tidyValue() method
  • Improves descriptions for fields in extended tags

I don't think I'll be able to rework this to use alter's rather than its current functionality, since that would amount to a rewrite. It would be good to get this in, as its currently blocking a release of the AGLS project.

Thanks

DamienMcKenna’s picture

FileSize
0 bytes

I've thought about it some more and am willing to commit it, with a few small adjustments.

DamienMcKenna’s picture

FileSize
29.58 KB

Of course it helps if I generate the patch correctly ;-)

DamienMcKenna’s picture

Status: Needs review » Fixed

Committed. Thanks xtfer!

xtfer’s picture

Very good. Thanks DamienMcKenna.

DamienMcKenna’s picture

I've identified a problem with this - the metatag_filter_values_from_defaults() handling doesn't work, so all records end up with the nested array of values settings.

DamienMcKenna’s picture

Status: Fixed » Needs work
xtfer’s picture

We hadn't noticed that as a problem... what's it supposed to do?

DamienMcKenna’s picture

I'm sorry but I've had to revert this patch until the problems can be resolved.

xtfer’s picture

Sure, and if you can describe the problem in a bit more detail, I can look at resolving it.

DamienMcKenna’s picture

The main problem is that metatag_filter_values_from_defaults() can't handle array values, neither can the JS that tracks the field changes for the vertical tabs.

xtfer’s picture

The main problem is that metatag_filter_values_from_defaults() can't handle array values, neither can the JS that tracks the field changes for the vertical tabs.

I thought we'd fixed that. Nevermind, I'll take a look.

larowlan’s picture

Assigned: Unassigned » larowlan
Issue summary: View changes

re-rolling

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 16: metatag-n1970362-16.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review
FileSize
30.87 KB
larowlan’s picture

Assigned: larowlan » Unassigned
larowlan’s picture

here's a test for metatag_filter_values_from_defaults()
and then another one with this patch - should fail

The last submitted patch, 31: support-all-attributes-1970362.fail_.patch, failed testing.

larowlan’s picture

Fixes the defaults issue (test coverage attached).
Adds test coverage as follows:

  • Creates a new node type
  • Logs in admin user who can administer meta tags and create nodes in that content type
  • Adds a new metatag config for the new node-type
  • Makes sure default values for dcterms.title is inherited from default config
  • Saves some token values for dcterms tags
  • Re-edits config to ensure values stuck
  • Creates new node of given type
  • Validates that defaults values are correct
  • Sets override for robots, dcterms.title and dcterms.title direction (ltr)
  • Saves the node and verifies that defaults are stripped (only overrides are saved - this was the original issue with the earlier patch)
  • Verifies that the dcterms.title and robots tags appear in the page with the correct values.
  • Verifies that the extended metatag dir attribute is output as expected

From what I can see this expands test coverage for the core metatags module significantly

Manual testing verified that the vertical tabs JavaScript is working - screenshot below

larowlan’s picture

DamienMcKenna’s picture

@larowlan: I don't suppose you'd willing to move the additional tests into #1848338: Add more tests?

DamienMcKenna’s picture

@larowlan: PS, a *HUGE* thank-you for the additional tests!

larowlan’s picture

@DamienMcKenna done :)

xtfer’s picture

Status: Needs review » Reviewed & tested by the community

I've done a follow-up manual test using the AGLS module. On top of the automated test results, I can edit the new metatag elements and see them rendered correctly once saved. The summary while editing is correct.

Setting this to RTBC, as I believe it corrects the issues with my original patch.

DamienMcKenna’s picture

Status: Reviewed & tested by the community » Needs work

This is really close. A few small things:

  • Please streamline metatag_dc_metatag_info() by adding a $default array which is then appended to each meta tag definition.
  • I see that the Migrate support was updated - thanks for that. Was the Feeds integration tested?
  • Some of the default values in metatag_dc_metatag_bundled_config_alter() were inadvertently removed.
kim.pepper’s picture

Issue tags: +Need reroll
kim.pepper’s picture

Status: Needs work » Needs review
Issue tags: -Need reroll
FileSize
24.42 KB

A re-roll and addresses 2/3 issues in #40

  1. Done
  2. Not tested
  3. Done

Status: Needs review » Needs work

The last submitted patch, 42: 1970362-all-attrs-42.patch, failed testing.

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
323 bytes
24.42 KB

Renames MetaTagsTestHelper to MetaTagTestHelper.

Status: Needs review » Needs work

The last submitted patch, 44: 1970362-all-attrs-44.patch, failed testing.

DamienMcKenna’s picture

FYI "MetaTagTestHelper" was renamed to "MetatagTestHelper", and the tests were recently extended so you'll need to update accordingly.

You can run the tests locally by enabling the simpletest module and then running "drush test-run Metatag".

  • DamienMcKenna committed 61b0855 on 8.x-1.x authored by xtfer
    Issue #1970362 by xtfer, DamienMcKenna: Added new meta tag type for...
  • DamienMcKenna committed f890447 on 8.x-1.x
    Reverted #1970362 xtfer, DamienMcKenna: Added new meta tag type for...
mstrelan’s picture

Here is a reroll of #44 with the test moved to a separate file in the tests directory.

Status: Needs review » Needs work

The last submitted patch, 48: 1970362-48-extended-metatags.patch, failed testing.

The last submitted patch, 48: 1970362-48-extended-metatags.patch, failed testing.

Feng-Shui’s picture

Re-roll against 1.6 version.

1: Updated metatag_translate() to metatag_translate_metatag()
2: Updated a bunch of tests (to get them passing) to support the new nested structure used by DC.
3: Also tweaked metatag_panels_ctools_render_alter() to support the new nested structure.

I've not tested any of the above manually yet, just working to get the patch passing tests.

Feng-Shui’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 51: 1970362-51-extended-metatags.patch, failed testing.

Feng-Shui’s picture

Status: Needs work » Needs review
FileSize
35.38 KB

Getting the patch to apply would be a good start...

Status: Needs review » Needs work

The last submitted patch, 54: 1970362-54-extended-metatags.patch, failed testing.

Feng-Shui’s picture

Extended tag getValue method was constructing a name variable to pass through to metatag_translate_metatag(). Looks like we can just pass through the tag name without issue, as per the other tag controllers. Didn't result in any regressions in MetatagCoreExtendedTest. Maybe a pass this time...

Josh Waihi’s picture

Status: Needs review » Reviewed & tested by the community

Thanks @Feng-Shui!

DamienMcKenna’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
26.81 KB

Rerolled.

Status: Needs review » Needs work

The last submitted patch, 58: metatag-n1970362-58.patch, failed testing.

DamienMcKenna’s picture

Status: Needs work » Needs review
FileSize
28.88 KB
23.62 KB

WIP on moving the functionality into a separate module that can be turned on / off.

Status: Needs review » Needs work

The last submitted patch, 60: metatag-n1970362-60.patch, failed testing.

DamienMcKenna’s picture

I'll continue working on this.

wiifm’s picture

Reroll of #58 (before the module split)

tobybellwood’s picture

The 58/64 patch is broken when applying to 7.x-1.25 release - will have to investigate a re-roll...

sonnykt’s picture

govind.maloo’s picture

@sonnykt : Please make sure to submit interdiff so every time we don't need to review whole patch.

This require more work as last submitted patch failed test.