contao / core-bundle

[READ-ONLY] Contao Core Bundle
GNU Lesser General Public License v3.0
123 stars 58 forks source link

Do not make TL_HEAD tags unique #1214

Closed qzminski closed 6 years ago

qzminski commented 6 years ago

The TL_HEAD tags that are being added to the page are first filtered out of duplicates. This is could be a problem for OpenGraph tags where you can define the arrays – http://ogp.me/#array.

https://github.com/contao/core-bundle/blob/master/src/Resources/contao/library/Contao/Controller.php#L898

So if you would have two images and if their dimensions would be the same the output code should look like this:

<meta property="og:image" content="http://example.com/rock.jpg" />
<meta property="og:image:width" content="300" />
<meta property="og:image:height" content="300" />
<meta property="og:image" content="http://example.com/rock2.jpg" />
<meta property="og:image:width" content="300" />
<meta property="og:image:height" content="300" />

But because the array_unique is called, some of the image size tags would be stripped:

$GLOBALS['TL_HEAD'][] = '<meta property="og:image" content="http://example.com/rock.jpg" />';
$GLOBALS['TL_HEAD'][] = '<meta property="og:image:width" content="300" />';
$GLOBALS['TL_HEAD'][] = '<meta property="og:image:height" content="300" />';
$GLOBALS['TL_HEAD'][] = '<meta property="og:image" content="http://example.com/rock2.jpg" />';
$GLOBALS['TL_HEAD'][] = '<meta property="og:image:width" content="300" />'; # will be dropped
$GLOBALS['TL_HEAD'][] = '<meta property="og:image:height" content="300" />'; # will be dropped

Contao 3 is affected as well. Also see https://github.com/codefog/contao-social_images/issues/19

backbone87 commented 6 years ago

Imho you should add all 3 tags for one Image into one TL_HEAD entry

m-vo commented 6 years ago

Allthough I agree with @backbone87 's comment, why do we use array_unique here? Was this a fix for something else?

Regarding og/social images, it would be nice to have some abstraction for that. Contao modules itself add og tags in some places (e.g. image and description on a newsreader detail site) but the core does not support it by default in the BE (and that might be a good decisison). This is leaving opengraph extensions with the need to regex/tokenize the existing entries to eventually alter/overwrite them.

discordier commented 6 years ago

The array_unique is there because of modules and content elements adding tags multiple times. Imagine 5 elements on the same page, each adding a certain head tag. These tags (usually some script) should only be added once. Therefore we can not change this without having a big bc break.

fritzmg commented 6 years ago

Regarding og/social images, it would be nice to have some abstraction for that. Contao modules itself add og tags in some places (e.g. image and description on a newsreader detail site) but the core does not support it by default in the BE (and that might be a good decisison). This is leaving opengraph extensions with the need to regex/tokenize the existing entries to eventually alter/overwrite them.

There are some plans: https://github.com/contao/core/issues/8787#issuecomment-334469447

qzminski commented 6 years ago

@backbone87 that's a good idea, I will use it!

@discordier at some point that is true but if you have N elements on the same page, it's likely that each of the scripts should check and overwrite a certain head tag. That's not the case here because for the array_unique() function always the first occurrence of doubled item is preserved and the next are dropped. This might be a topic for a different discussion though.

leofeyer commented 6 years ago

I don't think we can do much about it; this is how array_unique() works. And as @discordier explained, we cannot stop using it. You have to stick with @backbone87's solution.