Incorrect site content in multi-language setup after a plugin uses site()->content()

[hdodov]

Kirby Team summary

Status quo

In multi-lang setups, the ModelWithContent::content() method can be called with a specific language code or null for "the content of the current language". The return value for null is cached in memory. So if the method is called before the current language has been set by the router with AppTranslations::setCurrentLanguage(), the default language is used instead. Even worse, the content of the default language is cached in memory and used for template rendering etc.

Target goal

It should not be possible to access the content with $languageCode = null when the current language is not yet known.

Possible solutions

1. Reorganize loading order so that the current language code is known earlier As suggested here: https://github.com/getkirby/kirby/issues/2629#issuecomment-636979041 (needs to be checked if viable without major restructuring)
2. If not possible: Throw an exception If ModelWithContent::content() is called with $languageCode = null in a multi-lang setup, it should check if the current language has already been set in App (we might need a new flag for this). If no language is set, throw a LogicException that access to the content is not yet possible.

---

Describe the bug If a plugin calls the site() function immediately when loaded, the default language content is used and cached, and then the template does not use the translations.

To reproduce

1. Set up a multi-language site
2. Add a plugin index.php with site()->content() inside
3. In a template, echo site()->title()
4. Navigate to the translated page
5. You'll see the non-translated title

Kirby Version
4.3.6

Additional context I'm pretty sure that this happens because Kirby loads plugins before figuring out the language. When the plugin calls site()->content() at that phase, it's likely the first call. This means that Kirby will load the content for the current language (which is not the correct one) and cache the resulting Content object for performance reasons. Later, in the template, that same Content object is returned and it holds the invalid values.

This happened to me when using Sitemapper by Cre8iv Click. This is the line that causes the issue.

---

I know this is technically an issue with the plugin, but it's also an issue with Kirby. It took me a while to debug that, and the plugin author likely doesn't even know about it because that's a pretty tricky and obscure error.

Kirby should either determine the language before loading the plugins, or avoid caching the Content object until plugins have loaded and Kirby has initialized in general.

Edit: This issue also affects the panel. When you click to view the content of a translation, you just see the default content.

[texnixe]

Yes, we have already come across this multiple times on the forum and there is already an issue in the plugin: https://gitlab.com/kirbyzone/sitemapper/-/issues/4. But might make sense to solve this Kirby-wise.

[hdodov]

@texnixe yeah. It might be the expected behavior from Kirby's point of view, but leads to unexpected behavior from the user's point of view that goes unnoticed. At the very least, Kirby should throw an error to let you know that something is wrong.

If site()->content() is called before Kirby is fully initialized, it leads Kirby to cache the incorrect site content. Yes, the caller is obviously the one causing the issue, but Kirby shouldn't be letting it happen in the first place.

[lukasbestle]

These loading order issues are always super super hard. It would be so lovely if there was a way to load everything literally simultaneously, but of course that's impossible. So because there is a fixed loading order, stuff depends on another and changing the order becomes impossible as well. There is just some stuff plugins can't have access to while the system is still loading.

In this particular case, the plugin should initialize the option default value with null and fill in the default value when getting the option later with kirby()->option('cre8ivclick.sitemapper.title', site()->title()->html() . ' Sitemap').

But as you both say, the question should be if we can at least prevent the consequences further down the chain directly in the core. These are our (non-)options:

• Initialize the languages earlier
  I have made some tests and it looks like the issue here is not that the languages are not loaded yet, as the issue still occurs if the site content is accessed from the system.loadPlugins:after hook (which is triggered after the languages have been loaded). Instead, it looks like it's about the router not having initialized the current language yet. This can't happen any sooner unfortunately.
• Re-initialize Kirby objects after loading the plugins
  Not really viable IMO, as it will cause a performance penalty and also the question is if a half-accurate Kirby object is of any use to the plugin anyway. So it doesn't really make sense that plugins access data that's not fully initialized yet. Which in turn means that it doesn't make sense that Kirby should reset objects that shouldn't have been accessed in the first place.
• Block access to Kirby objects before they are ready
  So to be honest I think that the best way would be to block access completely and to throw an Exception. This will make the issue apparent quickly already while developing the plugin. The issue is just that we would need to manually review each Kirby object and each initialization method and add the Exceptions to them based on the appropriate conditions that make sense for that particular method.

[hdodov]

@lukasbestle doesn't it make sense for the router to have two types of initializations? As far as my debugging went, the issue comes from the fact that the content() method is called with $languageCode = null and the translation is not loaded. The request URL is constant for the current request and therefore the router doesn't have to wait for other stuff to initialize in order to determine if the visited page is a translation or not. Maybe the following is possible:

1. The router determines if the visited page is a translation and initializes as much stuff as it can that revolves around similar constant values such as the URL
2. Plugins are initialized and can call site()->content() and the correct data will be returned
3. Other stuff is initialized, custom routes are registered, etc.
4. The router is fully initialized

---

If there's no easy workaround, it makes the most sense to me to throw an error. Maybe the App class could have some sort of flag that indicates whether Kirby is initialized and the Site object could throw an error if said flag is false and someone accesses something that is not ready until everything is fully initialized, such as the content.

I would very much prefer to have a fatal error and know what's wrong than to spend several hours debugging. It would also lead to more stable plugins.

[lukasbestle]

Regarding the translation loading: @bastianallgeier can probably tell you more as he has written the translation loading code.

Maybe the App class could have some sort of flag that indicates whether Kirby is initialized and the Site object could throw an error if said flag is false and someone accesses something that is not ready until everything is fully initialized, such as the content.

There is no real "Kirby is fully loaded" state. The objects depend on another, so a full block would also prevent Kirby from loading. So we would need to take a look at each initialization method on its own and verify that its specific prerequisites are there.

I would very much prefer to have a fatal error and know what's wrong than to spend several hours debugging. It would also lead to more stable plugins.

Exactly, that's what I think as well.

[fabianmichael]

@lukasbestle @bastianallgeier Hey there, I’ve just run into that issue with my meta plugin. I would really appreciate if you could handle this, as it is absolutely not obvious and has cost me quite some time to figure this out (I used site()->content() within a routes callback of the plugin).

[lukasbestle]

@fabianmichael Sorry that this has cost you debugging time.

In an ideal world we would review every part of Kirby for loading dependencies and add appropriate errors or warnings everywhere, but maybe we can already start with this specific part where we know it causes issues in the real world.

I've added an issue summary to the initial post of this issue.

[fabianmichael]

@lukasbestle I did not mean to blame you for anything. But since this is really not obvious when calling the function, it would be very helpful if Kirby would use situations like this to protect itself from screwing up its own state. If this needs more investigation (i.e. will result in a longer process), a hint in the docs would be a first step.

[lukasbestle]

I didn't understand it as blame at all. :)

A hint in the docs has the disadvantage that there's no good place to put it. It will be missed. As I wrote, I suggest to fix this particular case. We only need to resolve the open questions in the issue summary (first post).

[github-actions[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs.

[fabianmichael]

If this issue still exists, I think it should still be fixed. Maybe the method could just throw an exception when called to early?