Like the D7 version, the D8 version needs to support meta tags that allow multiple values. The attribute 'multiple' was added already (#2613654: Parse image fields for image URLs), now we just need to generally support it.

CommentFileSizeAuthor
#32 interdiff-28-32.txt6.26 KBidebr
#32 2628934-32.patch4.92 KBidebr
#28 metatag-n2628934-25.patch3.27 KBe.escribano
#25 metatag-n2628934.patch2.32 KBe.escribano
#23 interdiff-2628934-20-23.txt1.18 KBsmaz
#23 metatag-n2628934-23.patch3.56 KBsmaz
#20 interdiff-2628934-17-20.txt879 bytessmaz
#20 metatag-n2628934-20.patch2.27 KBsmaz
#17 metatag-n2628934-17.patch2.18 KBDamienMcKenna
#14 2628934-14.patch2.33 KBnikunjkotecha
#9 2628934-9.patch2.17 KBnikunjkotecha
#8 2628934-8.patch2.17 KBnikunjkotecha
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

DamienMcKenna created an issue. See original summary.

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna at Mediacurrent commented
Related issues: +#2613654: Parse image fields for image URLs
mr.baileys’s picture

mr.baileys
Primary language Dutch
Location 🇧🇪 (Ghent)
CreditAttribution: mr.baileys at Calibrate commented
Related issues: +#2664062: 'multiple' option not working correctly, e.g. og:see_also

Marked #2664062: 'multiple' option not working correctly, e.g. og:see_also as duplicate.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented

I proposed #2589203: Explore moving to one field item per metatag instead of a serailized blob for this, but it was voted down

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna at Mediacurrent commented

@larowlan: I was planning to follow D7's lead by just splitting on a comma, which would automatically make it compatible with tokens that return multiple values.

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna at Mediacurrent commented
Issue tags: +Needs tests

Some support already exists, need to add tests to cover all meta tags that support multiple values.

nikunjkotecha’s picture

nikunjkotecha
he/him
Primary language Gujarati
Location India, Gujarat, Rajkot
CreditAttribution: nikunjkotecha as a volunteer and at Axelerant commented

I'm trying to use og_locale_alternative but not able to get it working for multiple values, it simply shows one with all values, any idea if this is fixed already in dev version?

nikunjkotecha’s picture

nikunjkotecha
he/him
Primary language Gujarati
Location India, Gujarat, Rajkot
CreditAttribution: nikunjkotecha as a volunteer and at Axelerant commented
FileSize
2628934-8.patch2.17 KB

I checked dev version and don't see any code for the support, adding patch to allow support for property based meta tags.

nikunjkotecha’s picture

nikunjkotecha
he/him
Primary language Gujarati
Location India, Gujarat, Rajkot
CreditAttribution: nikunjkotecha as a volunteer and at Axelerant commented
FileSize
2628934-9.patch2.17 KB

Corrected output.

nikunjkotecha’s picture

nikunjkotecha
he/him
Primary language Gujarati
Location India, Gujarat, Rajkot
CreditAttribution: nikunjkotecha as a volunteer and at Axelerant commented
DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna at Mediacurrent commented
Status: Active » Needs review

@nikunjkotecha: Thanks for putting that together! Lets see what the testbot says.

Status: Needs review » Needs work

The last submitted patch, 9: 2628934-9.patch, failed testing.

nikunjkotecha’s picture

nikunjkotecha
he/him
Primary language Gujarati
Location India, Gujarat, Rajkot
CreditAttribution: nikunjkotecha as a volunteer and at Axelerant commented

Hi Damien, seems for the change in MetatagManager related in failure; either we update the code to avoid suffixing with index for tags that are not multiple or update test case, I'm not able to confirm if other failures are because of this change, but I'll work on them tonight and keep the post updated. Please update here if you find some useful info.

nikunjkotecha’s picture

nikunjkotecha
he/him
Primary language Gujarati
Location India, Gujarat, Rajkot
CreditAttribution: nikunjkotecha as a volunteer and at Axelerant commented
Status: Needs work » Needs review
FileSize
2628934-14.patch2.33 KB

Refactoring to get it passing in tests.

Status: Needs review » Needs work

The last submitted patch, 14: 2628934-14.patch, failed testing.

flyke’s picture

flyke CreditAttribution: flyke commented

This patch cannot be applied against the latest dev version (8.x-1.x-dev)

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna at Mediacurrent commented
Status: Needs work » Needs review
FileSize
metatag-n2628934-17.patch2.18 KB

An attempted reroll.

Status: Needs review » Needs work

The last submitted patch, 17: metatag-n2628934-17.patch, failed testing. View results

smaz’s picture

smaz
Primary language English
Location England, UK
CreditAttribution: smaz at Five Mile Media commented

I've tried using the latest patch from #17, but this is just giving me empty tags for everything - just the opening & closing bracket.

I am testing with 8.x-1.2 on Drupal 8.3.5, but I've checked and the only changes between that & dev are modifications to tests - nothing that should affect this.

I'm stepping through things now to see if I can work out the cause.

smaz’s picture

smaz
Primary language English
Location England, UK
CreditAttribution: smaz at Five Mile Media commented
FileSize
metatag-n2628934-20.patch2.27 KB
interdiff-2628934-17-20.txt879 bytes

I think there was too much nesting of arrays returned by MetatagManager::generateRawElements().

I've tweaked the patch from #17 slightly to reduce a level of nesting & everything appears to work as it should.

smaz’s picture

smaz
Primary language English
Location England, UK
CreditAttribution: smaz at Five Mile Media commented
Status: Needs work » Needs review

Updating status.

Status: Needs review » Needs work

The last submitted patch, 20: metatag-n2628934-20.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

smaz’s picture

smaz
Primary language English
Location England, UK
CreditAttribution: smaz at Five Mile Media commented
Status: Needs work » Needs review
FileSize
metatag-n2628934-23.patch3.56 KB
interdiff-2628934-20-23.txt1.18 KB

The test failures were because MetaItempropBase needed to handle multiple values too. Patch fixes that.

I also noticed that MetaItempropBase didn't check for replacing secure URLs, so I've added that in too (though maybe that should be a different patch?).

I'm not sure if this is enough though - do all of the base classes need this? And then potentially all of the classes that extend & overwrite this?

Status: Needs review » Needs work

The last submitted patch, 23: metatag-n2628934-23.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

e.escribano’s picture

e.escribano CreditAttribution: e.escribano commented
FileSize
metatag-n2628934.patch2.32 KB

I think if the changes are applied to the class MetaNameBase directly, any of the extended classes will handle multiple values.

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna at Mediacurrent commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 25: metatag-n2628934.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

e.escribano’s picture

e.escribano CreditAttribution: e.escribano commented
FileSize
metatag-n2628934-25.patch3.27 KB

New patch

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna at Mediacurrent commented
Status: Needs work » Needs review

Thanks.

BTW please try to remember to change the status to "needs review" after you upload a patch.

That last patch fails again.

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna at Mediacurrent commented
Status: Needs review » Needs work
ZapevalovAnton’s picture

ZapevalovAnton CreditAttribution: ZapevalovAnton as a volunteer commented

Patch #25 not work for me. Metatag 8.x-1.x-dev

idebr’s picture

idebr CreditAttribution: idebr at ezCompany commented
Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
2628934-32.patch4.92 KB
interdiff-28-32.txt6.26 KB

#28 introduced some code duplication over different base classes. Not sure why this was necessary. The 7.x Metatag makes no such distinction, so I moved this code back to MetaNameBase.

Attached patch introduces the following changes:
- Moved support for multiple values back to MetaNameBase
- Added test coverage for an og:image:url metatag with multiple values
- Rerolled against 8.x-1.x

caspervoogt’s picture

caspervoogt CreditAttribution: caspervoogt at Plethora commented

the patch from #32 works well for me

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna at Mediacurrent commented
Parent issue: » #2957104: Plan for Metatag 8.x-1.6

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna at Mediacurrent commented
Status: Needs review » Fixed

Committed. Thanks everyone!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.