Problem/Motivation

Content entities have a feature that allows to control which translation is used when trying to view or do something else with it.

That is an API: \Drupal\Core\Entity\EntityRepositoryInterface::getTranslationFromContext().

Internally, that calls a hook that content_translation implements. the tricky thing is that the hook has a dynamic context operation argument, and we have different contexts in core.

In EntityViewBuilder, we use the default, which is 'entity_view'. But in EntityConverter, which is used when loading the node from the route, we are using 'entity_upcast'.

The problem is that content_translation.module currently only implements the hook for the entity_view operation. What it does is interesting, because it implements a fallback, that goes like this:

You have a site with 3 languages, EN, DE and FR. If you have an node 1 that has 2 translations, EN is the default language and published, DE is not published. If you pass that node through that API and request the published EN translation, that is returned, done. If you ask for DE, then it removes that as an allowed language and it falls back to EN, showing that. If you request FR, which doesn't exist, then it also falls back to EN and shows that.

This is the behavior when viewing node 1 through a view or entity reference field on another node, because then it goes directly to EntityViewBuilder and picks the language there with operation entity_view and the hook runs.

But, if you access that node directly, then something interesting happens. We load the node from the route, and pass it to getTranslationFromContext() with operation entity_upcast. The hook doesn't run. That means if we ask for DE, we do get the DE translation, with status 0. After that, we check access, but because this translation is unpublished, we deny access.

So this happens (as anonymous user, without access to unpublished content):

/en/node/1: EN translation is shown
/fr/node/1: Fallback to EN because FR doesn't exist, EN is shown.
/de/node/1: 403 access denied.

It just doesn't make sense to me (@berdir) that a non-existing translation and an unpublished one behave differently. But I haven't been able to convince @plach that this is a bug that must be fixed, he argued that too few people have reported this and it could be seen as an unwanted change. But if you take the above example and then for example add an unpublished FR translation as well, /fr/node/1 goes from EN to access denied. I really don't see how that would be a desired behavior for anyone.

Another example is a list of content, that shows all your content as a teaser, if possible translated, e.g. on the frontpage. That means with the setup above, on /en, /de, /fr, you always see that node, in EN. But then you click on it on /de and you get an access denied. Again, just doesn't make sense :)

Proposed resolution

Implement the hook also for entity_upcast. In fact, I'm not sure why it shouldn't be done for *any* operation.

Remaining tasks

Decide if we should do this or not.

User interface changes

API changes

Data model changes

Release notes snippet

Problem/Motivation

From #2951294.9:

  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.

Proposed resolution

The logic from content_translation_language_fallback_candidates_entity_view_alter should apply to entity_upcast context as well.

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#38 2978048-38-d11.patch3.77 KBmurilohp
#6 unpublished-translation-fallback-2978048-5-interdiff.txt887 bytesmbovan
#6 unpublished-translation-fallback-2978048-5.patch2.52 KBmbovan
#6 unpublished-translation-fallback-2978048-5-TEST-ONLY.patch1.72 KBmbovan
#3 unpublished-translation-fallback-2978048-3-interdiff.txt1.48 KBmbovan
#3 unpublished-translation-fallback-2978048-3.patch2.28 KBmbovan
#3 unpublished-translation-fallback-2978048-3-TEST-ONLY.patch1.48 KBmbovan
#2 unpublished-translation-fallback-2978048-2.patch820 bytesmbovan

Issue fork drupal-2978048

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

mbovan created an issue. See original summary.

mbovan’s picture

mbovan commented
Status: Active » Needs review
StatusFileSize
new unpublished-translation-fallback-2978048-2.patch820 bytes

Let's see if there are tests that cover a use case with unpublished translations.

mbovan’s picture

mbovan commented
StatusFileSize
new unpublished-translation-fallback-2978048-3-TEST-ONLY.patch1.48 KB
new unpublished-translation-fallback-2978048-3.patch2.28 KB
new unpublished-translation-fallback-2978048-3-interdiff.txt1.48 KB

Test coverage.

The last submitted patch, 3: unpublished-translation-fallback-2978048-3-TEST-ONLY.patch, failed testing. View results

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Maybe we can save without a translation first and access that URL first to show that not having a translation and having an unpublished translation results in a different behavior?

mbovan’s picture

mbovan commented
StatusFileSize
new unpublished-translation-fallback-2978048-5-TEST-ONLY.patch1.72 KB
new unpublished-translation-fallback-2978048-5.patch2.52 KB
new unpublished-translation-fallback-2978048-5-interdiff.txt887 bytes

Addressed #5.

The last submitted patch, 6: unpublished-translation-fallback-2978048-5-TEST-ONLY.patch, failed testing. View results

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented

As I mentioned in the other issue, I don't think we can simply change core's behavior like the patch is doing. Even in a minor release, this would be an IMO unjustified BC break. I think we should make this behavior opt-in through a CT configuration. We can discuss making it the default behavior for new installations, but we should keep the old behavior in place for existing sites.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

I do think that the current behavior goes completely against the content translation status description (" and is also very inconsistent.

See my updated test coverage #2972308: Allow users to translate content they can edit which shows that accessing an entity through an entity reference field vs. its route has a completely different behavior, that again differs based on whether the entity has its own status that it checks too vs. just a translation status.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented

@Berdir:

I'm happy to leave the decision to a product manager, but this issues has 6 followers: it's not like we're being flooded by complaints about the current behavior being buggy. I recognize that it's far from ideal, that it was not thought through as it should in the design phase (if we even had one for it), but IMO we cannot simply assume that changing this behavior to something more consistent will satisfy every user out there.

That said, feel free to ask a third-party opinion: I wont't fight :)

mqanneh’s picture

mqanneh
he/him
Primary language English
Location Amman
commented

Patch in #6 worked for me
8.5.x

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Issue summary: View changes
bohemier’s picture

bohemier commented

What @berdir described seems to make sense to me... In a multilingual setup, if you unpublish a particular translation, I would think you'd get to the default language when accessing that translation, although I also understand that in some use cases, you might want the current access denied approach.

In my use case, I want to revert to the default language (en) if a translation (fr) is unpublished. So I applied the patch and it works well.

Then I started thinking, what if I unpublish the original language node (en) and publish its translation (fr)? I was then redirected to the translated node (fr) when accessing /en/node/1. Now in this case, I was expecting an access denied because to me it that redirection should only happen from translated language to default (or original) language and not the other way around.

So maybe this approach should check to redirect only to the original language?

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

moghingold’s picture

moghingold commented

I'm seeing a bug with #6

Drupal version: 8.6.15

Reproduction steps:

  1. Navigate to a published node (eg /node/1)
  2. Create a translation for a node, making sure to change the content so it is distinct from the version published in the site's default language (eg /fr/node/1)
  3. Unpublish the translation
  4. Run node_access_update()
  5. In a different browser session, visit the translated node as an anonymous user (eg /fr/node/1)

Expected results:
The user should see the contents of /node/1

Actual results:
The user sees /fr/node/1, even though they do not have permission to view unpublished content

I am still looking into the cause of this, but I thought that @berdir and anyone else using this patch might wish to be made aware of the regression as they update their sites.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

> Run node_access_update()

I assume that means you are using node grants? Your own or a contrib module?

Note that node grants must consider the language code and provide grants for all translations. Otherwise there's a strange fallback logic that applies those grants automatically to all translations, then the result is that all translations share the same access checks.

We did run into similar issues with the group module and improved that in #3030187: Node translations accessible for anonymous users . You can likely even reproduce such issues without this patch, in the reverse order, e.g. adding an unpublished translation might result in removing access for the published one as it was last saved. But that depends a bit on the specific implementation.

geek-merlin’s picture

geek-merlin
Primary language German
Location Freiburg, Germany
commented
Status: Needs review » Postponed

Can we fold this into #2552663: Consider deprecating language_fallback in favor of entity_language_fallback?

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Postponed » Needs review

I don't understand how a core issue should be postponed on/duplicated by a contrib module/issue?

geek-merlin’s picture

geek-merlin
Primary language German
Location Freiburg, Germany
commented

Ups, facepalm, i meant to paste #2951294: Sort out and fix language fallback inconsistencies and would be happy if you comment there on the updated IS.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

Edit:
Sadly I have to revoke my comment. Saving the unpublished content leads to "Access denied" for the published content in original language again. So the problem is not fixed.

Patch #6 fixed this (potential duplicate) issue for me: #2964383: Access denied on content translation and unpublished content with non-english fallback language (see details in #5 there) in Drupal 8.9.11! Everything works as expected for me now.
I only had to clear cache after patching, no further steps required.

#2951294: Sort out and fix language fallback inconsistencies didn't fix it for me @geek-merlin, but I agree it would make a lot of sense to solve both together!

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

hlopes’s picture

hlopes commented

IMO, not just unpublished translations. Might be the case for outdated translations as well.

For example for T&Cs, it's preferable to show the updated content in the original language than the outdated translation.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Needs Review Queue Initiative

From the comments it seems like this isn't ready for review as the approach is still being decided. Let me know if I'm wrong.

hchonov’s picture

hchonov
Primary language Bulgarian
Location Burgas
commented
Related issues: +#3353243: Unpublished entity reference translations use a fallback while unpublished translations of the main entity do not

I've just added a related issue - #3353243: Unpublished entity reference translations use a fallback while unpublished translations of the main entity do not and when making a decision both issues have to be considered.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
commented

We have a site where we don't want language fallback to happen. We made this work by relying on the behavior discussed here. By simply making sure that we have a translation for all languages, even if empty, we can choose which languages the content is available in by setting the unwanted translations to unpublished. This correctly leads to an access denied in our eyes.

I'm not denying the IS makes a valid point about inconsistency, but I am also agreeing with @plach that we can't simply change this behavior. It would kill our site's access logic and probably a lot more site's expected behavior too.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

#3158130: Many calls to ContextRepository::getAvailableContexts() due to entity upcasting is currently implementing basically #3031124: Deprecate the "entity_upcast" operation specified in EntityConverter, although for now more as a proof of concept to see if that works.

Not sure what to reply to #33. Relying on this behaviour is IMHO unreliable, there are many ways to view an entity, like reference fields, views, various API integrations which will all not use the entity_upcast operation. See also my explanation over there. *Maybe* that doesn't apply to your specific site and you never reference an entity that's still being translated from another entity that already is and have no other entry points to view those entities, but it doesn't seem like something that should be relied on on.

If that's the behavior that you want then what about instead implementing access logic that denies view access if the entity language doesn't match the current language, so you essentially never allow access to view an entity "in the wrong language" (if the user isn't allowed to edit, to allow preview). Then you don't need to maintain fake-translations.

In our case, we handle this by displaying a message on top of the content that it's not available in the current language.

I wish entity_upcast would never have existed, I'm pretty certain this was just a misunderstanding of what that operation is meant to be used for, it's undocumented, but I also acknowledge that people now rely on it now that it exists. I don't know what to do about that. A configurable BC layer for this seems quite a bit of complexity.

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
commented

Relying on this behaviour is IMHO unreliable ...

We got to that point where we checked how Drupal works, saw that under certain conditions access would be denied and found that said conditions matched our client's requirements perfectly. At that point we went with it.

Whether this is advisable for all projects is definitely up for discussion, but Drupal supported it without any mention of it being a bad idea or plans for removal of said functionality in the future. If we were to change this outside of a major version, our project and potentially others would definitely suffer.

Which is why I still side with #8 here. We are fortunate enough to be part of the 45 followers here, but breaking BC here will definitely upset more people than those following this issue.

In our case, we handle this by displaying a message on top of the content that it's not available in the current language.

Our client has specific needs about who can translate what entity in certain languages. By combining the behavior exhibited here with Group, we managed to deliver said functionality perfectly. there's bound to be other people who came up with similar solutions.

I wish entity_upcast would never have existed

+1, but I don't have a time machine :)

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
commented

Ran into another issue where we need to be able to deny access to missing translations (which we're currently using aforementioned workaround of unpublished translations for). Documented this on the unofficial parent issue: #2951294-63: Sort out and fix language fallback inconsistencies

Should we go ahead and make that one the parent issue of all the other related issues, including this one?

berdir opened merge request !10219

murilohp’s picture

murilohp commented
StatusFileSize
new 2978048-38-d11.patch3.77 KB

This patch is a static version of the MR to be applied on drupal 11.

oily made their first commit to this issue’s fork.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.