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

BUGFIX: Language fallback handling for canonical and alternate language links #154

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Sebobo
Copy link
Member

@Sebobo Sebobo commented Jan 25, 2021

The canonical link will now link to the original node if a dimension fallback is active.

The list of alternate language links will not contain nodes anymore if they don't exist in the target language.

Also the lang attribute of the html tag is now correct when a fallback is shown.

All three issues can be tested by browsing to /uk in the Neos.Demo.
lang should be en_US, canonical should link to it to and no alternate language link for uk should exist.

Without this change untranslated pages
have canonicals to themselves instead
of their fallback node if set.
@Sebobo Sebobo requested a review from bwaidelich January 25, 2021 16:32
@bwaidelich bwaidelich removed their request for review January 25, 2021 17:17
@regniets
Copy link

Thanks for fixing this issue!

This is nevertheless a breaking change which could impact many websites to the negative.
You mostly have reasonable fallbacks like en-us > en which do make sense and should have hreflang and canonical to themselves.
But there are cases (like on neos.io) that fall back from de > en. Now your hreflang is faulty because you show english content flagged german.
I would not recommend merging this fix as it is. We could

  1. Provide only a gist to show how to handle it (and/or write documentation for this special case)
  2. Provide a feature flag in order to activate it in case you need.

What we are trying to handle might actually be quite an edge case...
What do you think?

Copy link

@regniets regniets left a comment

Choose a reason for hiding this comment

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

Code is fine, but see my comment on use case.

@Sebobo
Copy link
Member Author

Sebobo commented Jan 26, 2021

I disagree that it's an edge case. Actually most projects I'm involved with which use multiple languages use fallbacks to completely different languages. Most of the time everything falls back to English and the customers want to have a full page tree in all languages as they don't know when they are able to localise everything.

I also don't really see the negative SEO effect. Why should the UK version when it's not localised have its own canonical and language link?

If this is really required to be defined on a per preset basis we might need additional options in the presets or in the SEO settings.

@regniets
Copy link

Got it - and good to know there are many websites that will benefit (so will neos.io)!

But we are introducing something that will have an effect on many websites that are using this package
(I would not want this to happen unnoticedly).

Flagging derivates is a main use case of hreflang. When i have a website for Austria (.at) that shares most of the content with the german version (.de) but has different products, i would want the austrian user to find austrian results (de-at).
If i let most of the pages point to their de version, austrian people would end up most likely on my .de website.

@Sebobo
Copy link
Member Author

Sebobo commented Jan 26, 2021

So you mean regarding the canonical and alternate language links the new behaviour should be behind a feature flag, which we might inverse with a major release?

@@ -22,6 +22,7 @@ prototype(Neos.Seo:AlternateLanguageLinks) < prototype(Neos.Fusion:Component) {
@if.isDefaultLocale={props.defaultLocale == item.dimensions[props.dimension][0]}/>
<Neos.Seo:AlternateLanguageLink node={item.node}
@if.isNotAbsent={item.state != 'absent'}
@if.existsInTargetDimensions={item.node.dimensions == item.node.context.targetDimensionValues}
Copy link

@regniets regniets Jan 26, 2021

Choose a reason for hiding this comment

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

Checked the hreflang specs. We need to remove hreflang in the "unlogic fallback" case as nothing points to this version.
Actually "@if.hasNoForeignCanonical" needs to be adapted as it does have a foreign canonical
https://developers.google.com/search/docs/advanced/crawling/localized-versions?hl=en

Copy link
Member Author

Choose a reason for hiding this comment

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

Don't fully understand what needs to be done from your description.

Copy link

@regniets regniets Jan 26, 2021

Choose a reason for hiding this comment

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

actually something like
@if.hasNoForeignCanonical = ${String.isBlank(q(this.node).property('canonicalLink'))}
@if.nodeIsTranslated = ${this.node.dimensions == this.node.context.targetDimensionValues}
... line 5

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah ok, so an additional check for the canonical

Copy link

@mbiberger mbiberger Oct 27, 2022

Choose a reason for hiding this comment

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

With the query @if.existsInTargetDimensions={item.node.dimensions == item.node.context.targetDimensionValues} no hreflang is output when I only adjust content on a page that has a language fallback.
We have now added the following queries in Neos.Seo:AlternateLanguageLink:
@if.hasMetaRobotsNoIndexNotSet = ${q(this.node).property('metaRobotsNoindex') || q(this.node).is('[instanceof Neos.Seo:NoindexMixin]') ? false : true} @if.hasNoForeignCanonical = ${String.isBlank(q(this.node).property('canonicalLink'))}

thanks to @regniets 😉

@bwaidelich
Copy link
Member

bwaidelich commented Jan 26, 2021

I'm not into SEO that much (I just know that duplicate content is bad) so I can't really vote on this change. But from my own experience I can tell that breaking changes in this package can be very harmful because they can be quite subtle. When I updated a large project recently it introduced a different set of metatags leading to a downgrade on Google.
So in general I think, a feature flag might be a good idea.
And for the long run it might be a solution to have some more CLI tools to setup/verify SEO settings interactively

@regniets
Copy link

@Sebobo - i think a feature flag might be the best solution.
@bwaidelich - i just happen to have some knowledge in hreflang, but from my experience a lot of people don't - so i consider it dangerous as well. I like the cli tool idea; just the featureset of neos/seo is pretty advanced already, maybe there won't be so many options to be set.

@kdambekalns kdambekalns self-requested a review March 16, 2021 09:14
@Sebobo
Copy link
Member Author

Sebobo commented Sep 1, 2021

A customer of mine required the same fix today for a project with many languages and 2 dimensions.
I applied it there including @regniets adjustments and will finish up this PR when they are happy with it.

Copy link
Member

@kdambekalns kdambekalns left a comment

Choose a reason for hiding this comment

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

[…] and will finish up this PR when they are happy with it.

Any news?

@regniets
Copy link

indeed, got a customer project as well now that could need the fix :)

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

Successfully merging this pull request may close these issues.

6 participants