Translated Nodes do not switch languages properly when set as the home page.

jenlampton commented 3 years ago

Description of the bug

Translated Nodes do not switch languages properly when they are set as the front page.

If I create an English about page, then translate into Spanish, I can click the "English" link to see the English version, or the "Spanish" link to see the Spanish version -- no problem.

However, if I then navigate to admin/config/system/site-information and set my Default home page to be about that breaks the language switching.

Now when I go to the about page (used for home) and click the "Spanish" link to see the Spanish version of the page, the page remains in English. The only way I can view the Spanish version of the page is to navigate directly to the Spanish node.

Steps To Reproduce

Enable locale and content translation modules
add at least 1 other language
Translate the about page
Place the Locale language switcher block.
Navigate to admin/config/system/site-information and set the Default home page to be about
Visit the about/home page.
Use the link in the language switcher block to view the other version of the home page.

Actual behavior

Front page remains in primary language, regardless of which language was selected.

Expected behavior

Front page should show in selected language.

jenlampton commented 3 years ago

Well, I'm not sure what fixed it but my site seems to be switching languages properly now. (I know edited the both translations of the content and both menu entries. Not sure what else...)

klonos commented 3 years ago

Related: I'm not sure what has changed recently, but I've been often testing PRs with RTL languages by doing the following:

enable Language module
add Arabic, or Hebrew, or any other RTL language
set the RTL language as the default -> site (front end and admin UI) instantly changes to RTL so that I can test 👍

This doesn't seem to work as before anymore ("before" being 2-3 bug releases back maybe?), in that I now have to specifically add the language code in the URL to have things switch to RTL.

klonos commented 3 years ago

...scratch all the above; I just tested again and it seems that we've fixed that in the latest 1.x. Everything works as expected again 👍

jenlampton commented 3 years ago

I wonder if something recently caused an extra cache-clear to be necessary, when it wasn't before? (Maybe entity cache? No, that wouldn't make sense, these are different entities...)

jenlampton commented 3 years ago

Well, everything stopped working as expected again on my site. Re-opening with new STR (in OP). It appears as though is is also a known drupal issue: https://www.drupal.org/project/drupal/issues/339958

indigoxela commented 3 years ago

Well, everything stopped working as expected again on my site

@jenlampton A quick question: I suppose you do have the Redirect module turned on? Did you change the title (and/or alias) of the node between the time it seemed to work and the time it stopped working?

Anyway, I can confirm that the language switcher does not switch the node translation if the node is set to be the front page. For other nodes and their translations everything works fine.

jenlampton commented 3 years ago

I'm not sure where I need to fix this problem in core, but I've added a work-around to my site as follows:

/**
 * Implements hook_url_inbound_alter().
 */
function HOOK_url_inbound_alter(&$path, $original_path, $path_language) {
  if (backdrop_is_front_page()) {
    global $language;
    if ($language->langcode != $path_language) {
      if (arg(0) == 'node' && is_numeric(arg(1))) {
        $node = node_load(arg(1));
        $translations = translation_node_get_translations($node->tnid);
        if (!empty($translations[$language->langcode])) {
          $path = 'node/' . $translations[$language->langcode]->nid;
        }
      }
    }
  }
}

indigoxela commented 3 years ago

I'm not sure where I need to fix this problem in core

I'm also clueless. :shrug:

Maybe something in translation_language_switch_links_alter fails?

Or the config item "site_frontpage" overrides it again at a later point?

jenlampton commented 3 years ago

Maybe something in translation_language_switch_links_alter fails?

I suspect it is actually something in how it determines what to load for the home page. When my site is on "/es" it always loads the English version of the home page instead of the Spanish, because that is what's set in system.core.site_frontpage.

What it needs to do is retrieve site_frontpage, then check the language, then check if there is a translation of the site_frontpage in the matching language, and if so, load that instead.

I spent some time poking around in path.inc and system.module but was unable to find where the frontpage magic happens... My next step is to search for drupal issues relating to site_frontpage and review the patches. Then I'll see if there's a hook nearby that lets me change that behavior, and then maybe add that into content translation module?

indigoxela commented 3 years ago

I suspect it is actually something in how it determines what to load for the home page

Seems plausible. Then it's rather a bug in the translation module, which fails to load the proper nid for the current tnid?

This is also broken in Drupal 7, BTW. Fancy that I never realized that.

indigoxela commented 3 years ago

Hey, @jenlampton, you know what... your "workaround" implemented as translation_url_inbound_alter() (use the alter hook in the translation module) fixes the problem. :wink:

Mind to create a PR from it? Or are there any concerns?

jenlampton commented 3 years ago

I've created a PR. My only concern was if adding a node_load() here would degrade performance, but I think entitycache should prevent that from doing any additional db queries.

docwilmot commented 3 years ago

Posting mostly to sort out in my head: the process goes roughly bootstrap loads the saved front page from config to $_GET['q'], then when menu_execute_active_handler() is called, that checks menu_get_item() which loads the $path as whatever is saved to $_GET['q']. Then Layout takes over (because the active handler in Backdrop is layout_route_handler()).

See:

function backdrop_path_initialize() {
  // Ensure $_GET['q'] is set before calling backdrop_get_normal_path(), to
  // support path caching with hook_url_inbound_alter().
  if (empty($_GET['q'])) {
    $_GET['q'] = config_get('system.core', 'site_frontpage');
  }
  $_GET['q'] = backdrop_get_normal_path($_GET['q']);
}

Somehow I dont see where the translation system gets involved here at all.

So hook_url_inbound_alter() is actually called in backdrop_get_normal_path(); so maybe we can add a call to menu_get_object() there instead of the node load, and trigger the translation action right into that function rather than an alter()?

indigoxela commented 3 years ago

This is how I tested:

Enable language, translation, locale modules
Add a language
Set page multilingual support to enabled, with translation
Add a page and translate
Set that node (one of them) as the start page (default home page)
Add the language switcher to default layout
Go to start page and switch language

Works like a charm.

@docwilmot did you miss that there's already a PR?

indigoxela commented 3 years ago

@jenlampton mind to add "Fixes" to your PR comment? It's much easier to find if the link, if it appears here in the sticky header. :wink:

docwilmot commented 3 years ago

@docwilmot did you miss that there's already a PR?

No, the line about "So hook_url_inbound_alter() is actually called in backdrop_get_normal_path(); so maybe ..." is with reference to that PR. We usually try to directly fix the origin of the process, rather than calling alters. Ideally altering should be for contrib, or if other modules are accessing a core API, I think.

indigoxela commented 3 years ago

We usually try to directly fix the origin of the process, rather than calling alters

IMO it has to be fixed in the translation module, translations are only available, when it's enabled. I don't think that menu_get_object() is the right place - why should it deal with tnid?

docwilmot commented 3 years ago

I was considering avoiding the node load.

indigoxela commented 3 years ago

I was considering avoiding the node load.

I see. But I don't think it does any harm.

docwilmot commented 3 years ago

Youre likely right; and in either case, you could call menu_get_object() in that PR instead of the node load as well.

docwilmot commented 3 years ago

Just also curious, does translation work fine if the front page is set to a layout path or any other non-node path?

jenlampton commented 3 years ago

mind to add "Fixes" to your PR comment?

Whoops, added :) https://github.com/backdrop/backdrop-issues/issues/4785 Sorry!

Ideally altering should be for contrib, or if other modules are accessing a core API, I think.

This seemed like a bit of a gray area to me too... This "fix" is really only necessary if you have content translation enabled, so it seemed like maybe the alter in the content translation module is appropriate?

I agree it would be cleaner if whatever builds any path could look for translations of the item, compare to requested language, and load the alternative instead.

I don't think that menu_get_object() is the right place - why should it deal with tnid?

The strange thing is it works on paths that are not the front page, but fails on the one that is. That makes me think that the code we need is already in here somewhere -- it is just not being called. Something is wrong with the front page handling and I'd much rather find and fix that than add a work-around for content translation.

Just also curious, does translation work fine if the front page is set to a layout path or any other non-node path?

Layouts are not "translatable" so having a front page as a layout doesn't test for the problem (the same path is loaded for a layout regardless of language). I haven't tried it with a taxonomy term, but I would expect the same behavior as we see for nodes, since the system blindly loads whatever path was provided. I'll test that...

jenlampton commented 3 years ago

Hm, I don't see a translate tab on my term... Suggestions on how to test this?

indigoxela commented 3 years ago

Maybe off topic, but there's a contrib module that uses the same hook, but goes beyond what the PR does.

I created it after some feedback in the i18n issue queue - formerly i18n_variables did the job, but can't cover that anymore.

I still belief that hook_url_inbound_alter() in the translation module is the right approach to get the language switching for the front page, if it's a node.

herbdool commented 2 years ago

I think this is the right approach as well. Code looks good.

herbdool commented 2 years ago

Sorry, I added a comment in the PR for a small fix. Plus I'm thinking this might need a test to be thorough.

backdrop / backdrop-issues

Translated Nodes do not switch languages properly when set as the home page. #4785