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.
Comment | File | Size | Author |
---|---|---|---|
#66 | 1970362-66-reroll-of-64.patch | 26.74 KB | sonnykt |
#64 | 1970362-64-reroll-of-58.patch | 26.76 KB | wiifm |
#60 | metatag-n1970362-60.patch | 23.62 KB | DamienMcKenna |
Comments
Comment #1
xtfer CreditAttribution: xtfer commentedPatch attached.
Comment #2
DamienMcKennaThis 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.
Comment #3
xtfer CreditAttribution: xtfer commentedHappy 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?
Comment #4
DamienMcKennaYou'll also need to work on the default field handling in metatag.admin.js.
Comment #5
DamienMcKennaCorrection, metatag.vertical-tabs.js is the file that needs work to handle the new default values.
Comment #6
xtfer CreditAttribution: xtfer commentedYep, found it. There's a couple of other minor adjustments to be made as well, in other parts of the patch. Coming soon...
Comment #7
nick_schuch CreditAttribution: nick_schuch commentedThe 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.
Comment #8
nick_schuch CreditAttribution: nick_schuch commentedThe following patch addresses the remaining requirements outlined in #7
Comment #9
xtfer CreditAttribution: xtfer commentedThis 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
Comment #10
DamienMcKennaThe 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.
Comment #11
DamienMcKennaFor 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:
Comment #12
xtfer CreditAttribution: xtfer commentedI 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.
That I can do.
Comment #13
xtfer CreditAttribution: xtfer commentedComment #14
xtfer CreditAttribution: xtfer commentedThis patch:
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
Comment #15
DamienMcKennaI've thought about it some more and am willing to commit it, with a few small adjustments.
Comment #16
DamienMcKennaOf course it helps if I generate the patch correctly ;-)
Comment #17
DamienMcKennaCommitted. Thanks xtfer!
Comment #18
xtfer CreditAttribution: xtfer commentedVery good. Thanks DamienMcKenna.
Comment #19
DamienMcKennaI'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.
Comment #20
DamienMcKennaComment #21
xtfer CreditAttribution: xtfer commentedWe hadn't noticed that as a problem... what's it supposed to do?
Comment #22
DamienMcKennaI'm sorry but I've had to revert this patch until the problems can be resolved.
Comment #23
xtfer CreditAttribution: xtfer commentedSure, and if you can describe the problem in a bit more detail, I can look at resolving it.
Comment #24
DamienMcKennaThe 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.
Comment #25
xtfer CreditAttribution: xtfer commentedI thought we'd fixed that. Nevermind, I'll take a look.
Comment #26
larowlanre-rolling
Comment #29
larowlanComment #30
larowlanComment #31
larowlanhere's a test for metatag_filter_values_from_defaults()
and then another one with this patch - should fail
Comment #33
larowlanFixes the defaults issue (test coverage attached).
Adds test coverage as follows:
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
Comment #34
larowlanComment #35
DamienMcKenna@larowlan: I don't suppose you'd willing to move the additional tests into #1848338: Add more tests?
Comment #36
DamienMcKenna@larowlan: PS, a *HUGE* thank-you for the additional tests!
Comment #37
larowlan@DamienMcKenna done :)
Comment #38
xtfer CreditAttribution: xtfer commentedI'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.
Comment #40
DamienMcKennaThis is really close. A few small things:
Comment #41
kim.pepperComment #42
kim.pepperA re-roll and addresses 2/3 issues in #40
Comment #44
kim.pepperRenames MetaTagsTestHelper to MetaTagTestHelper.
Comment #46
DamienMcKennaFYI "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".
Comment #48
mstrelan CreditAttribution: mstrelan commentedHere is a reroll of #44 with the test moved to a separate file in the tests directory.
Comment #51
Feng-Shui CreditAttribution: Feng-Shui commentedRe-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.
Comment #52
Feng-Shui CreditAttribution: Feng-Shui commentedComment #54
Feng-Shui CreditAttribution: Feng-Shui commentedGetting the patch to apply would be a good start...
Comment #56
Feng-Shui CreditAttribution: Feng-Shui commentedExtended 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...
Comment #57
Josh Waihi CreditAttribution: Josh Waihi at Acquia commentedThanks @Feng-Shui!
Comment #58
DamienMcKennaRerolled.
Comment #60
DamienMcKennaWIP on moving the functionality into a separate module that can be turned on / off.
Comment #62
DamienMcKennaComment #63
DamienMcKennaI'll continue working on this.
Comment #64
wiifmReroll of #58 (before the module split)
Comment #65
tobybellwood CreditAttribution: tobybellwood at govCMS (Australian Government Department of Finance) for govCMS (Australian Government Department of Finance) commentedThe 58/64 patch is broken when applying to 7.x-1.25 release - will have to investigate a re-roll...
Comment #66
sonnyktReroll of #64 for 1.25.
Comment #67
govind.maloo CreditAttribution: govind.maloo at Salsa Digital commented@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.