TranslationLanguageRenderer tries to add langcode field to the view for entity types that have no langcode

I have an entity type that does not have a langcode entity key (It is a translation job, it actually has two languages, source and target language, but the job itself doesn't have a language and is also not translatable

I presume that this issue is just about content entities, but I'm wondering if the above premise conflicts with the change being proposed in #2451885: Config entities need to ship with language or are assumed undefined, specifically, this claim from that patch's docs:

+   * ... All configuration entities support third
+   * party settings, so even configuration entities that do not directly
+   * store settings involving text in a human language may have such third
+   * party settings attached. This means configuration entities should be in one
+   * of the configured languages or the built-in English.

Does it make sense for us to say that content entities (which can be extended with fields) are allowed to not have a langcode, but config entities (which can be extended with third party settings) are not?

Do we have any entity type without a language in core?

I'd argue that for those we should on top not allow people to select the TranslationRenderer in the UI.

Regarding #8's suggestion, ... what would happen if the base table for the view was for a translatable entity, and it had a relationship to an entity that wasn't?

Jen has a good point in #11: the Translation renderer should be able to fall back to the Default language behavior if no langcode key is available. This way related non-translatable entities would be handled smoothly even if the translation renderer is configured in the view. I agree it should not be available if the view base table refers to a non-translatable entity type.

+++ b/core/modules/views/src/Entity/Render/TranslationLanguageRenderer.php
@@ -34,12 +34,13 @@ public function query(QueryPluginBase $query) {
+    if ($langcode_key = $this->entityType->getKey('langcode')) {
+      foreach (array('data_table', 'revision_table', 'base_table') as $key) {
+        if ($table = $this->entityType->get($key)) {

I know this line was not introduced here, but we should be using TableMappingInterface::getFieldTableName($langcode_key) now.

Setting to NW also for tests.

I rerolled the patch.

Thanks, setting back to needs work per #12.

Does not seem to be actually worked on.

Done, I'm not sure if I did this in the best way, but it works.
Now I will work on the tests.

Core should support content entities without langcode key
\Drupal\Core\Entity\Entity::language() already returns \Drupal\Core\Language\LanguageInterface::LANGCODE_NOT_SPECIFIED when no langcode key provided by annotation.

Also views needs to care about relation, and do not try to add non-existing field to query

There's a lot of reasons to have conetnt entities in contrib without language.

the Translation renderer should be able to fall back to the Default language behaviour if no langcode key is available

+1

Rerolled and fixed some table mapping calls... Also, changed wrong variable type in RendererBase.
Tests are still needed.

^ As there is no loop anymore.

Yeah, no loop => no break.

Leaving at Needs Work because this still needs a test.

Adding tests... Btw, not sure whether core/modules/language/src/Tests/ is right place for this test.

This test looks right to me!

But I'm not sure if it reproduces the problem in this issue. I think it does, but we really need an issue summary update, because the issue summary is difficult to understand (kind of garbled or incomplete or ... anyway not great).

Minor issue with the test:

+++ b/core/modules/language/src/Tests/LanguageViewsTest.php
@@ -0,0 +1,78 @@
+ * Tests the view creation of test language entities.

This should probably have more correct information about the purpose of the test, which is to verify that entities without translation don't break Views. I don't think it's really about test language entities.

Moved the test to View module and improved it a bit with API calls.

Updated the issue summary. Hope it's a bit more clear how to reproduce the bug.

Thanks for the summary update! Much nicer.

Looks perfect now!

This all looks good, but:

+++ b/core/modules/views/src/Entity/Render/TranslationLanguageRenderer.php
@@ -35,12 +35,13 @@ public function query(QueryPluginBase $query, $relationship = NULL) {
     // If the data table is defined, we use the translation language as render
     // language, otherwise we fall back to the default entity language, which is
     // stored in the revision table for revisionable entity types.

It seems like this code comment needs to be revised, since the code below it got revised. I don't think it matches any more.

Sure!

+++ b/core/modules/views/src/Entity/Render/TranslationLanguageRenderer.php
@@ -27,21 +27,17 @@ class TranslationLanguageRenderer extends EntityTranslationRendererBase {
+    // Do not alter the query in case the site is not multilingual or the entity
+    // type has no langcode key.

Nitpick: this comment line goes over 80 chars.

I would also change "in case" to "if", which would bring it down below 80 chars. ;)

Or maybe explain what the method is actually doing... something like:

In order to render in the translation language of the entity, we need to add the language code of the entity to the query. Skip if the site is not multilingual or the entity is not translatable.

Anyway, this is just the comment. I think the code looks great! Very clear.

Added the comment above.

Thanks! But:

+++ b/core/modules/views/src/Entity/Render/TranslationLanguageRenderer.php
@@ -27,21 +27,18 @@ class TranslationLanguageRenderer extends EntityTranslationRendererBase {
+    // add the language code of the entity to the query. Skip if the site is not

nitpick: comment line over 80 characters, please rewrap.

My IDE says that it's just 80 characters and that it's fine.

Hm. The line is 80 characters plus 1 for the \n at the end, totalling 81. dreditor complains, and that is what we normally go by... can you please rewrap? Thanks!

Ok, done. I didn't know that, every day it's a school day.

Thanks! but that patch is missing files
core/modules/language/tests/language_test/config/optional/views.view.no_entity_translation_view.yml
core/modules/views/src/Tests/Entity/ViewNonTranslatableEntityTest.php
from the previous patch.

Sorry, I didn't realize. My gitignore did not add them :(
Now should be fine.

Thanks! Yes, you have to be careful when making a patch, if there are new files added rather than just changes to existing files.

Anyway... the latest patch looks perfect. As in previous patches, we have a clear test that fails without the patch, and passes with the patch. The code is cleaner than it was, and the comments are clear. Let's get this in.

It should be RC eligible -- no API change, no disruption, just a bug fix and I think it's a contrib blocker though I'm not the reporter so I'm not sure. Adding RC eval notes to issue summary.

Discussed with the other committers, and +1 to this being sufficiently non-disruptive and sufficiently beneficial in terms of solving a fatal bug for the conditions that trigger it.

Committed b412acf and pushed to 8.0.x. Thanks!