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.

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

CanonicalLink->prepForRender() - noindex detection fails for multiple meta robots values #1378

Closed
Abromeit opened this issue Nov 8, 2023 · 5 comments
Labels

Comments

@Abromeit
Copy link

Abromeit commented Nov 8, 2023

Bug description

I have noticed that seomatic does correctly avoid issuing a canonical tag if a post is set to meta robots=noindex or meta robots=none. This is the expected behavior.

In addition, one would expect that a combined value of "noindex, nofollow" would have the same effect on the generated HTML as the value "none". That is, no canonical tag should be output.

However, if you assign more than one value to the meta robots field, e.g.

  • "noindex, nofollow",
  • "max-snippet:-1,noindex"
  • "noarchive,noindex,notranslate"

a canonical tag will be rendered in the HTML.

example-values

To reproduce

Steps to reproduce the behaviour:

  1. Make sure you're on a craft live environment or enforce the environment in the respecitve seomatic setting
  2. Make sure the seomatic setting Always include canonical links regardless of environment is inactive / false
  3. Create a new post / entry in your cms
  4. Goto the post's SEO settings and add 2 or more values for the robots metatag
  5. Save and publish the post
  6. Inspect the frontend HTML result

Versions

  • Plugin version: v4.0.33
  • Craft version: v4.5.9

Presumed cause

In src/models/metalink/CanonicalLink.php the prepForRender() method expects the return value of $robotsArray['content'] to be an array, if multiple values were entered.

  • $robotsArray = $robots->renderAttributes();
    $content = $robotsArray['content'] ?? '';
    if (!empty($content)) {
    if (\is_array($content)) {
    if (\in_array('noindex', $content, true) || \in_array('none', $content, true)) {
    return false;
    }

On my test instance i just var_dumped the value of $robotsArray['content'] and it is a string, like one that would also be used for the final output. (Like "noindex,nofollow".)

To solve this, you could probably use explode() on the string. But I suspect that there is already a more suitable solution in this project. For example, a method that can actually still return the raw array.

@Abromeit Abromeit added the bug label Nov 8, 2023
khalwat added a commit that referenced this issue Nov 8, 2023
khalwat added a commit that referenced this issue Nov 8, 2023
@khalwat
Copy link
Collaborator

khalwat commented Nov 8, 2023

Yeah the code was just wrong; since we render the attributes, its always going to be a string.

Addressed in: 50c4e71 & 7cd91f2

Craft CMS 3:

You can try it now by setting your semver in your composer.json to look like this:

    "nystudio107/craft-seomatic": "dev-develop as 3.4.64”,

Then do a composer clear-cache && composer update

…..

Craft CMS 4:

You can try it now by setting your semver in your composer.json to look like this:

    "nystudio107/craft-seomatic": "dev-develop-v4 as 4.0.34”,

Then do a composer clear-cache && composer update

@khalwat khalwat closed this as completed Nov 8, 2023
@Abromeit
Copy link
Author

Abromeit commented Nov 9, 2023

Wow, thanks for the quick response!

But now there is the problem that no canonical tag is displayed if max-image-preview:none is included in the selected values.

'max-image-preview:none': Craft.t("seomatic", "No image preview is to be shown."),

if (str_contains($content, 'noindex') || str_contains($content, 'none')) {

@khalwat khalwat reopened this Nov 9, 2023
khalwat added a commit that referenced this issue Nov 10, 2023
khalwat added a commit that referenced this issue Nov 10, 2023
@khalwat
Copy link
Collaborator

khalwat commented Nov 10, 2023

Not doing well on this one, am I? :)

Addressed in: aaf8ea4 & 8ba39a7

Craft CMS 3:

You can try it now by setting your semver in your composer.json to look like this:

    "nystudio107/craft-seomatic": "dev-develop as 3.4.64”,

Then do a composer clear-cache && composer update

…..

Craft CMS 4:

You can try it now by setting your semver in your composer.json to look like this:

    "nystudio107/craft-seomatic": "dev-develop-v4 as 4.0.34”,

Then do a composer clear-cache && composer update

@khalwat khalwat closed this as completed Nov 10, 2023
@khalwat
Copy link
Collaborator

khalwat commented Nov 15, 2023

This working for you @Abromeit ?

@Abromeit
Copy link
Author

@khalwat tested successfully – all fine now. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants