Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Allow array values for meta tags #600

Closed
wants to merge 2 commits into from
Closed

Allow array values for meta tags #600

wants to merge 2 commits into from

Conversation

BlackWiCKED
Copy link

@BlackWiCKED BlackWiCKED commented Sep 27, 2021 •
edited by core23
Loading

Subject

  1. Updating addMeta() function to allow multiple values for specific meta tags: og:image
$page->addMeta('property', 'og:image', 'http://example.com/image1.jpg');
$page->addMeta('property', 'og:image', 'http://example.com/image2.jpg');

Renders as:

<meta property="og:image" content="http://example.com/image1.jpg">
<meta property="og:image" content="http://example.com/image2.jpg">
  1. Utilize the $extra parameter to define structured properties when their key starts with :
$page->addMeta('property', 'og:image', 'http://example1.com/image1.jpg', [":width" => 640, ":height" => 480]);

Renders as:

<meta property="og:image" content="http://example.com/image1.jpg">
<meta property="og:image:width" content="640">
<meta property="og:image:height" content="480">

I am targeting this branch, because it's a missing feature and it should be available in 2.x too.

Closes #554

Changelog

### Added
- Extra parameters starting with a colon symbol (`:`) are now rendered as structured properties

### Changed
- Allow calling `SeoPage::addMeta()` with `og:image` multiple times

core23
core23 reviewed Sep 28, 2021
View reviewed changes
src/Seo/SeoPage.php Outdated Show resolved Hide resolved
core23
core23 reviewed Sep 28, 2021
View reviewed changes
src/Twig/Extension/SeoExtension.php
$this->normalize($name),
$this->normalize($content)
);
if (\is_array($content)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method has a large code complexity and is hard to understand. Can you try to extra a few methods.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can pass an $extra variable to generateMetaTag() method as a new property, then the foreach would not be duplicated. The structured properties (additional tags) would be generated from the inside as a recursive function.
Or should I simply introduce a new generateMetaTagExtra() method and call it?

@VincentLanglet
Copy link
Member

Wouldnt it better to say that the add method always has the same behavior to support array values for every possible tags ?

And if needed we can introduce later a reset/replace/set/... method.

@jordisala1991
Copy link
Member

Wouldnt it better to say that the add method always has the same behavior to support array values for every possible tags ?

And if needed we can introduce later a reset/replace/set/... method.

I am not sure if all possible metatag allows multiple values. I guess not all of them do.

@VincentLanglet
Copy link
Member

Even if they dont, it would be better if you dont need to know if they do when you use the add method to determine the behavior you'll get.

Two different methods might be better

@BlackWiCKED
Copy link
Author

@VincentLanglet In my PR I introduced a constant (ALLOW_MULTIPLE_TAGS) where we can define which metatags shall accept array values. For now, I only added og:image, but I presume there are plenty more. Some of them are defined in https://ogp.me/#array but probably there are more meta tags (other than Open Graph) that allow multiple values. Just for the record, I don't support the idea of enabling this behavior for all tags. If that's what you aiming for, then this PR is obsolete, and there should be another one that stores the metadata in arrays by default all the time, and not just when we add more than one.

@core23 I understand the problem with the code complexity - and it's mainly because of the two options how we store the metadata. Most of the time it will be a string as it was before though - not much was added there. The real new feature is rendering structured data through the $extra variable - it can generate additional meta tags under the root node.

VincentLanglet
VincentLanglet reviewed Sep 29, 2021
View reviewed changes
Copy link
Member

@VincentLanglet VincentLanglet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TLDR: This feature might seems easy but could introduce multiple inconsistency issues and BC-break to me. Might be simplier to think about a plan, add deprecation in this branch and finish it in next major. Good news the next major will be released soon.

Looking at the method:

  • setMetas is resetting the metas and then calling addMeta with every value.
  • addMeta is supposed to add a value, without replacing the previous one when the value is in this special array of "multiple values allowed". When it's not it will replace the value.
  • a setMeta method could be introduce to reset and set a new value but it's basically removeMeta + addMeta.

The new strategy could add some issues:

  • First adding/removing a key to this new constant is a BC-break ; because we're changing the behavior of the addMeta method.
  • The addMeta won't have a consistent behavior depends of the tag used.
  • The addMeta won't have a consistent behavior with all add method like addHtmlAttributes, addHeadAttribute (you can also notice the lack of consistency with the s).
  • Also, you're checking the name but your usage is only in the case of a type property
  • Since you're changing the structure of the metas, the call setMetas(getMetas()) is not possible anymore. foreach ($metaItem as $name => $meta) { can give us array of array.

I don't like the add name to set a value. Renaming
addHtmlAttributes to setHtmlAttribute, etc, would make sens.
then removeHtmlAttributes to unsetHtmlAttribute, etc, would make sens too.

The addMeta would be deprecated to a setMeta method which ALWAYS override the value.
And a new addMeta could be introduce and ALWAYS add the value to the existing array.

Also, it would be simplier to say that a meta is ALWAYS an array of array to avoid always checking if it's a string or an array and just doing a foreach on it.

@VincentLanglet VincentLanglet mentioned this pull request Oct 7, 2021
Merged
@github-actions
Copy link

github-actions bot commented Apr 1, 2022

This PR has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale label Apr 1, 2022
@github-actions github-actions bot closed this Apr 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@VincentLanglet VincentLanglet VincentLanglet left review comments

@core23 core23 core23 left review comments

Assignees
No one assigned
Labels
pending author stale
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow array values for meta tags
4 participants
@BlackWiCKED @VincentLanglet @jordisala1991 @core23