search

getgrav / grav-plugin-sitemap

Grav Sitemap Plugin
https://getgrav.org
MIT License
42 stars 42 forks source link

Properly support translated entries in sitemap #83

Closed nbusseneau closed 3 years ago

nbusseneau commented 4 years ago

This fixes #59 by properly adding one entry for each translation of each page in sitemap, with all entries referencing all other translations of the same page.

As per https://support.google.com/webmasters/answer/189077:

Common Mistakes

Here are the most common mistakes with hreflang usage:

Missing return links: If page X links to page Y, page Y must link back to page X. If this is not the case for all pages that use hreflang annotations, those annotations may be ignored or not interpreted correctly.

diegomagikal commented 3 years ago

Great PR, I'm waiting for it..

rhukster commented 3 years ago

From my initial testing, this does not preserve the existing default behavior. For example with no multilanguage enabled, it doesn't show all the pages. I cannot consider this for merging unless default behavior is maintained as it will break anyone's site that is not multilanguage enabled.

nbusseneau commented 3 years ago

You are 100% right, my mistake for not testing behaviour with multilanguage disabled. I think I see where the issue is, I'll revisit this PR when I have some free time (probably next week).

nbusseneau commented 3 years ago

@rhukster Should be fixed now. Logic was flawed as I hadn't taken into account that translatedLanguages() would not work when multi-language was not enabled. The new logic first checks if multi-language is enabled so as to properly add either the entry as-is or all its available translations.

rhukster commented 3 years ago

Will take a look at this next week. Thanks!

nbusseneau commented 3 years ago

Bumping this -- don't hesitate to HMU if I can do anything.

01Kuzma commented 3 years ago

I've tested this commit on November 6 here

I've downloaded two files from PR #83 and replaced originals and the result is negative. Tested on Grav 1.7 RC17 Now a URL http://localhost/project/LANG_CODE/sitemap loads and shows some data (before it there was no any links), but rendered links are without the language code inside. They are pointing to the same default's language data.

nbusseneau commented 3 years ago

@01Kuzma Can you share a minimal website sample reproducing the issue? That way I'll have a better chance to successfully fix the code for more usages than just my own :)

01Kuzma commented 3 years ago

@Skymirrh, here is what I'm getting:

As for example, once I access the default webpage: website.com/sitemap

I've got links to all resources with all languages shown. Firstly comes links with a default language:

http://website.com/about

http://website.com/cookies

and then links with a second language, BUT all slugs are the same:

http://website.com/ru/about

http://website.com/ru/cookies

When I access URL website.com/ru/sitemap the slug's of a second language are changed to normal:

http://website.com/ru/o-nas

http://website.com/ru/politika

BUT the slugs of a default language are also taken from second language:

http://website.com/o-nas

http://website.com/politika

Currently I'm running Grav 1.7 RC20 and without commits plug-in is working correctly. Probably something was changed in the core...

And I observed a bug: website is running under SSL, but the links are displayed without https

What should I provide you? A skeleton of a website? I could remove unnecessary parts and zip it.

nbusseneau commented 3 years ago

Yes, an archive with a minimal skeleton reproducing the issue is perfect.

I have only developed and tested on 1.6, not 1.7, so that is also something I'll have to consider.

01Kuzma commented 3 years ago

@Skymirrh , this should work. I left only minimal required data.

Plug-in is not patched, so you could check before and after. As archive is too big, I've temporary placed it here: ` Zip password: `

nbusseneau commented 3 years ago

Thanks, I'll have a look at it when I have some time next week.

01Kuzma commented 3 years ago

OK, could I remove the archive from website?

nbusseneau commented 3 years ago

I downloaded it just now, you can remove it if you want :)

rhukster commented 3 years ago

I tried your PR branch, but it didn't produce the desired results I was looking for. Unfortunately, It didn't take into account page fallback nor did it produce all the hreflang crosslinks. I found it easier to just do things my own custom way: https://github.com/getgrav/grav-plugin-sitemap/commit/856aea099cf5b62ba566669c9a2bf9f66d6f35ea

Please test the 'develop' branch and if there's no major issues I'll release it as 3.0. Cheers!

nbusseneau commented 3 years ago

Nice! I'll try it out this evening and report back.

nbusseneau commented 3 years ago

Sorry for not testing last evening, had some other stuff pop up.

Tested just right now: it works fine on my end, I do get one entry for each translation of each page on my website, identically to my own implementation attempt 👍🏻