Sort out and fix language fallback inconsistencies

I feel like putting this into the content moderation issue component (in the issue metadata), but I can see how it's difficult to categorize.

@sdewitt Thank you for an exemplary issue summary.

I'm not sure I'll have the time this needs over the next couple of days. It'd be awesome to see a test replicating this bug. I also wonder if this is possible to reproduce via the API without Content Moderation? Is it another one of these issues that content moderation just exposes and doesn't cause?

If I understand correctly this is causing unpublished content to be viewed by users who don't have permission to see it. Therefore should we bump this to critical?

I don't think this is critical on the basis that what is being wrongly shown on what should be translated paths is already published in the source language.

This looks like it would happen without content moderation too - can you try with a clean install of the standard profile?

There's no information disclosure here, just content shown for language prefixes you're not expecting it to be shown for, I think it might possibly be by design even to show English as a fallback in the absence of a translation.

I recently also noticed this in a slightly different context. And yes, it actually doesn't matter if you use content_moderation or even forward revisions at all.

Interestingly, it doesn't happen when you *reference* an entity and display it through that. The reason is that we use two different contexts for getTranslationFromContext(), \Drupal\Core\ParamConverter\EntityConverter::convert() uses 'entity_upcast' while \Drupal\Core\Entity\EntityViewBuilder::viewMultiple() uses the default entity_view context.

This is relevant because the access logic for translations happens in content_translation_language_fallback_candidates_entity_view_alter(), as you can guess from that name, it only applies to the entity_view context.

So the fix should be as simple as removing the entity_view context or applying the content_translation logic to both (or all?) context strings.. I honestly don't quite understand why we need that context feature, maybe @plach can help with that.

I also mentioned this to him a while ago but forgot to create an issue.

I investigated this issue and I believe @Berdir and @sdewitt are describing two different, albeit related, scenarios.

First of all I would like to confirm that current core behavior is by design: we do want to have a valid route for a node (or any other entity) regardless of the current (content) language. This could be seen as a form of language fallback, if you will, but I believe that the main goal was to keep the navigation structure intact for highly "symmetric" multilingual sites, regardless of language. The SEO concerns @sdewitt brought up should be mitigated by the fact that we have a canonical URL link in the page metadata pointing to the original language URL, when no translation is available for the current language.

That said, I can see why the current behavior can be deemed problematic and/or inconsistent:

1. When we have a default revision having only a published English translation, both /node/1 and /it/node/1 return a 200.
2. When we have a default revision having a published English translation and an unpublished Italian translation, /node/1 returns a 200 and /it/node/1 returns a 403.

If I understand correctly, what @Berdir is suggesting would make case 2 behave as case 1: CT's language fallback would kick in and the English translation would be displayed also on /it/node/1.

OTOH what @sdewitt suggests is to ensure that no "fallback logic" is applied also for case 1, which would mean that in case 1 /node/1 returns a 200 and /it/node/1 returns a 404 (403 would not be appropriate, since there is no Italian translation in that case).

Given that we need to retain the current core behavior, because there are most certainly existing sites relying on it, I think the only solution to address this issue is to add an option to the Content Translation module allowing to pick one of these three language fallback modes.

While implementing @Berdir's suggestion is indeed trivial, I think supporting @sdewitt's use case is a bit trickier, since the language fallback API does not support returning no candidates. One possible workaround could be to throw ResourceNotFoundException from content_translation_language_fallback_candidates_entity_upcast_alter(), but this may have unwanted side-effects when checking route access while generating links. A POC patch would be welcome to check whether anything breaks.

Sorry, yes, I confused those things, my scenario only happens when you have unpublished translations in the default revision in which case they are then not filtered out on /node/ID and you then get an access denied but inside an entity reference, it falls back and shows a matching translation.

I think it is conceptually related though because I think it should behave consistently between having no translation and an unpublished one and between node/ID and an entity reference (worth pointing out that for a list of entities in a view, the behavior is already configurable).

I created an issue for the case described in #7.

I spoke with Gabor about this and he reminded that at the time we introduced this functionality we were planning to let contrib take care of all the possible fallback alternative behaviors. Of course we are both fine with performing any core change needed to enable this to happen in contrib.

As of proposed solution #1:
Add a flag to the getTranslationFromContext() method that when set to TRUE (defaults to FALSE to maintain BC), returns NULL if no translation is found for the current language context.

I came across this when implementing #1960684: Allow filtering by "Language (with fallback)" in search_api. From contrib i can not use getTranslationFromContext() to distiguish between "current translation is a fallback $langcode" and "$langcode does not have a fallback" (nor do i have another API). But for correct search API indexing, we need this.

Patch flying in that implements this. Not sure how to document the new paramter though, and what to do with the interface.

(Meta: Is this a hijack? Feel free to split this into its own issue.)

@plach in #13

> I spoke with Gabor about this and he reminded that at the time we introduced this functionality we were planning to let contrib take care of all the possible fallback alternative behaviors. Of course we are both fine with performing any core change needed to enable this to happen in contrib.

Matching this, i could not find any EntityRepository::loadEntityByConfigTarget unit tests for that fallback behavior. (but quite some webtests that may cover this)

This is a complex issue. Contemplating a bit on this.

As of @berdir #11, i think the original sin is content_translation_language_fallback_candidates_entity_view_alter.
It removes fallbacks for entities that the current user does not have access to. But should access be mixed into fallbacks? I strongly feel no. It will create all kinds of inconsistencies.
And beyond all: Thinking of above case where we want to index entities. We surely do not want the index to depend on the indexing user. neither do we want to embody user1 for the indexing operation. Yes we can add an "index" operation (and curse that noone knows it). But the problem does not arise if in the first place we separate fallbacks from access.

(#18...) Let's see what breaks on removal of content_translation_language_fallback_candidates_entity_view_alter()

EDIT: Also see @berdir's comments on that function in #2972308-22: Allow users to translate content they can edit

Interesting. We should investigate if the above is result of lack of tests for this code path or if this code path is indeed unnecessary as access is checked elsewhere too (which my gut feeling is).

As of patch #16:

> Add a flag to the getTranslationFromContext() method that when set to TRUE (defaults to FALSE to maintain BC), returns NULL if no translation is found for the current language context.

As we have no base class i can not see how we can do this in a BC way with an extra parameter (am i missing something?).

But OTOH we have a $context array where we can overload it there. Patch flying in that does this and removes said hook implementation.

Carried together arguments in the IS. See there why i deem this a bug (but do not want to bikeshed about that one).

Also i'd say this is broader than content_translation.module

THANK YOU ALL!!!

I have been searching for a fix to access_unpublished working with latest revision drafts for content that has a published revision. Without this core patch only the default language works correctly in this scenario (hashkey access using access_unpublished)

however with this core patch, it resolves my issue.

Prior to finding this I was trying to run backtraces but those seem to crater my server that only has 16 gigs of ram, I think I need about 64 gigs of ram just to be able to do a proper backtrace.

Honestly I spent all weekend working on this, and 2 nights this week, I was debugging like crazy trying to figure out what the heck was going on, didn't even really know what to look for, then I just started trying core patches that had the word 'moderation' and 'translation' in it.

This is the right fix!

Bumping up the priority on this, this fix is going to be used on a Drupal site for some very important and internationally famous persons!

Million thank yous! I tested this fix to work on Drupal 9.0.x dev. I'm going to be using this on a production system using either 8.7.10 or 8.8.0.

to explain at a very high level how this resolves issues, during my testing without this core patch using the access_unpublished module with content moderation and pending draft revisions, the route and route_match in the latest revision check class of access_unpublished loads the entity based on route and route_match but the default language is loaded instead of the correct language. I'm not sure why but this access check processes the same entity several times in this scenario and which probably explains why a get translation won't work even if hard code loading the expected revision. there's several entities involved not just node entities that get processed, so this is why this core patch is needed in this case.

There's likely more fixing that this does internally, with this patch the access checks refer to the correct language revision and works as expected however without this patch no contrib code change would do. It's too deep into the internals of core.

I highly recommend this core patch. We just need tests for it.

Still needs tests.
This patch is critical to many of my clients, for now it is in all of our builds

Here is a re-roll for 9.1.x.

This might be related #2964383: Access denied on content translation and unpublished content with non-english fallback language so I tried Axels patch from #21 but it didn't change the situation. Anyway both issues seem to be closely related?

@TODO:
review whether or not we still need the patch for Drupal 9.1.10 or the latest D9.2.x branch or 9.3.x

#2972308: Allow users to translate content they can edit

@TODO:
review whether or not we still need the patch for Drupal 9.1.10 or the latest D9.2.x branch or 9.3.x

#2972308: Allow users to translate content they can edit

It surprises me a lot this issue hasn't had that much followers?
I'm afraid we are experiencing this in all of our sites, but just never realised. We just noticed it when doing some deeper testing than usual and at first we were thinking it was a problem with the content_access module that we are using. Instead it seems it's a wider thing, so we'll test the patch from #21 on 8.9.17.

If i'm reading the tests correctly, something should be added to web/core/tests/Drupal/KernelTests/Core/Entity/EntityTranslationTest.php in order to test this properly? I'll try doing my best there.

I hope i managed to write a test for this? Here's my effort. So now i'll try patching :)

web/core/tests/Drupal/FunctionalTests/Entity/EntityTranslationPublishTest.php

The test fails for me ATM... i suspect because of a bug somewhere?

Sorry the test i added was definitely wrong. This is a better version i hope :)

... And finally a complete, real patch with this tests.

Tried to fix custom failures. Please review.

I think #9 is a perfect summary of what is happening.

Given that we need to retain the current core behavior, because there are most certainly existing sites relying on it, I think the only solution to address this issue is to add an option to the Content Translation module allowing to pick one of these three language fallback modes.

It would be great to have an option to select what kind of fallback behaviour is appropriate for a specific site. Site editors probably need different rules though. If a user has access to an unpublished/draft translations, we can probably not simply ignore any fallbacks.

I've figured out a problem with language fallback logic, which it always returns the default language, even if fallback to this language is disabled and this language is absent in the fallback candidates array.

Here is my issue about this: #3308838: EntityRepository::getTranslationFromContext() function forcibly adds default entity language to fallback candidates

The proper solution for that problem is just return NULL from EntityRepository::getTranslationFromContext, but this breaks a lot of other Drupal Core functions (and most likely Contrib too) that doesn't expect the NULL response from this function.

Rerolled patch 9.5x

Still needing and using this patch in a few hundred Drupal 8 and 9.x sites, three years into this now.

NOTE:
We observed similar issue when writing test for decoupled menu. @bbrala took a deep dive into what is happening and came up with a summary here https://www.drupal.org/project/drupal/issues/3227824#comment-14757962

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

try this, changes to the automated tests as per the bot suggestions.

code sniffer.

Added #3067999: ProcessedText filter doesn't pass in correct language when using language fallback

+1 for somehow making this configurable.

Currently we have a site where I would like to see @sdewitt's suggestion as explained by @plach in #9. I.e: No translation -> 404, unpublished translation -> 403

But I can definitely see websites where you want a fallback at all cost and then it would make more sense to have a fallback in both the above cases. The downside to a toggle is that if you flip it on a live website with existing data, all of a sudden your access logic gets turned upside down.

Query access seems to be fine, as missing or unpublished translations don't show up anyway (fulfulling scenario 1's desires) and the fallback always shows up for both scenarios.

We ran into another demand for disabling language fallbacks, adding that here to emphasize the need to be able to do this. Currently, entity repository adds the default language as a fallback and there is no way to stop it from doing so as it runs after the fallback hooks.

We are currently relying on the buggy behavior explained in #2978048-33: Unpublished translations should fallback to the original language and #2978048-35: Unpublished translations should fallback to the original language as at least it is some way to make Drupal not use fallbacks. All we have to do then is make sure that whenever a new content entity is published, we create unpublished translations for it.

It would be far superior for Drupal to be consistent across the board and to allow developers to choose whether or not they even want fallbacks to happen. It's perfectly valid to say: "Deny access to translated versions of a node until it's translated and ready". Especially in legal contexts where you are not allowed by law to show certain content outside or inside of certain countries.

Our current work-around doesn't work perfectly, because ER field, the entity_upcast operation and the default entity_view operation all behave differently.

Patch on #58 works like a charm on Drupal 10.3.5. In my case the problem was paragraph related: we have a paragraph published in Catalan and unpublished in English . However, before applying patch, it was still showing publicly in English.

Thanks @rdocina, only one fail now
https://git.drupalcode.org/issue/drupal-2951294/-/jobs/4065933

With latest merge from 11.x, all pipeline tests are all green.

MR doesn't have all the changes from patch #58. Now the hook content_translation_language_fallback_candidates_entity_view_alter is in the class, so MR actually does not remove it, but patch - does.

True, now the hook is in src/Hook/ContentTranslationHooks.php?ref_type=tags#L382-415 and the 11.x MR still has it, while the 10.x patch has the hook removed.

But all tests are green 🤔

Are the tests wrong? Or could the hook still stay?