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

Pages: Collection for secondary language contains dummy items for untranslated default language #2985

Closed
pamtbaau opened this issue Aug 21, 2020 · 25 comments
Assignees

Comments

@pamtbaau
Copy link
Contributor

pamtbaau commented Aug 21, 2020

This is a follow up on an earlier somewhat related issue: #2517

The collection for a non-default language contains dummy substitutes of default language items when translation doesn't exist. The dummy item throws 404 when clicked.

Consider the following folder structure:

user/pages
├── 01.home
├── 02.typography
└── 03.blog
    ├── blog-item1
    │   ├── item.en.md
    │   └── item.fr.md
    ├── blog-item2
    │   └── item.en.md    <-- No French translation
    ├── blog.en.md
    └── blog.fr.md

And using the yet undocumented content_fallback option mentioned in above issue:

languages:
  supported:
    - en
    - fr
  include_default_lang: true
  pages_fallback_only: false
  translations: true
  translations_fallback: true
  session_store_active: false
  http_accept_language: false
  override_locale: false
  include_default_lang_file_extension: true
  content_fallback:
    en: en
    fr: fr

The French blog contains two items instead of one: The translated 'Blog item 1' + a dummy for 'Blog item 2'
image

Clicking the dummy version of 'Blog item 2' will throw 404.

@pamtbaau pamtbaau changed the title 1.7: Collection for secondary language contains dummy items of default language 1.7: Collection for secondary language contains dummy items for untranslated default language Aug 21, 2020
@rhukster
Copy link
Member

The multilang was completely rewritten in Grav 1.7. Matias did some extensive work on that, so I've added him to this ticket. He will know best what's going on.

@rhukster rhukster added the 1.7 label Aug 21, 2020
@ondrejvysek
Copy link

Is there any chance to have this backported into the Grav 1.6 as the behavior is exactly the same.

@mahagr
Copy link
Member

mahagr commented Aug 25, 2020

The short answer is NO. The logic changes a lot and is not fully backwards compatible (some silly sites which actually use this need a minor configuration update).

@pamtbaau
Copy link
Contributor Author

@mahagr , Anything to share yet on the actual issue?

@mahagr
Copy link
Member

mahagr commented Aug 26, 2020

Not really. There was a design flaw (or too simple implementation) in language fallbacks in older versions of Grav. Fallbacks in Grav 1.6 look through all the previous languages and uses what is found first. Or just fall back to default. This doesn't really work if you have multiple languages, especially if you want to have different content in them. In Grav 1.7 you set fallbacks per language, which helps to fix this issue.

Well, actually in this case 404 comes from a slightly different issue, as Grav 1.6 sees the page as existing one, but because of the translation, it fails to display it. Still, it really comes back to the previous explanation on how translations have been dealt with.

My recommendation for a multi-language site is Grav 1.7 because of the improvements that were made into it.

@pamtbaau
Copy link
Contributor Author

@mahagr , The original posted issue is using the latest Grav 1.7 ...

@mahagr
Copy link
Member

mahagr commented Aug 26, 2020

Wow, sorry, I totally missed that! I just assumed too much from the previous comment.

I've not seen this bug before. Which theme are you using as your base?

@pamtbaau
Copy link
Contributor Author

It's a default install using Quark

@mahagr
Copy link
Member

mahagr commented Aug 26, 2020

I'm able to reproduce the issue.

@mahagr mahagr added the bug label Aug 26, 2020
@mahagr
Copy link
Member

mahagr commented Aug 26, 2020

After some investigation, this works only with Flex Pages enabled from the System configuration. So it's a new feature in Flex Pages, which is unfortunately not fully implemented in the frontend yet (and thus turned off by default as being an experimental feature). :(

Right now it's also slower to use flex pages in the frontend in big sites because of lack of optimizations. As being database-like, flex needs to be used in a certain way to make the queries fast.

@pamtbaau
Copy link
Contributor Author

pamtbaau commented Aug 26, 2020

@mahagr, I'm a bit confused...

this works only with Flex Pages enabled

What is 'this' referring to? It it refers to the new option content_fallback, there is no mentioning of 'flex pages' in issue Disabling translation fallback does not work #2517 where it is suggested as a solution.

Also, In my default Grav 1.7 install, Flex pages are not enabled, yet content_fallback solves issue #2517. Shouldn't the option then ignored when not using Flex pages?

Right now it's also slower to use flex pages in the frontend in big sites

What are big sites? Shouldn't that be mentioned on the Grav 1.7 download? I have the impression that quite a few just download Grav 1.7 because it is believed to be "near" production. Also Andy's bog from Nov. 2019 speaks about the great performance gain using Flex pages.

Anyway... still in awe of Grav and its dev team :-)

@mahagr
Copy link
Member

mahagr commented Aug 31, 2020

I was referring to enabling Flex pages experimental feature from the system configuration. Looks like the feature isn't implemented in the old page's logic...

@mahagr
Copy link
Member

mahagr commented Aug 31, 2020

I found out what causes the issue. It looks like the old Pages logic sees the untranslated pages as folders.

@mahagr
Copy link
Member

mahagr commented Aug 31, 2020

@pamtbaau I think the best thing to do is to filter blog pages if they are routable or not. Filtering is something that really needs to be done with Flex, too, as you may want to have page pointing to the other translations if you try to access it in a language that doesn't exist (it doesn't mean that the page is not there -- it is just not published using your current language).

@pamtbaau
Copy link
Contributor Author

@mahagr,

I think the best thing to do is to filter blog pages if they are routable or not.

I presume you mean that Grav needs to filter blog pages?

@mahagr
Copy link
Member

mahagr commented Aug 31, 2020

That is a good question. I don't think that Grav page collection can do that decision as you may want to do something else. It really has to be done in either inside the blueprints or preferably inside the template file, IMHO. This also means that the current Flex Pages frontend implementation is wrong.

I am even thinking if we could have default setting inside the collection which removes the non-routable pages, but it would break so many sites that we just cannot do it.

@pamtbaau
Copy link
Contributor Author

pamtbaau commented Aug 31, 2020

languages:
  supported: [en, fr]
  content_fallback:
    en: en
    fr: fr

If the user sets above setting, isn't he telling Grav explicitly to eliminate all other languages from the collection? English collection should only contain English, French collection only contains French pages.

Doesn't it mean there is no breaking of sites if the (still undocumented) setting is not used.

@mahagr
Copy link
Member

mahagr commented Sep 1, 2020

Well, kind of. It is already doing that in a form that those pages which do not exist are not routable. So if you filter the collection by routable, you will only get the translated blog pages.

@pamtbaau
Copy link
Contributor Author

pamtbaau commented Sep 1, 2020

@mahagr , What I meant was that with the new setting, the user is explicitly asking to not include other languages. It would be counter intuitive if a collection is returned that still contains other languages which have to be filtered out in Twig... No one would like items in a collection that will throw a 404 when accessed.

This behaviour is prone to errors due to misconception.

Since the new setting is undocumented and hence not (widely) used, no sites will break if Grav would remove the non-routable page itself.

@mahagr mahagr changed the title 1.7: Collection for secondary language contains dummy items for untranslated default language Flex Pages: Collection for secondary language contains dummy items for untranslated default language Dec 11, 2020
@mahagr mahagr changed the title Flex Pages: Collection for secondary language contains dummy items for untranslated default language Pages: Collection for secondary language contains dummy items for untranslated default language Dec 11, 2020
@mahagr
Copy link
Member

mahagr commented Jan 15, 2021

I've been thinking about this and the only reason to include untranslated pages is to get other translations. So you may be right. I'm just worried about parent pages that have no translation blocking their children.

Maybe we just need to try what happens...?

@mahagr
Copy link
Member

mahagr commented Jan 15, 2021

$page->collection() will now contain only pages which exist for the current language.

@pamtbaau
Copy link
Contributor Author

I presume you mean when using config:

languages:
  supported: [en, fr]
  content_fallback:
    en: en
    fr: fr

If that is indeed the case, I can confirm that '/en/blog' only return English blog items and '/fr/blog' only returns French blog items.

@mahagr
Copy link
Member

mahagr commented Jan 18, 2021

Yes, if you define fallbacks, they are also included in the collection.

This raised another issue, though... In most themes, it looks like navigation (or menu) twig does not check if the items are routable or published.

@pamtbaau
Copy link
Contributor Author

If I understand correctly...

Before this fix, collection used for menu could contain 'dummy' pages for non-existing translations. If theme does not check for routable/published it would create a menu-item leading to a 404.

Using this fix, the collection used for the menu would not contain a 'dummy' page and hence no menu-item would be created which points to an non-existing page.

@mahagr
Copy link
Member

mahagr commented Jan 18, 2021

The issue is related, but not the same -- the issue exists also in Grav 1.6 and is not related to the languages.

@mahagr mahagr closed this as completed Jan 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants