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.

Comments

xtfer’s picture

Status:Active» Needs review
StatusFileSize
new17.92 KB
PASSED: [[SimpleTest]]: [MySQL] 21 pass(es).
[ View ]

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

StatusFileSize
new21.82 KB
PASSED: [[SimpleTest]]: [MySQL] 73 pass(es).
[ View ]
new8.05 KB

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
StatusFileSize
new23.71 KB
PASSED: [[SimpleTest]]: [MySQL] 73 pass(es).
[ View ]
new5.18 KB

The following patch addresses the remaining requirements outlined in #7

xtfer’s picture

StatusFileSize
new7.63 KB
new31.13 KB
PASSED: [[SimpleTest]]: [MySQL] 73 pass(es).
[ View ]

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
StatusFileSize
new2.29 KB
new30.34 KB
PASSED: [[SimpleTest]]: [MySQL] 73 pass(es).
[ View ]

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

StatusFileSize
new0 bytes
PASSED: [[SimpleTest]]: [MySQL] 73 pass(es).
[ View ]

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

DamienMcKenna’s picture

StatusFileSize
new29.58 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch metatag-n1970362-16.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

larowlan queued 16: metatag-n1970362-16.patch for re-testing.

Status:Needs review» Needs work

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

larowlan’s picture

Status:Needs work» Needs review
StatusFileSize
new30.87 KB
PASSED: [[SimpleTest]]: [MySQL] 73 pass(es).
[ View ]
larowlan’s picture

Assigned:larowlan» Unassigned
larowlan’s picture

StatusFileSize
new3 KB
PASSED: [[SimpleTest]]: [MySQL] 97 pass(es).
[ View ]
new33.87 KB
FAILED: [[SimpleTest]]: [MySQL] 91 pass(es), 13 fail(s), and 1 exception(s).
[ View ]

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

StatusFileSize
new11.32 KB
new37.97 KB
PASSED: [[SimpleTest]]: [MySQL] 105 pass(es).
[ View ]
new9.29 KB

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.